RFR: 8352092: -XX:AOTMode=record crashes with InstanceKlass in allocated state

[David Holmes]

On Sat, 22 Mar 2025 04:39:40 GMT, Ioi Lam <iklam at openjdk.org> wrote:

> Please review this small fix for a crash that can be caused only with JNI `DefineClass()` calls, which can leave a class in the `allocated` state inside a `ClassLoaderData` when the class is in a prohibited package such as `java/foo`.
> 
> The bug was found when running the JCK with an AOT cache. I tried to write a jtreg regression test but it's not possible to use pure Java code to reproduce this condition -- attempts by Java code to define classes in a prohibited package will be blocked from entering the ClassFileParser.

Marked as reviewed by dholmes (Reviewer).

tl;dr  Okay the presented fix is acceptable.

I went through the JVMS to try and determine exactly when a prohibited package name should be detected and cause the class to be rejected. Unfortunately this is not clearly stated anywhere. JVMS 5.3.5 gives the process whereby a purported class representation is used to derive a class, and the checking of the package name does not occur there. That section ends with:

> If no exception is thrown in steps 1-4, then derivation of the class or interface C
succeeds. The Java Virtual Machine marks C to have L as its defining loader, records
that L is an initiating loader of C (§5.3.4), and creates C in the method area (§2.5.4).

which is what the classfile parsing code has done. JVMS 5.3.6 defines module and layers and it is there that we have the rule that packages can not be split and thus a user-defined classloader cannot "load" (successfully) a class in the java.base module (and by implication the `java.*` package tree). But there is no direction on exactly when this package  check is to be performed - or even the error to throw in such a case. The JVMS effectively delegates the semantics to `ModuleLayer.defineModules` but that also does not address this issue. In fact the only places it is stated explicitly that attempts to define a class in the `java.*` namespace will throw a `SecurityException`  is in `ClassLoader.defineClass` and JNI `DefineClass`. The former performs the package check upfront. The latter allows the JVM to do the package check "somewhere".

I think a case can be made that JNI `DefineClass` should eagerly check the package name, as the Java code does., and thus avoid leaving a "partially formed" class defined in the `classLoaderData`. However, there are practical problems with actually trying to do such a check in the JNI code - the name may not be available until classfile parsing has identified it. Additionally, although this seems quite reasonable behaviour for something that is an error case, it is a change in behaviour that would have to be scrutinized. As such it seems out-of-scope to suggest such a  change in this PR.

FWIW I think the JNI spec for `DefineClass` should have been expressed to act as-if the `defineClass` method of the supplied loader were invoked. The JVMS could then expect that any purported class representation has already been approved by the associated classloader and the VM would not need to do any package checks. (Though again the fact the name of the class may only be available inside the presented bytes makes this awkward, as some parsing is needed to get to it.)

That all said, my analysis leaves me a bit puzzled about the proposed fix of checking `is_loaded()`' because from my reading of 5.3.5 the class is actually loaded. So it seems the `instanceKlass` definition of `is_loaded` does not exactly align with JVMS. But again this is not something for this PR to try and rectify.

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

PR Review: https://git.openjdk.org/jdk/pull/24172#pullrequestreview-2712212063
PR Comment: https://git.openjdk.org/jdk/pull/24172#issuecomment-2749954137