On Tue, 11 Jun 2024 18:04:45 GMT, Kevin Walls <kev...@openjdk.org> wrote:
>> 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?
Yes, the check is very light and it makes sure the old code and new code are
fully separated. It's safe since it can be quite easy to see when SM is allowed
there is no behavior change. In the near future when we finally remove the SM
it will also be simple to remove the old code.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1635352326
