RFR(S): 8200466: Revisit the setting of _transitive_interfaces in InstanceKlass

Calvin Cheung calvin.cheung at oracle.com
Mon Apr 30 20:58:59 UTC 2018


Hi Coleen,

Thanks for your review.

On 4/30/18, 10:23 AM, coleen.phillimore at oracle.com wrote:
>
> This sharing of metadata has been prone to bugs.  Can you have the 
> same bug with the _secondary_super field? 
> InstanceKlass::deallocate_contents() checks:
>
>       secondary_supers() != transitive_interfaces() &&
>
> and will free the field possibly from the super class if 
> transitive_interfaces() is still NULL.  (If I understand this correctly).
If it passes the conditions check in the 'if' block, it will deallocate 
the _secondary_supers.
It maybe fine because the crash was in visit_all_interfaces() where it 
doesn't dereference the _secondary_supers.
>
> Wouldn't it be easier just to eagerly set the super class, and keep 
> the call to initialize_supers() where it is?
Do you mean adding the following in 
ClassFileParser::apply_parsed_class_metadata() and reverting the rest of 
the changes?

     this_klass->set_super(const_cast<Klass*>(_super_klass));

BTW, I think the first arg to initialize_supers() should be casted to 
Klass* like above because the _super_klass is already an InstanceKlass*.

ik->initialize_supers(const_cast<InstanceKlass*>(_super_klass) ...


>
> http://cr.openjdk.java.net/~ccheung/8200466/webrev.00/src/hotspot/share/classfile/classFileParser.cpp.udiff.html 
>
>
> **** 5884,5893 *****
> *--- 5889,5899 ----*
> *Annotations::free_contents(_loader_data, _fields_annotations);*
> *Annotations::free_contents(_loader_data, _fields_type_annotations);*
> *}*
> **
> *clear_class_metadata();*
> *+ _transitive_interfaces = NULL;*
> **
>
> clear_class_metadata() already sets the _transitive_interfaces to 
> NULL, so this shouldn't be needed.
The current change does not set the _trasitive_interfaces to NULL in 
clear_class_metadata().

 From the udiff above:

**** 5832,5842 *****
*--- 5838,5847 ----*
*     _cp = NULL;*
*     _fields = NULL;*
*     _methods = NULL;*
*     _inner_classes = NULL;*
*     _local_interfaces = NULL;*
*     _transitive_interfaces = NULL;*
*     _combined_annotations = NULL;*
*     _annotations = _type_annotations = NULL;*
*     _fields_annotations = _fields_type_annotations = NULL;*
*   }*

thanks,
Calvin
>
> thanks,
> Coleen
>
> On 4/27/18 12:46 AM, Calvin Cheung wrote:
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8200466
>>
>> webrev: http://cr.openjdk.java.net/~ccheung/8200466/webrev.00/
>>
>> This bug was discovered during the investigation of JDK-8200078 
>> <https://bugs.openjdk.java.net/browse/JDK-8200078>.
>>
>> I've included the following comment in classFileParser.cpp to 
>> summarize the change:
>>
>> // Delay the setting of _transitive_interfaces until after 
>> initialize_supers() in
>> // fill_instance_klass(). It is because the _transitive_interfaces 
>> may be shared with
>> // its _super. If an OOM occurs while loading the current klass, its 
>> _super field
>> // may not have been set. When GC tries to free the klass, the 
>> _transitive_interfaces
>> // may be deallocated mistakenly in 
>> InstanceKlass::deallocate_interfaces(). Subsequent
>> // dereferences to the deallocated _transitive_interfaces will result 
>> in a crash.
>>
>> Testing: (on Oracle platforms)
>>     hs-tier{1,2,3}
>>     closed (soon will be open) PCL (parallel class loading) tests
>>
>> thanks,
>> Calvin
>


More information about the hotspot-runtime-dev mailing list