Review Request JDK-8202113: Reflection API is causing caller classes to leak

[mandy chung]

On 4/28/18 6:52 PM, Peter Levart wrote:
> Hi Mandy,
>
> On 04/28/18 11:44, mandy chung wrote:
>> Webrev:
>> http://cr.openjdk.java.net/~mchung/jdk11/webrevs/8202113/webrev.00/
>>
>> The reflection machinery stores the caller class in each 
>> AccessibleObject
>> such that it can skip the access check if access to a member has been
>> verified for a given caller.  At the first time accessing the 
>> AccessibleObject,
>> it creates an Accessor object and then cache for subsequent use.  
>> This cached
>> Accessor object keeps a reference to the AccessibleObject object that
>> will keep the caller class alive.
>
> ...because the same instance of Accessor object is also set on the 
> root AccessibleObject in order to be shared among other child 
> AccessibleObject instances created from it and because this root 
> AccessibleObject is retained by the cache of AccessibleObject(s) in 
> the declaring j.l.Class...
>

Yes

>>
>> The implementation has a root object for each AccessibleObject and
>> the API returns a child object for users to access (that may suppress
>> access check via setAccessible). The caller class is set in the cache
>> of the child object.  This patch proposes to change ReflectionFactory
>> newXXXAccessor methods to ensure to pass the root object rather than
>> the child object.  The cache of the root object is always null.
>
> ...since root AccessibleObject(s) are never used for accessing the 
> member(s), they will never cache the caller Class and so they can be 
> retained by the Class-level cache together with cached Accessors that 
> now  just point back to them and never to AccessibleObject(s) handed 
> out to users that do cache caller Class.
>

Exactly.

> Right, this patch fixes the issue of retaining (leaking) the caller 
> class when using AccessibleObject(s) returned from Reflection API and 
> then throwing them away.
>
> There might be a separate issue when user-handed AccessibleObjects are 
> cached by user code and such cache is located in a different class 
> loader than classes that make invocations. This could only be solved 
> with a WeakReference in the access-check cache I suppose...
>

User code caching the AccessibleObject should be for its own use and I 
would think the cached caller would be itself.

> But this patch is good anyway, because it ensures Accessor(s) only 
> retain root AccessibleObjects which by itself is more memory-friendly.
>
> About the tests:
>
> - There seems to be some strange ununiform formatting in AccessTest.
>
> - For public and protected members, the test looks good, but for 
> private members, I don't think it is doing anything useful, because if 
> access-check fails, the caller class is not cached (from 
> AccessibleObject):
>
Right.

> private boolean slowVerifyAccess(Class<?> caller, Class<?> memberClass,
>                                      Class<?> targetClass, int modifiers)
>     {
>         if (!Reflection.verifyMemberAccess(caller, memberClass, 
> targetClass, modifiers)) {
>             // access denied
>             return false;
>         }
> ...
> ...
>         securityCheckCache = cache;         // write volatile
>         return true;
>     }
>
>
> But for private members, there's no issue of leaking caller class(es) 
> because the only successfully caller is the declaring class itself.

including the private method/field is solely for completeness (still 
partial since it does not cover the constructor).  I can add a comment 
to mention that it won't cache caller for such access failing case.

Mandy