On Wed, 26 Nov 2025 10:15:42 GMT, Francisco Ferrari Bihurriet <[email protected]> wrote:
>> Hi, this is a proposal to fix 8352728. >> >> The main idea is to replace >> [`java.nio.file.Path::toRealPath`](https://docs.oracle.com/en/java/javase/24/docs/api/java.base/java/nio/file/Path.html#toRealPath(java.nio.file.LinkOption...)) >> by >> [`java.io.File::getCanonicalPath`](https://docs.oracle.com/en/java/javase/24/docs/api/java.base/java/io/File.html#getCanonicalPath()) >> for path canonicalization purposes. The rationale behind this decision is >> the following: >> >> 1. In _Windows_, `File::getCanonicalPath` handles restricted permissions in >> parent directories. Contrarily, `Path::toRealPath` fails with >> `AccessDeniedException`. >> 2. In _Linux_, `File::getCanonicalPath` handles non-regular files (e.g. >> `/dev/stdin`). Contrarily, `Path::toRealPath` fails with >> `NoSuchFileException`. >> >> #### Windows Case >> >> @martinuy and I tracked down the `File::getCanonicalPath` vs >> `Path::toRealPath` behaviour differences in _Windows_. Both methods end up >> calling the >> [`FindFirstFileW`](https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-findfirstfilew) >> API inside a loop for each parent directory in the path, until they include >> the leaf: >> >> * >> [`File::getCanonicalPath`](https://github.com/openjdk/jdk/blob/jdk-24-ga/src/java.base/share/classes/java/io/File.java#L618 >> "src/java.base/share/classes/java/io/File.java:618") goes through the >> following stack into `FindFirstFileW`: >> * >> [`WinNTFileSystem::canonicalize`](https://github.com/openjdk/jdk/blob/jdk-24-ga/src/java.base/windows/classes/java/io/WinNTFileSystem.java#L473 >> "src/java.base/windows/classes/java/io/WinNTFileSystem.java:473") >> * >> [`WinNTFileSystem::canonicalize0`](https://github.com/openjdk/jdk/blob/jdk-24-ga/src/java.base/windows/native/libjava/WinNTFileSystem_md.c#L288 >> "src/java.base/windows/native/libjava/WinNTFileSystem_md.c:288") >> * >> [`wcanonicalize`](https://github.com/openjdk/jdk/blob/jdk-24-ga/src/java.base/windows/native/libjava/canonicalize_md.c#L233 >> "src/java.base/windows/native/libjava/canonicalize_md.c:233") (here is the >> loop) >> * If `FindFirstFileW` fails with `ERROR_ACCESS_DENIED`, >> `lastErrorReportable` is consulted, the error is [considered >> non-reportable](https://github.com/openjdk/jdk/blob/jdk-24-ga/src/java.base/windows/native/libjava/canonicalize_md.c#L139 >> "src/java.base/windows/native/libjava/canonicalize_md.c:139") and the >> iteration is stopped >> [here](https://github.com/openjdk/jdk/blob/jdk-24-ga/src/java.base/windows/native/libjava/canonicalize_md.c#L246-L250 >> "src/ja... > > Francisco Ferrari Bihurriet has updated the pull request with a new target > base due to a merge or a rebase. The incremental webrev excludes the > unrelated changes brought in by the merge/rebase. The pull request contains > 14 additional commits since the last revision: > > - Merge openjdk:master into franferrax:JDK-8352728 > - Do symlinks resolution in a single place > > Remove path normalization from debugging messages, only keep it where > strictly necessary. > > Adjust test case to the new debugging messages which contain absolute > but not normalized paths (can contain some ../../ elements). > - Use the right OutputAnalyzer method > > OutputAnalyzer::stderrMatches returns a boolean while > OutputAnalyzer::stderrShouldMatch performs the check. > - Remove path resolution from exception message > - Detect cyclic includes with Files::isSameFile > > checkCyclicInclude() is invoked after we successfully get an InputStream > for the path to avoid skipping the same IOException several times inside > checkCyclicInclude() if the path doesn't exist. > > Also, perform symlinks resolution only in the following cases: > • When we need to resolve a relative include > • For clarity to the user in logging messages > • For clarity to the user in exception messages > > In the first case, the resolution is a requirement, in the last two > cases it is a nice-to-have. But given the last two are exceptional > cases anyway, we let any resolution error bubble up. > - Merge openjdk:master into franferrax:JDK-8352728 > - Introduce a shell test for anonymous pipes > > This use case has been discussed and analyzed in the pull request, but > we didn't have a test case for it. By introducing a test, we make sure > we don't have regressions in this area, regardless of the alternative > we choose to advance with for this fix. > - Merge openjdk:master into franferrax:JDK-8352728 > - Fix typo > - Merge openjdk:master into franferrax:JDK-8352728 > - ... and 4 more: https://git.openjdk.org/jdk/compare/21256aa2...b5063f00 Hi folks, as we know [JDK 26 enters Rampdown Phase One next week](https://mail.openjdk.org/pipermail/jdk-dev/2025-November/010639.html) on Thursday, 4 December. Please let me know if we'll want this fix to move forward before that happens, and please note that soon I will need to focus on the OJVG work. If you prefer, I'm fine with using `path.toRealPath()` instead of `path.toFile().getCanonicalFile().toPath()`. That would keep relative includes broken in _Windows_ UWP Java applications and from _Linux_ anonymous pipes and files (where they don't make a lot of sense anyway). But that is better than the current status quo (_Windows_ UWP Java applications break if they load the `java.security.Security` class). Just let me know if you want to proceed that way and I will, also removing the part of the new test that will break by that change. ------------- PR Comment: https://git.openjdk.org/jdk/pull/24465#issuecomment-3580656663
