On Fri, 22 Nov 2024 19:19:37 GMT, Kevin Walls <kev...@openjdk.org> wrote:
>> Remove redundant SecurityManager, AccessController references
>> (following on from JDK-8338411: Implement JEP 486: Permanently Disable the
>> Security Manager).
>
> Kevin Walls has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Remove last import sun.reflect.misc.ReflectUtil
src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java
line 108:
> 106: ClassLoaderRepository repository =
> mbeanServer.getClassLoaderRepository();
> 107: this.classLoaderWithRepository = new
> ClassLoaderWithRepository(repository, dcl);
> 108: this.defaultContextClassLoader = new
> CombinedClassLoader(Thread.currentThread().getContextClassLoader(), dcl);
`this.` are not needed
src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java
line 1231:
> 1229: return
> getServerNotifFwd().fetchNotifs(clientSequenceNumber, timeout,
> maxNotifications);
> 1230: } else {
> 1231: return Subject.callAs(subject, () ->
> getServerNotifFwd().fetchNotifs(clientSequenceNumber, timeout,
> maxNotifications));
This call may throw CompletionException instead of PrivilegedActionException
That's strange that this case does not go through standard
`doPrivilegedOperation` way.
I think this class needs to be improved (most likely as a separate PR).
For most of operations it created "operation id" and creates array of
arguments, but then `doOperation` parses the id and arguments and performs
required action. It would be much simpler if doPrivilegedOperation/doOperation
accept Callable.
src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java
line 1313:
> 1311: Throwable e = ce.getCause();
> 1312: if (e instanceof SecurityException) {
> 1313: throw (SecurityException) e;
Suggestion:
if (e instanceof SecurityException se) {
throw se;
src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java
line 1315:
> 1313: throw (SecurityException) e;
> 1314: } else if (e instanceof Exception) {
> 1315: throw new PrivilegedActionException((Exception)
> e);
Suggestion:
} else if (e instanceof Exception e1) {
throw new PrivilegedActionException(e1);
src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java
line 1484:
> 1482: setCcl(old);
> 1483: }
> 1484: } catch (Exception e) {
Need to handle CompletionException (or maybe at Subject.callAs call)?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/22270#discussion_r1854766377
PR Review Comment: https://git.openjdk.org/jdk/pull/22270#discussion_r1854784308
PR Review Comment: https://git.openjdk.org/jdk/pull/22270#discussion_r1854786790
PR Review Comment: https://git.openjdk.org/jdk/pull/22270#discussion_r1854787507
PR Review Comment: https://git.openjdk.org/jdk/pull/22270#discussion_r1854789725
