RFR: 8344319: SM cleanup in jdk.dynlink module

[Attila Szegedi]

On Fri, 15 Nov 2024 16:38:53 GMT, Roger Riggs <rriggs at openjdk.org> wrote:

> Refactor to remove use of SecurityManager

Hey Roger, thanks for doing this no doubt unglamorous maintenance task. I reviewed it, and it mostly looks good, there's few things I thought about maybe taking a step further (and one thing I'd prefer was taken a step back 😃)

src/jdk.dynalink/share/classes/jdk/dynalink/DynamicLinkerFactory.java line 462:

> 460: 
> 461:     private static ClassLoader getThreadContextClassLoader() {
> 462:         return Thread.currentThread().getContextClassLoader();

This could get inlined. It's only invoked from a single location, and the sole purpose of it was to evaluate the expression in doPrivileged.

src/jdk.dynalink/share/classes/jdk/dynalink/SecureLookupSupplier.java line 46:

> 44:      * {@link #getLookup()} method.
> 45:      */
> 46:     public static final String GET_LOOKUP_PERMISSION_NAME = "dynalink.getLookup";

This public member is [referenced from Nashorn](https://github.com/search?q=repo%3Aopenjdk%2Fnashorn%20GET_LOOKUP_PERMISSION_NAME&type=code). If we remove it, Nashorn will have to attempt to look it up reflectively as long as it supports older versions of Java. (Presuming the `java.security.AccessController` and related classes aren't also removed altogether. Are they?)

FWIW, as long as we're making breaking changes, this whole class could be removed – all it did was serve as a secure gate to access to a `MethodHandles.Lookup`. References to it could be replaced by the lookup object itself. I can understand if this might be too much work for this PR, though.

On the other hand, if we do _not_ remove the whole class, I'd prefer to keep the public string constant for the permission name so that we don't break binary compatibility for whoever might be referencing it (specifically, Nashorn.)

src/jdk.dynalink/share/classes/jdk/dynalink/linker/GuardingDynamicLinkerExporter.java line 53:

> 51:      * anymore as the Security Manager is no longer supported.
> 52:      */
> 53:     public static final String AUTOLOAD_PERMISSION_NAME = "dynalink.exportLinkersAutomatically";

This at least is not used externally, it seems so I guess we can remove it no strings attached.

src/jdk.dynalink/share/classes/jdk/dynalink/linker/GuardingDynamicLinkerExporter.java line 65:

> 63:         if (sm != null) {
> 64:             sm.checkPermission(AUTOLOAD_PERMISSION);
> 65:         }

We can probably remove the whole empty default constructor now. The class is abstract, the autogenerated default constructor will be protected anyway.

src/jdk.dynalink/share/classes/jdk/dynalink/linker/GuardingTypeConverterFactory.java line 121:

> 119:      * method on the passed supplier will be subject to the same security checks
> 120:      * as {@link SecureLookupSupplier#getLookup()}. An implementation should avoid
> 121:      * retrieving the lookup if it is not needed.

This whole passage starting with "Invoking the…" and ending with "… is not needed" should be removed now.

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

Changes requested by attila (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/22152#pullrequestreview-2440557095
PR Review Comment: https://git.openjdk.org/jdk/pull/22152#discussion_r1844995516
PR Review Comment: https://git.openjdk.org/jdk/pull/22152#discussion_r1844997321
PR Review Comment: https://git.openjdk.org/jdk/pull/22152#discussion_r1845001347
PR Review Comment: https://git.openjdk.org/jdk/pull/22152#discussion_r1845003011
PR Review Comment: https://git.openjdk.org/jdk/pull/22152#discussion_r1845009155