On Tue, 11 Jun 2024 17:03:58 GMT, Weijun Wang <wei...@openjdk.org> wrote:
>> Kevin Walls has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Sean comments
>
> src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java
> line 1301:
>
>> 1299: }
>> 1300: };
>> 1301: if (acc == null) {
>
> This is a comment to all the code changes in this PR. I would recommend we
> make use of the `allowSecurityManager()` call everywhere when the behavior is
> different, like this:
>
> if (SharedSecrets.getJavaLangAccess().allowSecurityManager()) {
> if (acc == null)
> return action.run();
> else
> return AccessController.doPrivileged(action, acc);
> } else {
> if (subject == null)
> return action.run();
> else
> return Subject.doAs(subject, action);
> }
>
> This makes me much more confident.
Thanks I'll go through the above comments and update - some of the changes I
see are unnecessary and from when I was trying migrating to callAs, and
doPrivileged, and yes they can be simpler.
On the allowSecurityMananger check:
If we check in e.g. RMIConnectionImpl constructor, and if SM is allowed we
assign an ACC.
Later, we know if we have a non-null ACC then SM is allowed. Are you saying we
should then check allowSecurityManager again?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1635291389
