RFR(S) 8226645: [TESTBUG] some AppCDS tests relies on illegal reflective access

[David Holmes]

Hi Calvin,

On 10/08/2019 1:50 pm, Calvin Cheung wrote:
> Hi Ioi,
> 
> Thanks for your review!
> 
> On 8/9/19 1:07 PM, Ioi Lam wrote:
>> Hi Calvin,
>>
>> This makes it hard to tell where the privateLookupIn is from.
>>
>>     import static java.lang.invoke.MethodHandles.*;
>>     ....
>>     Lookup lookup = privateLookupIn(target, lookup());
>>
>> I think it's better to do this:
>>
>>     import static java.lang.invoke.MethodHandles;
>>     ....
>>     Lookup lookup = MethodHandles.privateLookupIn(target, 
>> MethodHandles.lookup());
> Fixed.
>>
>>
>> Also, instead of forcing every test case to carry a class called 
>> "Hello", I think
>> it's better to do this:
>>
>>
>>     /**
>>      * Define the class as stored in clsFile, in the same loader as 
>> peerClass,
>>      * with the following modification:
>>      * <ul>....</ul>
>>      */
>>     public static Class<?> defineModifiedClass(Class<?> peerClass, 
>> File clsFile,
>>                            String fromString, String toString)
>>         throws .....
>>     {
>>         ....
>>         Lookup lookup = MethodHandles.privateLookupIn(peerClass, 
>> MethodHandles.lookup());

I don't understand how this works. MethodHandles.lookup() returns a 
Lookup for the current/caller class, which in this case is the Utils 
test library class. You then use that Lookup instance to try and get a 
private Lookup for peerClass, which is an arbitrary test class. But the 
Utils Lookup should not have permissions to get a private Lookup for an 
arbitrary class! Unless there is some kind of "un-named module" general 
access being granted here ??

Thanks,
David

>>         Class<?> cls = lookup.defineClass(buff);
>>         ....
> This works with defineModifiedClass but not with the defineClassFromJAR. 
> It is because the current usage of defineClassFromJAR is that the class 
> is supposed to be loaded by a URLClassLoader. If we just pass the test 
> class as the peerClass, the loader will be AppClassLoader. Therefore, I 
> need to keep the Hello class for the LoaderSegregationTest but the 
> loading of the Hello class will be in the LoaderSegregation app class 
> instead of in Util.java. I also modified defineClassFromJAR so that its 
> signature is the same as defineModifiedClass.
>>
>> Then the caller of defineModifiedClass can be changed from:
>>
>> Class superClass = 
>> Util.defineModifiedClass(RewriteBytecodes.class.getClassLoader(), 
>> clsFile, from, to);
>>
>> to
>>
>> Class superClass = Util.defineModifiedClass(RewriteBytecodes.class, 
>> clsFile, from, to);
> 
> Fixed.
> 
> updated webrev:
> 
>      http://cr.openjdk.java.net/~ccheung/8226645/webrev.01/
> 
> thanks,
> 
> Calvin
> 
>>
>> Thanks
>> - Ioi
>>
>>
>>
>> On 8/9/19 11:19 AM, Calvin Cheung wrote:
>>> bug: https://bugs.openjdk.java.net/browse/JDK-8226645
>>>
>>> webrev: http://cr.openjdk.java.net/~ccheung/8226645/webrev.00/
>>>
>>> Updating couple of AppCDS tests to use Lookup.defineClass instead of 
>>> ClassLoader.defineClass. A dummy class will be loaded by the 
>>> requested class loader and then a lookup instance is obtained using 
>>> the privateLookupIn API.
>>>
>>> Testing: mach5 tiers1 - 3.
>>>
>>>              locally on linux-x64 with -vmoption:--illegal-access=deny
>>>
>>> thanks,
>>>
>>> Calvin
>>>
>>