On Mon, 17 Jun 2024 15:53:19 GMT, Kevin Walls <kev...@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
