RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v3]

[Weijun Wang]

On Tue, 11 Jun 2024 18:04:45 GMT, Kevin Walls <kevinw at 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