RFR (S) JDK-8202758: SIGSEGV calling Class.forName(String,Boolean,ClassLoader) with mocked loader
Lois Foltan
lois.foltan at oracle.com
Fri Jun 1 14:24:57 UTC 2018
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