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

[Sean Mullan]

On Mon, 17 Jun 2024 15:53:19 GMT, Kevin Walls <kevinw at openjdk.org> wrote:

>> src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java line 1314:
>> 
>>> 1312:                     return AccessController.doPrivileged(action, acc);
>>> 1313:                 }
>>> 1314:             }
>> 
>> This piece of code is repeated at several places in similar but different forms - sometime we check for `SharedSecrets.getJavaLangAccess().allowSecurityManager()`, sometime for `! SharedSecrets.getJavaLangAccess().allowSecurityManager()`, sometime we check for `acc == null`, sometime for `acc != null` etc.. 
>> I wonder if we could abstract that to some utility method somewhere. Would that make sense? Maybe that's not possible due to using sometime `PrivelegedAction` and sometimes `PrivilegedExceptionAction` - but maybe we could use `PrivilegedExceptionAction` everywhere at the cost of handling `PrivilegedActionException`? 
>> If that doesn't prove useful (if handling `PrivilegedExceptionAction` adds again an equivalent amount of clutter) then maybe we could at least make sure we do the same checks in the same way (typically `acc == null` vs `acc != null`)?
>
> Hi,
> We almost always check !SharedSecrets.getJavaLangAccess().allowSecurityManager() (except in the RMIConnectionImpl contstructor)
> 
> This keeps the "modern" case first (except in that constructor, where it is only one line for each side of the if...).
> 
> This extra checking of SharedSecrets.getJavaLangAccess().allowSecurityManager() is to keep the modern and old style cases distinct, based on other comments here.  It keeps it simple to read I hope, but yes it is a little more verbose than it could be.
> 
> I'm hoping you'll agree that as these checks will be removed anyway, probably with a release, we should delay more extensive cleanup until then.  (I've also  rolled back my noPermissionsACC removal to keep this change away from being a general cleanup.)
> 
> I had a bunch of additional Exception handling for e.g. PrivilegedActionException I think in here, and it wasn't very pretty.  But, it might look better when there are fewer nested ifs in these methods, and when we are properly in the post-SecurityManager world... 8-)
> 
> Whether it checks acc != null or acc == null is based on the existing code.  I think that makes this diff easier to read, but also could be reworked and be made more consistent after SM removal.

Agree with Kevin. To minimize risk, especially since this is to fix a significant regression and we are in RDP1, we are really trying to preserve the existing code as much as possible, even though it could be improved.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1643395182