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

[Peter Levart]

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

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

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

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

     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.

>
> Mandy
>

Regards, Peter