On Fri, 15 Nov 2024 16:38:53 GMT, Roger Riggs <rri...@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
