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

[Calvin Cheung]

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

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
>>