Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8344183: (zipfs) SecurityManager cleanup in the ZipFS area #22101

Closed
wants to merge 4 commits into from
+28 −82
Closed

8344183: (zipfs) SecurityManager cleanup in the ZipFS area #22101

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d8936a5
Cleanup SM-dependent code in the ZipFs area
eirbjo Nov 14, 2024
37104a9
Update debug logging to not reference privileged operations
eirbjo Nov 14, 2024
513000d
Remove @SuppressWarnings("removal")
eirbjo Nov 14, 2024
c13e551
Revert catching of NoSuchFileException in expectedDefaultOwner and ex…
eirbjo Nov 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions test/jdk/jdk/nio/zipfs/TestPosix.java
View file
Original file line number Diff line number Diff line change
@@ -221,7 +221,7 @@ private static String expectedDefaultOwner(Path zf) {
return System.getProperty("user.name");
} catch (IOException e) {
System.out.println("Caught " + e.getClass().getName() + "(" + e.getMessage() +
") when running a privileged operation to get the default owner.");
") when getting the default owner.");
return null;
}
}
@@ -236,7 +236,7 @@ private static String expectedDefaultGroup(Path zf, String defaultOwner) {
} catch (UnsupportedOperationException | NoSuchFileException e) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting catch..

My understanding is that the purpose of expectedDefaultOwner and expectedDefaultGroup is to mimic the behavior of ZipFileSystem:initOwner and ZipFileSystem::initGroup.

The ZipFileSystem methods both check for UnsupportedOperationException.getCause() instanceof NoSuchFileException, while the TestPosix expect methods do not. This is why I added them, to make them consistent with the implementation code.

This inconsistency is a however a preexisting issue. Attempting to fixing them here might just clutter this review. So I have reverted the catch for NoSuchFileException.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, but your change did not check UnsupportedOperationException.getCause() instanceof NoSuchFileException, it just checked if the exception was NoSuchFileException.

In any case, I agree, best to preserve the existing code as much as possible.

Copy link
Contributor Author

@eirbjo eirbjo Nov 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, but your change did not check UnsupportedOperationException.getCause() instanceof NoSuchFileException, it just checked if the exception was NoSuchFileException.

For the record, there was a typo in my comment, I meant to say PrivilegedActionException. getCause() instanceof ...

...but yes, this doesn't matter now that we agree to not fix this code.

return defaultOwner;
} catch (IOException e) {
System.out.println("Caught an exception when running a privileged operation to get the default group.");
System.out.println("Caught an exception when getting the default group.");
e.printStackTrace();
return null;
}