daniellansun commented on code in PR #2755: URL: https://github.com/apache/groovy/pull/2755#discussion_r3737292737
########## src/main/java/org/apache/groovy/util/HiddenClassDefiner.java: ########## @@ -0,0 +1,215 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.groovy.util; + +import org.codehaus.groovy.control.CompilerConfiguration; +import org.objectweb.asm.ClassReader; +import org.objectweb.asm.ClassWriter; +import org.objectweb.asm.commons.ClassRemapper; +import org.objectweb.asm.commons.SimpleRemapper; + +import java.lang.invoke.MethodHandles; +import java.lang.invoke.MethodHandles.Lookup; +import java.util.Collections; + +/** + * Central facility for defining <em>hidden classes</em> + * (<a href="https://openjdk.org/jeps/371";>JEP 371</a>). + * + * <h2>Lookup ownership (read this first)</h2> + * <p>{@link MethodHandles#lookup()} is <em>caller-sensitive</em>: it returns a + * full-privilege lookup only for the class that literally contains the call. + * A lookup captured in this utility therefore only has full privilege for + * {@code HiddenClassDefiner} itself — never for arbitrary foreign classes + * (for example {@code java.lang.String} in {@code java.base}). + * + * <p>Consequently the <strong>preferred</strong> entry point is + * {@link #tryDefineNestmate(Lookup, byte[], boolean)}, where the caller + * supplies a {@link Lookup} obtained inside the intended nest-host class: + * <pre>{@code + * // Inside the class that should host the hidden nestmate: + * private static final MethodHandles.Lookup LOOKUP = MethodHandles.lookup(); + * + * Class<?> hidden = HiddenClassDefiner.tryDefineNestmate(LOOKUP, bytecode, false); + * if (hidden == null) { + * // fall back to ClassLoader.defineClass(...) + * } + * }</pre> + * + * <p>The overload {@link #tryDefineNestmate(Class, byte[], boolean)} is a + * <em>best-effort</em> helper for foreign hosts (user classes Groovy does not + * control). It uses {@link MethodHandles#privateLookupIn(Class, Lookup)} from + * this class and therefore succeeds only when the host's package is accessible + * to Groovy's module — typically true for unnamed-module application classes, + * and typically false for sealed / unopened packages such as those in + * {@code java.base}. Callers must always handle a {@code null} result. + * + * <h2>What this utility centralises</h2> + * <ul> + * <li>{@code NESTMATE} + weak-lifecycle policy for dynamic Groovy classes;</li> + * <li>rewriting {@code this_class} into the lookup class's package (required + * by {@link Lookup#defineHiddenClass});</li> + * <li>a soft API that returns {@code null} on the <em>expected</em> failure + * modes ({@link IllegalAccessException}, {@link IllegalArgumentException}, + * {@link SecurityException}, {@link LinkageError}) so call sites fall back + * to {@link ClassLoader#defineClass} with one null check. Unexpected + * failures (e.g. programming errors in callers) are not swallowed as a + * blanket {@link RuntimeException}.</li> + * </ul> + * + * <h2>Kill switch</h2> + * <p>{@code -Dgroovy.hidden.classes.disable=true} forces every {@code try*} + * method to return {@code null}. + * + * @since 6.0.0 + * @see Lookup#defineHiddenClass(byte[], boolean, Lookup.ClassOption...) + */ +public final class HiddenClassDefiner { + + /** System property that disables hidden-class definitions. */ + public static final String PROPERTY_DISABLE = "groovy.hidden.classes.disable"; + + /** + * {@code true} when hidden-class definitions are globally disabled. + * Evaluated once at class-init so hot paths pay no property-lookup cost. + */ + public static final boolean HIDDEN_CLASSES_DISABLED = + SystemUtil.getBooleanSafe(PROPERTY_DISABLE, false); + + /** + * Lookup for <em>this</em> class only — used exclusively as the caller + * argument to {@link MethodHandles#privateLookupIn(Class, Lookup)} in the + * foreign-host overload. It is never used as a nest host for user code. + */ + private static final Lookup LOOKUP = MethodHandles.lookup(); + + /** Nestmate + weak (eager unloading) — the only option set production uses. */ + private static final Lookup.ClassOption[] NESTMATE_WEAK = + new Lookup.ClassOption[]{Lookup.ClassOption.NESTMATE}; + + private HiddenClassDefiner() { + } + + /** + * @return {@code true} when hidden-class definition is enabled + * (the default unless {@value #PROPERTY_DISABLE} is set) + */ + public static boolean isEnabled() { + return !HIDDEN_CLASSES_DISABLED; + } + + /** + * Defines {@code bytes} as a hidden nestmate of {@code lookup.lookupClass()} + * with a weak lifecycle. + * + * <p>The lookup must have been obtained via {@link MethodHandles#lookup()} + * inside the intended nest-host class (or otherwise carry full privilege + * for that class). The class-file package is rewritten to match the lookup + * class before definition. + * + * @param lookup full-privilege lookup for the nest host + * @param bytes class-file bytes + * @param initialize {@code true} to run {@code <clinit>} immediately + * @return the hidden class, or {@code null} if definition is not possible + */ + public static Class<?> tryDefineNestmate( + final Lookup lookup, + final byte[] bytes, + final boolean initialize) { + if (HIDDEN_CLASSES_DISABLED || lookup == null || bytes == null) { Review Comment: Thanks — agreed on the module-level point, and it is a fair stress test for this design. **`MethodHandles.lookup()` and private access** You are right that every production call site sits in the Groovy runtime (module C). So `lookup()` always carries the same *module* rights: public API C can already see, plus reflective access only where packages are open to C. Capturing the lookup in `ProxyGeneratorAdapter` rather than elsewhere in the runtime does not open module A. `privateLookupIn` into an unopened package fails; a nestmate of `String` is the canonical example, and we treat that as expected. What still differs between those lookups is only the *nest host* (package, defining loader, protection domain, nest membership). That still matters for unloadability and linkage, but it is not a privilege escalation into a foreign module. We now pre-filter foreign hosts with `Module.isOpen` (`canAttemptPrivateLookup`, `@Internal`) so unopened platform types never pay for a guaranteed `IllegalAccessException`, and soft-fail to `null` so callers fall back to `ClassLoader.defineClass`. **`ReflectorLoader`** Agreed — no production callers. It is `@Deprecated(since = "6.0.0", forRemoval = true)`, the hidden-class path is gone (plain `defineClass` only), and tests are reduced to binary-compat smoke. Happy to remove the class entirely in a follow-up when we drop that surface. **`ProxyGenerator` / accessibility of interfaces** Public types such as `Map` are fine as *nominal* supers/interfaces: they are exported API, and C can already resolve them without `privateLookupIn`. The hard case is not “public method on a type in another module”, but private reflective entry into a package that module never opened to C. We do not claim nestmates solve that. Under A (Java library) / B (Groovy program) / C (runtime): 1. **Caller-owned nestmate** (of a type in C) — when every type the bytecode names is resolvable from C’s loader. 2. **Foreign host** — best-effort `privateLookupIn`, only when the host package is open to C (typical for unnamed-module B; not for strongly encapsulated A or `java.base`). 3. **Visible `defineClass`** — intentional path when neither nestmate option applies. **Is the nestmate path useful enough outside B and C?** Honestly, outside open/unnamed B and C it mostly is not — and that is deliberate. Nestmates are for non-discoverable, eagerly unloadable *generated* classes where C (or B) can already host them, not a tunnel into A’s encapsulation. For restricted A we rely on the visible fallback, same as before this PR for those cases. For the goals of this change (less class-space pollution, better unloading, clear soft-fail under modules and native image), I think that coverage is enough. Happy to resolve on that basis; please say if you still see a gap under the A/B/C layout. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
