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