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

[serguei.spitsyn at oracle.com]

Hi Brent,

Some quick reply below.


On 9/5/19 5:09 PM, Brent Christian wrote:
> 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.

Yes, it is just a left over from earlier version of the test.

> Serguei, anything to add?

I'm happy this test got used and included into the webrev, thanks!

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

Will look at the webrev soon.

Thanks,
serguei

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