On Thu, 14 Nov 2024 14:43:50 GMT, Eirik Bjørsnøs <eir...@openjdk.org> wrote:
>> Please review this PR which cleans up SecurityManager-related code following
>> JEP-486 integraion.
>>
>> * `ZipFileSystem` is updated to not perform `AccessController::doPrivileged`
>> * `ZipFileSystemProvider` is updated to not perform
>> `AccessController::doPrivileged`
>> * The test `TestPosix` is updated to perform `AccessController.doPrivileged`
>>
>> This change should be relatively straight-forward to review. Reviewers may
>> want to look extra close at the exception-unwrapping code in
>> `ZipFileSystem::initOwner` and `ZipFileSystem::initGroup`.
>>
>> Testing: Tests in `test/jdk/jdk/nio/zipfs` run green locally.
>
> Eirik Bjørsnøs has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Update debug logging to not reference privileged operations
src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 252:
> 250: if (o == null) {
> 251: try {
> 252: PrivilegedExceptionAction<UserPrincipal> pa =
> ()->Files.getOwner(zfpath);
Can you also remove the `@SuppressWarnings("removal")` on line 239?
src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 294:
> 292: return defaultOwner::getName;
> 293: }
> 294: PrivilegedExceptionAction<GroupPrincipal> pa =
> ()->zfpv.readAttributes().group();
Can you also remove the @SuppressWarnings("removal") on line 269?
src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 483:
> 481: beginWrite(); // lock and sync
> 482: try {
> 483:
> AccessController.doPrivileged((PrivilegedExceptionAction<Void>)() -> {
Can you also remove the @SuppressWarnings("removal") on line 442?
src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystemProvider.java line 317:
> 315:
> 316: //////////////////////////////////////////////////////////////
> 317: @SuppressWarnings("removal")
You can remove the annotation now.
test/jdk/jdk/nio/zipfs/TestPosix.java line 236:
> 234: }
> 235: return zfpv.readAttributes().group().getName();
> 236: } catch (UnsupportedOperationException | NoSuchFileException e) {
In the prior code, I think `NoSuchFileException` would have been the cause of
the `SecurityException`, so the code would have returned `null`. Just wondering
why you changed it here.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/22101#discussion_r1842463056
PR Review Comment: https://git.openjdk.org/jdk/pull/22101#discussion_r1842467210
PR Review Comment: https://git.openjdk.org/jdk/pull/22101#discussion_r1842466546
PR Review Comment: https://git.openjdk.org/jdk/pull/22101#discussion_r1842445637
PR Review Comment: https://git.openjdk.org/jdk/pull/22101#discussion_r1842456357
