-
-
Notifications
You must be signed in to change notification settings - Fork 15.9k
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.
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
CleanerJava9 should be able to do its job even with a SecurityManager… #8204
Conversation
… installed. Motivation: CleanerJava9 currently fails whever a SecurityManager is installed. We should make use of AccessController.doPrivileged(...) so the user can give it the correct rights. Modifications: Use doPrivileged(...) when needed. Result: Fixes #8201.
@jprante PTAL as well |
@normanmaurer handles out of scope? |
@johnou there is a lot other things to fix that also can use handles so I think I would do this as a follow up with a much bigger scope |
} | ||
|
||
private static void freeDirectBufferPrivileged(final ByteBuffer buffer) { | ||
Exception error = AccessController.doPrivileged(new PrivilegedAction<Exception>() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
idle question: why not use PrivilegedExceptionAction ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is not much advantage here.. We will need to unwrap then etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
} catch (InvocationTargetException e) { | ||
return e; | ||
} catch (IllegalAccessException e) { | ||
return e; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could just catch a Throwable
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also honestly I think it will not make any difference. So maybe just keep it ?
… installed.
Motivation:
CleanerJava9 currently fails whever a SecurityManager is installed. We should make use of AccessController.doPrivileged(...) so the user can give it the correct rights.
Modifications:
Use doPrivileged(...) when needed.
Result:
Fixes #8201.