RFR: JDK-8131600: heapdump/JMapHeap EXCEPTION_ACCESS_VIOLATION

[Kim Barrett]

On Jul 22, 2015, at 8:12 AM, Bengt Rutisson <bengt.rutisson at oracle.com> wrote:
> 
> 
> Hi everyone,
> 
> Could I have a couple of reviews for this patch to fix one of the critical nightly issues we are currently having?
> 
> http://cr.openjdk.java.net/~brutisso/8131600/webrev.00/
> https://bugs.openjdk.java.net/browse/JDK-8131600
> 
> Background
> 
> When we load a class we first create the InstanceKlass and then allocate the Java mirror on the heap. The allocation of the Java mirror is just like any other allocation, so it can be blocked by a safepoint since it will in some cases try to take the heap lock.
> 
> This means that when we hit a safepoint there can potentially exist InstanceKlasses where ik->java_mirror() == NULL.
> 
> The DumperSupport::dump_class_and_array_classes() currently does not handle that case. It blindly calls ik->signers() on the klass it is supposed to dump. signers() looks up the mirror (java_class()) and passes it on:
> 
> 
> objArrayOop InstanceKlass::signers() const {
>  // return the signers from the mirror
>  return java_lang_Class::signers(java_mirror());
> }
> 
> ...to java_lang_Class::signers(), which will crash if it gets NULL as java_class:
> 
> objArrayOop java_lang_Class::signers(oop java_class) {
>  assert(_signers_offset != 0, "must be set");
>  return (objArrayOop)java_class->obj_field(_signers_offset);
> }
> 
> The dump_class_and_array_classes() is called as part of dumping the heap from a jmap call. This is obviously done at a safepoint. So, if the jmap call comes in between the creation of the InstanceKlass and the allocation of the mirror we end up calling dump_class_and_array_classes() with an InstanceKlass that has NULL as its mirror. This is the reason for the crash in JDK-8131600.
> 
> 
> Except for DumperSupport::dump_class_and_array_classes() there is only one user of InstanceKlass::signers(). That is VM_HeapWalkOperation::iterate_over_class(). This method has an early exit if the class has not been properly initialized yet:
> 
> 
>    // ignore the class if it's has been initialized yet
>    if (!ik->is_linked()) {
>      return true;
>    }

Thanks for fixing the above comment.

> So, I think the proper fix for JDK-8131600 is to make sure that DumperSupport::dump_class_and_array_classes() has the same check.

This part looks good.

> 
> Now, DumperSupport::dump_class_and_array_classes() actually calls InstanceKlass::signers() twice. The second time around we don't have an InstanceKlass but just a Klass. The Klass does not have the is_linked() method. I haven't tried to provoke that code to fail, but I assume it has the same problem. So, I added a java_class() != NULL check to that code to make it handle this case as well.

I think this change shouldn't be needed.

The klass involved is the result of array_klass_or_null(). That
shouldn't be able to return an incompletely initialized (array) klass.