RFR: 8212117 : Class.forName may return a reference to a loaded but not linked Class

[David Holmes]

Hi Brent,

Good to see this all come together :)

A couple of comments below.

On 5/09/2019 7:12 am, Brent Christian wrote:
> Hi,
> 
> Please review my fix for JDK-8212117[1].  The webrev is here:
> 
> http://cr.openjdk.java.net/~bchristi/8212117/webrev09/

So to clarify for others any current caller to 
find_class_from_class_loader that passes true for "init" was already 
implicitly asking for a linked class (as you must be linked before you 
can be initialized). With that in mind I would suggest that in 
find_class_from_class_loader you add an assert:

! jclass find_class_from_class_loader(JNIEnv* env, Symbol* name, 
jboolean init, jboolean link,
                                       Handle loader, Handle 
protection_domain,
                                       jboolean throwError, TRAPS) {
+assert((init && link) || !init, "incorrect use of init/link arguments");

just to ensure the callers understand this.

Aside: in looking at this I've spotted another existing bug. JNI 
FindClass is not specified to perform class initialization, but the 
implementation passes init=true!

---

src/java.base/share/classes/java/lang/invoke/MethodHandles.java

I don't see the need for the new note given it already has

* @throws LinkageError if the linkage fails

---

src/java.base/share/classes/sun/launcher/LauncherHelper.java

So to clarify, the launcher would previously load the main class without 
linking via:

    mainClass = LoadMainClass(env, mode, what);
    CHECK_EXCEPTION_NULL_LEAVE(mainClass);

and then it would invoke the main method which implicitly initialized 
which implicitly linked and so any exceptions related to linking would 
be handled when the main thread detaches, and it all gets reported 
nicely. But now that we link earlier the exception could be pending 
after LoadMainClass and so we would "abort". So you catch that 
unexpected exception in the LauncherHelper.

Is AccessControlException the only exception, that is not a 
LinkageError, that might be thrown when linking? I would think a number 
of others are possible - in particular IllegalAccessError might occur 
during verification. Other than the fact a test obviously triggered 
this, it's not clear why ACE should be singled out here. ??

---

test/hotspot/jtreg/serviceability/jvmti/ClassStatus/ClassStatus.java

45     // public static void foo(Foo f) { }

Unclear why this exists. Also the test refers initially to class Foo but 
then uses Foo2 and Foo3. ??

Otherwise all seems good.

> There is also a CSR[2] in need of review.

I've added a couple of comments and will add myself as a reviewer once 
things are near finalized.

Thanks,
David
-----

> 
> The spec for the 2-arg and 3-arg Class.forName() methods states they 
> will "locate, load, and link" the class, however the linking part is not 
> ensured to happen.
> 
> Although the VM spec allows flexibility WRT when classes are linked, 
> this is a corner where the Class.forName() spec is not being upheld. 
> While this is not an issue for most usages,  8181144 [3] demonstrates 
> how this can be a problem (with the debugging interface, in this case).
> 
> This fix ensures that linking happens during the course of 
> Class.forName().  Class.forName() already @throws LinkageError, so no 
> spec change is needed there. MethodHandles.Lookup.findClass() needs a 
> small spec update, due to calling Class.forName with init=false,
> 
> Of course Errors are not required to be caught.  It is therefore 
> possible that the new behavior could introduce previously unseen, 
> possibly unhandled LinkageErrors.  A new VM flag 
> (-XX:+ClassForNameDeferLinking) is introduced to restore the previous 
> behavior (to keep such code running until it can be updated).
> 
> This change surfaced a couple new "A JNI error has occurred" situations 
> (see 8181033[5]) in the Launcher, by way of
>    test/jdk/tools/launcher/MainClassCantBeLoadedTest.java
> (using the 3-arg Class::forName, detailed in the bug report[4]),
> and
>    test/jdk/tools/launcher/modules/basic/LauncherErrors.java
> (using the 2-arg Class::forName).
> 
> The Launcher is updated to maintain non-confusing error messages :).
> 
> The new test included with this fix ensures that 8181144[3] is 
> addressed.  Thanks go to Serguei Spitsyn for writing the test.
> 
> Automated corelibs and hotspot tests pass cleanly.
> 
> Thanks,
> -Brent
> 
> -- 
> 1. https://bugs.openjdk.java.net/browse/JDK-8212117
> 
> 2. https://bugs.openjdk.java.net/browse/JDK-8222071
> 
> 3. https://bugs.openjdk.java.net/browse/JDK-8181144
> 
> 4. 
> https://bugs.openjdk.java.net/browse/JDK-8212117?focusedCommentId=14215986&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14215986 
> 
> 
> 5. https://bugs.openjdk.java.net/browse/JDK-8181033
>