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

Lois Foltan lois.foltan at oracle.com
Fri Jun 1 15:26:41 UTC 2018


On 6/1/2018 10:32 AM, Harold David Seigel wrote:

> Hi Lois,
>
> This looks good!  Can you remove the '-Xverify:all' from @run? (No new 
> webrev needed.)

Yes, thank you!  Will remove.
Lois

>
> Thanks, Harold
>
>
> On 6/1/2018 10: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
>>>>>>>
>>>>>
>>>
>>
>



More information about the hotspot-runtime-dev mailing list