On Thu, 1 May 2025 10:35:55 GMT, Alan Bateman <al...@openjdk.org> wrote:
>>> File::getCanonicalPath seems to take the best-effort approach (both in >>> Linux and Windows), whereas Path::toRealPath is stricter. >> >> Path::toRealPath is doing the right thing, and consistent with realpath(2). >> The issue with File::getCanonicalXXX is that it is specified to return a >> canonical file even if it doesn't exist, so this is why you see a lot more >> code to compute a result. >> >> Maybe the recursive include check them maybe it should use the file key >> instead. > >> @AlanBateman are you ok with letting the original >> [c6f1d5f](https://github.com/openjdk/jdk/commit/c6f1d5f374bfa9bde75765391d5dae0e8e28b4ab) >> reviewers know of this fix and take a look? Or do you think further >> discussion is needed somewhere else? > > Have you had time to try using the file key to detect the recursive include > case? Hi @AlanBateman, I'm resuming the work on this one. > Have you had time to try using the file key to detect the recursive include > case? By "file key", do you mean `Files.readAttributes(Path.of("..."), BasicFileAttributes.class).fileKey()`? This works on _Linux_ but returns `null` on _Windows_. If you mean using a `java.util.Set<java.io.File>` for circular includes detection, we can use it, but [`File::equals`](https://docs.oracle.com/en/java/javase/24/docs/api/java.base/java/io/File.html#equals(java.lang.Object)) and [`File::hashCode`](https://docs.oracle.com/en/java/javase/24/docs/api/java.base/java/io/File.html#hashCode()) operate on abstract pathnames without accessing the filesystem. This is not a problem as any circular include will eventually repeat verbatim anyway, but please note that we still need to perform symlinks resolution before resolving a relative include. The original [JDK-8352728] problem involves hitting the `java.nio` permissions problem when the master `$JAVA_HOME/conf/security/java.security` file is resolved. We can avoid this by doing the symlinks resolution only when strictly necessary, i.e. when a relative include is found and processed. In that case, we should also resolve symlinks in debugging messages, for a straightforward troubleshooting. However, delayed symlinks resolution with `java.nio` doesn't resolve the permissions problem when a relative include is processed from a symlink/junction properties file. We discovered this additional issue as a variant of the reported one, and is exercised by the current version of `ConfigFileTestDirPermissions` (7abb62c069ad35f4ec34f6cd9b9f6d05febceecc). Finally, if we are going to use `java.io.File`, we should also consider avoiding `java.nio.file.Path`, for a cleaner/unmixed usage of the APIs. All this leaded me to explore and compare the following alternatives, in decreasing order of my personal preference: | Code | Delayed symlinks resolution | Main API (and circular detection) | Symlinks resolution API | Solves the original [JDK-8352728] problem | Solves the 7abb62c069ad35f4ec34f6cd9b9f6d05febceecc variant | |:---------------------------------------------------------------:|:---:|:--------------------:|:--------------------:|:---:|:---:| | Current PR, franferrax@e29e9fc020909ebe5e95ef06e26d420b924976eb | No | `java.nio.file.Path` | `java.io.File` | Yes | Yes | | franferrax@54db2500ec3aef3567217bfc84669e7469882676 | No | `java.io.File` | `java.io.File` | Yes | Yes | | franferrax@a408dc57c75782801590bed31304580d78a1eb66 | Yes | `java.io.File` | `java.io.File` | Yes | Yes | | franferrax@c9a3985c7d11335f3c9b8ab67d166bdc2a16289f | Yes | `java.nio.file.Path` | `java.io.File` | Yes | Yes | | franferrax@fbec2fd80a9930cf4cb00f4f35684f2e2cc2fbf1 | Yes | `java.nio.file.Path` | `java.nio.file.Path` | Yes | No | | Current `master` | No | `java.nio.file.Path` | `java.nio.file.Path` | No | No | NOTEs: * I don't have a strong preference between the first two alternatives, but `java.nio` looks like a newer API, so perhaps preferred over `java.io` * I prefer the alternatives without delayed symlinks resolution because: * They have simpler and more uniform code: don't require an additional resolution for debugging messages * Circular include errors will show the resolved name (if we go with a delayed symlinks resolution alternative, we need to adjust either `ConfigFileTest` or that error message) [JDK-8352728]: https://bugs.openjdk.org/browse/JDK-8352728 "InternalError loading java.security due to Windows parent folder permissions" ------------- PR Comment: https://git.openjdk.org/jdk/pull/24465#issuecomment-3114241142
