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

[David Holmes]

On 13/08/2019 5:01 pm, Calvin Cheung wrote:
> 
> On 8/12/19 6:53 PM, David Holmes wrote:
>> Hi Calvin,
>>
>> On 13/08/2019 4:11 am, Calvin Cheung wrote:
>>> On 8/11/19 11:42 PM, Alan Bateman wrote:
>>>> On 12/08/2019 06:19, David Holmes wrote:
>>>>>
>>>>> 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 ??
>>>> It will only work id peerClass's module opens the packages to all 
>>>> unnamed modules. A saner approach might be for the Util method to 
>>>> take a Lookup object with the right access so that it defines the 
>>>> class in the same run-time package as the lookup object.
>>> For the tests in questions, both callerModule and targetModule are 
>>> unnamed and both callerClass and lookupClass don't belong to a 
>>> package. So I think the Lookup object was obtained correctly. Let me 
>>> know if there's a better way of obtaining a Lookup object.
>>
>> You're not supposed to "obtain" a Lookup for another class as such. A 
>> given class is supposed to obtain its own Lookup and then hand that 
>> out to trusted parties. So as Alan suggested you would have:
>>
>> public static Class<?> defineModifiedClass(Lookup lookup, File 
>> clsFile, String fromString, String toString) ... {
>>   ...
>>   Class<?> cls = lookup.defineClass(buff);
>> }
>>
>> and the test would do:
>>
>> Class superClass = Util.defineModifiedClass(MethodHandles.lookup(), 
>> clsFile, from, to);
> 
> I actually tried this initially and it only works if the expected class 
> loader for loading the class is AppClassLoader. There exists a test case 
> which uses the privateLookupIn API for the URLClassLoader case.
> 
> See:
> 
> http://hg.openjdk.java.net/jdk/jdk/file/75375b6617e6/test/jdk/java/lang/invoke/DefineClassTest.java#l182 

Sorry I don't quite see the connection between that test and the use of 
Utils.defineModifiedClass and the use of lookup(). ?? The type of 
classloader should not be relevant.

David
-----

> 
> thanks,
> 
> Calvin
> 
>>
>> Cheers,
>> David
>>
>>>> In addition, tests such as LoaderSegregation create the ClassLoader 
>>>> so they can define the class (C2 in this case) directly, no need for 
>>>> a dummy class.
>>>>
>>> For this test to work, the lookupClass' loader needs to be a 
>>> java.net.URLClassLoader. Only this test requires a dummy class and 
>>> the current fix is fairly simple. I'd like to leave the fix as is for 
>>> now.
>>>
>>> thanks,
>>>
>>> Calvin
>>>