RFR (S) JDK-8202758: SIGSEGV calling Class.forName(String,Boolean,ClassLoader) with mocked loader

[coleen.phillimore at oracle.com]

On 5/24/18 6:45 PM, David Holmes wrote:
> Hi Coleen,
>
> My 2c
>
> On 25/05/2018 6:10 AM, coleen.phillimore at oracle.com wrote:
>>
>> http://cr.openjdk.java.net/~lfoltan/bug_jdk8202758/webrev/test/hotspot/jtreg/runtime/modules/ClassLoaderNoUnnamedModuleTest.java.html 
>>
>>
>> This doesn't seem to use InMemoryJavaCompiler but imports it.
>>
>> http://cr.openjdk.java.net/~lfoltan/bug_jdk8202758/webrev/src/hotspot/share/classfile/moduleEntry.cpp.udiff.html 
>>
>>
>> Are you sure we want to do a guarantee()?  That will look like a VM 
>> crash to the user who will report it as a bug, so they and others 
>> will still think it's our bug.  Generally when we produce an 
>> hs_err_pid.log file, people think it's a bug in the vm (except the 
>> OOM variants but sometimes that's debatable too).
>>
>> I took out all the TRAPS to the call stack for where this is called, 
>> but you could put them back in and throw an exception instead (NPE) 
>> which would give the user his file and line where the error 
>> occurred.   Or else create a variant of vm_exit_during_initialization 
>> that doesn't say it's during initialization in order to exit the vm 
>> quickly with a message.
>
> An exception would legitimise this too much. This isn't a simple 
> programming error, but a subversion of the language semantics by 
> skipping the superclass constructor.

We have constructs that subvert the language and get VerifyError or 
ClassFormatError.  I don't see the difference.

>
> A vm_exit other than at initialization is not something I'd want to 
> encourage use of. We only have the at-initialization variant to handle 
> the inability to throw exceptions at that time and to avoid ugly crashes.
>

My second choice would be to add an vm_exit variant that doesn't call 
VMError::report_and_die and doesn't say that the error happened during 
initialization.  I'd rather have the user not see an ugly crash as you 
call it.

> In this case, I think a guarantee failure is warranted - though I 
> wonder whether we can be more specific about the potential cause? I 
> agree this will come back as a bug report against the JVM rather than 
> against Mockito/Objgenesis/whomever. Not sure what we can do there.
>

I would prefer not getting the crashes.  It tooks us several minutes to 
realize that one of the bugs reported was an instance of this. The 
guarantee message is a bit useful for finding duplicates but I'd much 
rather not get the reports.

thanks,
Coleen
> Cheers,
> David
>
>> Thanks,
>> Coleen
>>
>>
>> On 5/24/18 1:35 PM, Lois Foltan wrote:
>>> Please review this change to ensure that a given ClassLoader's 
>>> unnamed Module is a valid instance of java.lang.Module with the JVM.
>>>
>>> open webrev at 
>>> http://cr.openjdk.java.net/~lfoltan/bug_jdk8202758/webrev/
>>> bug link at https://bugs.openjdk.java.net/browse/JDK-8202758
>>>
>>> Testing: hs-tier1-3, jdk-tier1-3 in progress
>>>
>>> Thanks,
>>> Lois
>>