Review Request: JDK-8206240: java.lang.Class.newInstance() is causing caller to leak

[Peter Levart]

Hi Mandy,

I don't know if this matters though, but should 
Reflection.getCallerClass() ever return null, previous behavior was to 
throw NPE (from Reflection.verifyMemberAccess(...)), now the checks are 
skipped. This should only be observable if [Class, 
Constructor].newInstance() was called from JNI's newly attached thread I 
believe. You know the inner workings of Reflection.getCallerClass() more 
than I do, so is it trust-able to always return non-null?

Regards, Peter


On 10/03/2018 05:14 PM, Mandy Chung wrote:
>
>
> On 10/3/18 6:02 AM, Alan Bateman wrote:
>>
>>
>> On 02/10/2018 20:33, Mandy Chung wrote:
>>> Class::newInstance maintains its separate cache of the caller
>>> class after access check.  This leak has been there for a long
>>> time and unnoticed.
>>>
>>> This patch changes Class::newInstance to use the code path
>>> for Constructor::newInstance doing the access check and
>>> caching the caller class.  It will also get the illegal
>>> access check in effect when Class::newInstance is called
>>> consistent with Constructor::newInstance.
>>>
>>> Webrev at:
>>> http://cr.openjdk.java.net/~mchung/jdk12/webrevs/8206240/webrev.00
>>>
>>> Alan raises a question whether the cache is still needed
>>> given that we have improved the performance of slow
>>> path of access check with Class::getPackageName.  It's a
>>> good investigation to follow up.
>> The changes look okay. One niggle is Cache.callerClassCache as 
>> nothing to do with Cache, maybe the method should be removed and the 
>> one usage changed to create the weak ref.
>>
>
> Thanks.  I have removed the callerClassCache method as you suggest.
>
>> On the benefit of the cache then I think it would be good to get some 
>> data on the benefit. Prior to the JDK 9 then I think most of the cost 
>> was checking the package but that has mostly disappeared with the 
>> changes in 9.
>
> I created https://bugs.openjdk.java.net/browse/JDK-8211452 as a follow 
> up.
>
> Mandy