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

[David Holmes]

Looks good.

Thanks,
David

On 2/06/2018 12:24 AM, Lois Foltan wrote:
> Thank you David, Coleen and Alan for your reviews.  Based on the 
> discussion I have a new webrev that hopefully addresses the review comments.
> 
> http://cr.openjdk.java.net/~lfoltan/bug_jdk8202758.1/webrev/
> 
> Retesting hs-tier1-3, jdk-tier1-3 in progress.
> 
> Thanks,
> Lois
> 
> On 5/24/2018 10:16 PM, coleen.phillimore at oracle.com wrote:
>>
>>
>> On 5/24/18 8:27 PM, David Holmes wrote:
>>> On 25/05/2018 9:17 AM, coleen.phillimore at oracle.com wrote:
>>>> 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.
>>>
>>> The difference is that there is a clear specification for correctly 
>>> formed classfiles that we can check against and throw the appropriate 
>>> exception.
>>>
>>> That is not what is happening here. Unfortunately there's nothing in 
>>> the VM spec that requires the call chain of superclass constructors 
>>> that the Java language requires - so we can't detect this kind of 
>>> problem that way. As Alan wrote in the bug report there are numerous 
>>> system classes that are intimately tied to the VM and which rely on 
>>> implicit protocols between the VM and the library code. Here the 
>>> framework has replaced a class and caused it to not invoke its 
>>> superclass constructor - as the Java language requires - and as a 
>>> result initialization that the VM depends upon is not performed. 
>>> There are numerous ways that similar crashes could be provoked.
>>>
>>>>>
>>>>> 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.
>>>
>>> I totally oppose any notion that there can be, after initialization, 
>>> any abnormal termination of the VM other than a crash! If you've 
>>> subverted bytecode rules or API semantics then you get exceptions. If 
>>> you load native code, or an agent that stomps on memory, you get a 
>>> crash. If you inject any other foreign "code" into the VM and it 
>>> triggers an internal error then you get an internal error - which is 
>>> abrupt termination with a hs_err log (aka a crash).
>>>
>>> We've had similar cases where attempts have been made to replace 
>>> java.lang.Object for example.
>>
>> In those examples, we call vm_exit_on_initialization though.  See code 
>> in javaClasses.cpp.
>>>
>>>>> 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.
>>>
>>> You think people won't report a sudden termination of the JVM any 
>>> way?? The end user won't understand this error regardless of how we 
>>> report it.
>>>
>>
>> I'd rather not see an hs_err file and bug report, but I appear to be 
>> the minority.   Lois, can you add a comment to the guarantee so that 
>> someone doesn't change it to an assert (or remove it).
>>
>> + // Ensure that the unnamed module was correctly set when
>> + // the class loader was constructed.   Guarantee will cause a recognizable crash if the
>>     // user code has circumvented calling the ClassLoader constructor (or something like that) and that's his own
>>     // problem.
>>   + guarantee(java_lang_Module::is_instance(module), 
>> err_msg("ClassLoader %s has not been initialized correctly: the 
>> unnamed module is not set" class_loader->loader_name()));
>>
>> Might not need err_msg.
>>
>> Thanks,
>> Coleen
>>
>>> Thanks,
>>> David
>>>
>>>> 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
>>>>>>
>>>>
>>
>