RFR: 8167333: Invalid source path info might be used when creating ClassFileStream after CFLH transforms a shared classes in some cases

[Jiangli Zhou]

Hi Ioi,

Thanks for the review!

Jiangli

> On Oct 9, 2016, at 11:27 PM, Ioi Lam <ioi.lam at oracle.com> wrote:
> 
> 
> 
> On 10/9/16 2:10 PM, David Holmes wrote:
>> On 8/10/2016 4:23 AM, Jiangli Zhou wrote:
>>> Hi David,
>>> 
>>> Thanks for taking a look.
>>> 
>>>> On Oct 6, 2016, at 10:33 PM, David Holmes <david.holmes at oracle.com
>>>> <mailto:david.holmes at oracle.com>> wrote:
>>>> 
>>>> Hi Jiangli,
>>>> 
>>>> On 7/10/2016 2:39 PM, Jiangli Zhou wrote:
>>>>> Hi,
>>>>> 
>>>>> Please review the following fix for JDK-8167333
>>>>> <https://bugs.openjdk.java.net/browse/JDK-8167333>:
>>>>> 
>>>>> webrev: http://cr.openjdk.java.net/~jiangli/8167333/webrev.00/
>>>>> <http://cr.openjdk.java.net/~jiangli/8167333/webrev.00/>
>>>>> 
>>>>> When a shared class is transformed by a JVMTI agent during initial
>>>>> loading (via CFLH), the VM creates a new ClassFileStream using the
>>>>> transformed class data. The source path info from the class’
>>>>> associated SharedClassPathEntry is passed as the ‘source’ argument to
>>>>> ClassFileStream. However, some shared classes may not have an
>>>>> associated SharedClassPathEntry and the class_path_index is -1. The
>>>>> VM needs to detect such case and not passing an invalid source path info.
>>>> 
>>>> It isn't obvious to me that all callers of
>>>> CFS::source()/clone_source() will handle getting a NULL. Of course I
>>>> can't tell which of those callers may be involved in this particular
>>>> use-case.
>>> 
>>> I took a look of all the code that calls CFS::source()/clone_source().
>>> They all handle the NULL case with explicit NULL check. For our specific
>>> case, the particular caller involved
>>> is InstanceKlass::print_loading_log. Before the fix, it crashed when
>>> trying to print the invalid cfs->source after (cfs->source() != NULL) check.
>> 
>> Thanks for verifying. I've looked at the latest webrev with the additional asserts - all looks good.
>> 
> 
> Looks good to me, too. Thanks
> 
> - Ioi
>> David
>> 
>>> Thanks,
>>> Jiangli
>>> 
>>>> 
>>>> Thanks,
>>>> David
>>>> 
>>>>> Tested with all existing class data sharing tests.
>>>>> 
>>>>> Thanks,
>>>>> Jiangli
>>>>> 
>>> 
>