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

[David Holmes]

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

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