> 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/java.base/windows/native/libjava/c...
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 16 additional
commits since the last revision:
- Go back to java.nio.file.Path::toRealPath
After JDK-8355342 (1f08a3ede2445fb05d9700a1293d681ca89cbf5b)
File::getCanonicalFile is no longer resolving symlinks in Windows, plus
there is some reluctance from core-libs maintainers about relying on
specific java.io.File behaviour with regard to parent directories
permissions.
On the other hand, path resolution has been moved to a place where it's
strictly needed (when resolving a relative include directive), and this
should mitigate the majority of the use-cases, being an improvement with
respect to the current status quo.
Given the previous, I think that Path::toRealPath in its current
location is a good compromise.
This also reverts commit 7abb62c069ad35f4ec34f6cd9b9f6d05febceecc, since
we are no longer supporting that use-case.
- Merge openjdk:master into franferrax:JDK-8352728
- 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
- ... and 6 more: https://git.openjdk.org/jdk/compare/52e09f85...e80a28ea
-------------
Changes:
- all: https://git.openjdk.org/jdk/pull/24465/files
- new: https://git.openjdk.org/jdk/pull/24465/files/b5063f00..e80a28ea
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=24465&range=11
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=24465&range=10-11
Stats: 1474 lines in 48 files changed: 720 ins; 414 del; 340 mod
Patch: https://git.openjdk.org/jdk/pull/24465.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/24465/head:pull/24465
PR: https://git.openjdk.org/jdk/pull/24465
