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

[David Holmes]

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

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
>