RFR (S) JDK-8202758: SIGSEGV calling Class.forName(String,Boolean,ClassLoader) with mocked loader
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Fri May 25 02:16:10 UTC 2018
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:
>>>> This doesn't seem to use InMemoryJavaCompiler but imports it.
>>>> 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
> 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 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
>> 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
+ guarantee(java_lang_Module::is_instance(module), err_msg("ClassLoader
%s has not been initialized correctly: the unnamed module is not set"
Might not need err_msg.
>>>> 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
>>>>> bug link at https://bugs.openjdk.java.net/browse/JDK-8202758
>>>>> Testing: hs-tier1-3, jdk-tier1-3 in progress
More information about the hotspot-runtime-dev