RFR: JDK-8147026- Convert an assert in ClassLoaderData to a guarantee

[Ioi Lam]

Hi Shafi,

I read the bug report, and it says, " the JVM was crashing during GC 
because the object it was looking at had a pointer to a bad klass. The 
core file analysis showed that the classloader object in the klass was 
not a valid Oop". Are you sure that ClassLoaderDataGraph::find_or_create 
was indeed called with an invalid loader? There are many reason for this 
to happen. E.g., there could have been strayed memory writes that 
corrupts the pointer to the classloader object.

In fact, if you have a invalid loader here, the call will return junk

inline ClassLoaderData *ClassLoaderDataGraph::find_or_create(Handle 
loader, TRAPS) {
   guarantee(loader() != NULL && loader()->is_oop(), "Loader must be oop");
   // Gets the class loader data out of the java/lang/ClassLoader 
object, if non-null
   // it's already in the loader_data, so no need to add
   ClassLoaderData* loader_data= 
java_lang_ClassLoader::loader_data(loader());
   if (loader_data) {
      return loader_data;  <<<<<<  junk
   }

The callers of this code (systemDicrionary.cpp) will deference the 
returned value to do various operations related to class loading, and 
will crash quickly with a bad pointer. It's highly unlikely that your 
original test scenario would have been healthy enough to get to the next GC.

So, I don't think changing the assert to guarantee will significantly 
affect how the product JVM would behave with bad input to find_or_create.

- Ioi

On 2/16/16 1:51 AM, David Holmes wrote:
> Hi Shafi,
>
> On 16/02/2016 7:41 PM, Shafi Ahmad wrote:
>> Hi All,
>>
>>
>>
>> Please review the trivial change of replacing 'assert' by 'guarantee'
>>
>>
>>
>> Summary: Having a guarantee in method 
>> "ClassLoaderDataGraph::find_or_create" ensuring that the classloader 
>> is a valid oop will help catch the errors at an early stage during 
>> the run rather than crashing the JVM later on in the GC.
>
> I'm not sure I agree with this change on principle. The assert says 
> "we never expect this to fail and we expect to weed out any problems 
> during testing". I could see the need to expand the assert to also 
> check for it being an oop though.
>
> I'll defer approval of this to Coleen and Ioi as they have the 
> expertise in this code area.
>
> Thanks,
> David
>
>>
>>
>> Open webrev: http://cr.openjdk.java.net/~csahu/8147026/webrev.00
>>
>> Bug link: https://bugs.openjdk.java.net/browse/JDK-8147026
>>
>> Testing:  JPRT
>>
>>
>>
>> Since we are changing 'assert' by 'guarantee' which will add few 
>> instruction into code in product build and I am not sure about the 
>> *hotness* of this method.
>>
>> So I run the performance test [refworkload] to make sure the impact 
>> of this change in the product build.
>>
>> I am attaching the  performance run report for your reference.
>>
>>
>>
>> Regards,
>>
>> Shafi
>>
>>
>>