[jdk16] RFR: 8259028: ClassCastException when using custom filesystem with wrapper FileChannel impl
This patch tweaks `MemorySegment::mapFile` so that it will throw `IllegalArgumentException` whenever the path to be mapped is associated with a custom file system provider. The check in the implementation is heavily borrowed by what `UnixDomainSocketAddress::of(Path)` does (thanks Alan for the tip!). Not only we have to check if file system is the default one, but also if the default FS belongs to java.base (since that can be overridden). The test simply check that paths coming from the (internal) JRT file system are not supported by the factory. ------------- Commit messages: - Tweak javadoc - Clarify javadoc of MemorySegment::mapFile w.r.t. custom file systems Changes: https://git.openjdk.java.net/jdk16/pull/90/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk16&pr=90&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8259028 Stats: 22 lines in 3 files changed: 16 ins; 1 del; 5 mod Patch: https://git.openjdk.java.net/jdk16/pull/90.diff Fetch: git fetch https://git.openjdk.java.net/jdk16 pull/90/head:pull/90 PR: https://git.openjdk.java.net/jdk16/pull/90
On Wed, 6 Jan 2021 16:10:12 GMT, Maurizio Cimadamore <mcimadamore@openjdk.org> wrote:
This patch tweaks `MemorySegment::mapFile` so that it will throw `IllegalArgumentException` whenever the path to be mapped is associated with a custom file system provider.
The check in the implementation is heavily borrowed by what `UnixDomainSocketAddress::of(Path)` does (thanks Alan for the tip!). Not only we have to check if file system is the default one, but also if the default FS belongs to java.base (since that can be overridden).
The test simply check that paths coming from the (internal) JRT file system are not supported by the factory.
CSR: https://bugs.openjdk.java.net/browse/JDK-8259321 ------------- PR: https://git.openjdk.java.net/jdk16/pull/90
This patch tweaks `MemorySegment::mapFile` so that it will throw `IllegalArgumentException` whenever the path to be mapped is associated with a custom file system provider.
The check in the implementation is heavily borrowed by what `UnixDomainSocketAddress::of(Path)` does (thanks Alan for the tip!). Not only we have to check if file system is the default one, but also if the default FS belongs to java.base (since that can be overridden).
The test simply check that paths coming from the (internal) JRT file system are not supported by the factory.
Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Simplify test not to depend on JDK internals ------------- Changes: - all: https://git.openjdk.java.net/jdk16/pull/90/files - new: https://git.openjdk.java.net/jdk16/pull/90/files/4ff84b5b..aaea5ece Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk16&pr=90&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk16&pr=90&range=00-01 Stats: 3 lines in 1 file changed: 0 ins; 2 del; 1 mod Patch: https://git.openjdk.java.net/jdk16/pull/90.diff Fetch: git fetch https://git.openjdk.java.net/jdk16 pull/90/head:pull/90 PR: https://git.openjdk.java.net/jdk16/pull/90
On Wed, 6 Jan 2021 16:32:17 GMT, Maurizio Cimadamore <mcimadamore@openjdk.org> wrote:
This patch tweaks `MemorySegment::mapFile` so that it will throw `IllegalArgumentException` whenever the path to be mapped is associated with a custom file system provider.
The check in the implementation is heavily borrowed by what `UnixDomainSocketAddress::of(Path)` does (thanks Alan for the tip!). Not only we have to check if file system is the default one, but also if the default FS belongs to java.base (since that can be overridden).
The test simply check that paths coming from the (internal) JRT file system are not supported by the factory.
Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision:
Simplify test not to depend on JDK internals
Marked as reviewed by chegar (Reviewer). ------------- PR: https://git.openjdk.java.net/jdk16/pull/90
On Wed, 6 Jan 2021 16:32:17 GMT, Maurizio Cimadamore <mcimadamore@openjdk.org> wrote:
This patch tweaks `MemorySegment::mapFile` so that it will throw `IllegalArgumentException` whenever the path to be mapped is associated with a custom file system provider.
The check in the implementation is heavily borrowed by what `UnixDomainSocketAddress::of(Path)` does (thanks Alan for the tip!). Not only we have to check if file system is the default one, but also if the default FS belongs to java.base (since that can be overridden).
The test simply check that paths coming from the (internal) JRT file system are not supported by the factory.
Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision:
Simplify test not to depend on JDK internals
Marked as reviewed by alanb (Reviewer). ------------- PR: https://git.openjdk.java.net/jdk16/pull/90
On Wed, 6 Jan 2021 16:32:17 GMT, Maurizio Cimadamore <mcimadamore@openjdk.org> wrote:
This patch tweaks `MemorySegment::mapFile` so that it will throw `IllegalArgumentException` whenever the path to be mapped is associated with a custom file system provider.
The check in the implementation is heavily borrowed by what `UnixDomainSocketAddress::of(Path)` does (thanks Alan for the tip!). Not only we have to check if file system is the default one, but also if the default FS belongs to java.base (since that can be overridden).
The test simply check that paths coming from the (internal) JRT file system are not supported by the factory.
Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision:
Simplify test not to depend on JDK internals
Looks fine to me. Another option would have been to just check the class with instanceof. If the FileChannel returned by the path's filesystem has wrong type, it won't work. If we have some custom filesystem that just returns the original FileChannelImpl but does some additional checks before or after (e.g to hide some files from user), this would still work. But I think it's better to be clear here and deny anything which is not default operating system filesystem, until we have a better solution. In Lucene I added some hack that removes our test-only filesystem wrappers from the path. I will later go the route to mark those path instances with some interface Unwrappable that provides a method unwrap(). ------------- Marked as reviewed by uschindler (Author). PR: https://git.openjdk.java.net/jdk16/pull/90
On Wed, 6 Jan 2021 17:51:20 GMT, Uwe Schindler <uschindler@openjdk.org> wrote:
Another option would have been to just check the class with instanceof. If the FileChannel returned by the path's filesystem has wrong type, it won't work. If we have some custom filesystem that just returns the original FileChannelImpl but does some additional checks before or after (e.g to hide some files from user), this would still work.
We considered this - but we opted for better clarity and alignment with existing APIs. ------------- PR: https://git.openjdk.java.net/jdk16/pull/90
On Wed, 6 Jan 2021 16:10:12 GMT, Maurizio Cimadamore <mcimadamore@openjdk.org> wrote:
This patch tweaks `MemorySegment::mapFile` so that it will throw `IllegalArgumentException` whenever the path to be mapped is associated with a custom file system provider.
The check in the implementation is heavily borrowed by what `UnixDomainSocketAddress::of(Path)` does (thanks Alan for the tip!). Not only we have to check if file system is the default one, but also if the default FS belongs to java.base (since that can be overridden).
The test simply check that paths coming from the (internal) JRT file system are not supported by the factory.
This pull request has now been integrated. Changeset: d60a937e Author: Maurizio Cimadamore <mcimadamore@openjdk.org> URL: https://git.openjdk.java.net/jdk16/commit/d60a937e Stats: 20 lines in 3 files changed: 14 ins; 1 del; 5 mod 8259028: ClassCastException when using custom filesystem with wrapper FileChannel impl Reviewed-by: chegar, alanb, uschindler ------------- PR: https://git.openjdk.java.net/jdk16/pull/90
participants (4)
-
Alan Bateman
-
Chris Hegarty
-
Maurizio Cimadamore
-
Uwe Schindler