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

[Brent Christian]

Hi, David

On 9/5/19 12:52 AM, David Holmes wrote:
> 
> Good to see this all come together :)

:)

> 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.

I'm good with adding an assert to check for coherent arguments.

(Another interpretation is that if 'init' is set, then the 'link' 
argument is ignored, since linking is implied).

> 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!

OK, thanks.  I've noted this in JDK-8226540.

> 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

(Discussed in the CSR)

> src/java.base/share/classes/sun/launcher/LauncherHelper.java
> ... > 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. ??

Also discussed in the CSR[1].

> 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. ??

I'm guessing it's just a leftover from an earlier version of the test. 
I've removed the comment, and updated the test @summary.

Serguei, anything to add?

>> 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 for taking a look.

Updated webrev at:
http://cr.openjdk.java.net/~bchristi/8212117/webrev10/

-Brent

1. 
https://bugs.openjdk.java.net/browse/JDK-8222071?focusedCommentId=14288303&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14288303

>>
>> 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
>>