Re: Review Request JDK-8202113: Reflection API is causing caller classes to leak
Hi Peter, Sorry for the late reply. I was on vacation last week and just returned. On 5/14/18 8:43 AM, Peter Levart wrote: Hi Mandy, Sorry for noticing this too late, but... If it was not necessary to keep legacy hacky behavior (to honor the patched "modifiers" field), wouldn't it be cleaner to leave the ReflectionFactory as is, but modify the following private methods instead: - Field#acquireFieldAccessor - Method#acquireMethodAccessor - Constructor#acquireConstructorAccessor They already deal with 'root' object and could pass it to ReflectionFactory. The logic of ReflectionFactory need not deal with the notion of 'root' object then and no LangReflectAccess additions are necessary. You would keep the notion of root objects encapsulated. With tour patch it has leaked into ReflectionFactory too. I started with that approach but I wanted to assert that the root object is used to catch any future regression of the memory leak. There is other option doing that for example adding LangReflectAccess::isRoot or ensureRoot instead of getRoot method. I see all these are part of the reflection implementation. We could revisit this code when we touch this area in the future. Is it really that important to allow users to modify static final fields that way? As such fields are normally constant folded by JIT, I doubt that anybody is doing it nowadays. Doing it is bound to unpredictable program behavior, as JVM is free to never reload such field's value. Sadly, there are existing code using this hack. As Alan said, we will find out more once the illegal access is denied by default. I prefer to separate this breakage from this issue and disable the hack to change static final field via reflection completely in another day. Mandy
Re: Review Request JDK-8202113: Reflection API is causing caller classes to leak
On 14/05/2018 16:43, Peter Levart wrote: : Is it really that important to allow users to modify static final fields that way? As such fields are normally constant folded by JIT, I doubt that anybody is doing it nowadays. Doing it is bound to unpredictable program behavior, as JVM is free to never reload such field's value. Sadly, there are still a number of libraries using this hack to change static final fields. They should be seeing an "Illegal reflective ..." warning today and the hack will break once the encapsulation is dialed up. The torch and pitchfork crowd will be out when that happens and I read Mandy's approach as just leaving the issue to that day. -Alan
Re: Review Request JDK-8202113: Reflection API is causing caller classes to leak
On 05/11/2018 06:09 PM, mandy chung wrote: On 4/30/18 10:21 AM, Alan Bateman wrote: The updated webrev looks good. A minor comment is that I assume you can remove the cast from Executable::declaredAnnotations if you leave Executable::getRoot in place. It could but leave it as is. I found that this change breaks the hack that uses reflection to change a static final field by changing the private modifiers field in the Field object. That is a terrible hack but I think it's better to separate this incompatibility from this issue. I modified the fix to change the modifiers of the root and child field object be the same. http://cr.openjdk.java.net/~mchung/jdk11/webrevs/8202113/webrev.02/ Mandy Hi Mandy, Sorry for noticing this too late, but... If it was not necessary to keep legacy hacky behavior (to honor the patched "modifiers" field), wouldn't it be cleaner to leave the ReflectionFactory as is, but modify the following private methods instead: - Field#acquireFieldAccessor - Method#acquireMethodAccessor - Constructor#acquireConstructorAccessor They already deal with 'root' object and could pass it to ReflectionFactory. The logic of ReflectionFactory need not deal with the notion of 'root' object then and no LangReflectAccess additions are necessary. You would keep the notion of root objects encapsulated. With tour patch it has leaked into ReflectionFactory too. Is it really that important to allow users to modify static final fields that way? As such fields are normally constant folded by JIT, I doubt that anybody is doing it nowadays. Doing it is bound to unpredictable program behavior, as JVM is free to never reload such field's value. Regards, Peter
Re: Review Request JDK-8202113: Reflection API is causing caller classes to leak
On 11/05/2018 17:09, mandy chung wrote: It could but leave it as is. I found that this change breaks the hack that uses reflection to change a static final field by changing the private modifiers field in the Field object. That is a terrible hack but I think it's better to separate this incompatibility from this issue. I modified the fix to change the modifiers of the root and child field object be the same. http://cr.openjdk.java.net/~mchung/jdk11/webrevs/8202113/webrev.02/ Code using this hack should be seeing "Illegal reflective access" warnings today and it will eventually break. However, in the mean-time, I think what you have looks good. -Alan
Re: Review Request JDK-8202113: Reflection API is causing caller classes to leak
On 4/30/18 10:21 AM, Alan Bateman wrote: The updated webrev looks good. A minor comment is that I assume you can remove the cast from Executable::declaredAnnotations if you leave Executable::getRoot in place. It could but leave it as is. I found that this change breaks the hack that uses reflection to change a static final field by changing the private modifiers field in the Field object. That is a terrible hack but I think it's better to separate this incompatibility from this issue. I modified the fix to change the modifiers of the root and child field object be the same. http://cr.openjdk.java.net/~mchung/jdk11/webrevs/8202113/webrev.02/ Mandy
Re: Review Request JDK-8202113: Reflection API is causing caller classes to leak
On 30/04/2018 17:29, mandy chung wrote: : The 3 x getRoot methods on ReflectAccess looks okay. An alternative would to create T getRoot(T obj) and a package private getRoot() method on AccessibleObject. Good idea. I updated the patch. The updated webrev looks good. A minor comment is that I assume you can remove the cast from Executable::declaredAnnotations if you leave Executable::getRoot in place. -Alan
Re: Review Request JDK-8202113: Reflection API is causing caller classes to leak
On 4/30/18 7:39 PM, Alan Bateman wrote: The approach looks good, seems like this one was lurking (for protected members at least) for a long time. Yes and this issue becomes more noticeable in JDK 9 as public members needs additional module access check. The 3 x getRoot methods on ReflectAccess looks okay. An alternative would to create T getRoot(T obj) and a package private getRoot() method on AccessibleObject. Good idea. I updated the patch. You might want to check AccessTest before pushing. The webrev shows very odd alignment, maybe tabs expanded to 8 space indent although it's not consistently so. Thanks for catching it. I reformatted it. I also included a comment in AccessTest to mention that private member is not accessible and caller class is not cached in that case. Updated webrev: http://cr.openjdk.java.net/~mchung/jdk11/webrevs/8202113/webrev.01 thanks Mandy
Re: Review Request JDK-8202113: Reflection API is causing caller classes to leak
On 28/04/2018 10: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. 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. The approach looks good, seems like this one was lurking (for protected members at least) for a long time. The 3 x getRoot methods on ReflectAccess looks okay. An alternative would to create T getRoot(T obj) and a package private getRoot() method on AccessibleObject. You might want to check AccessTest before pushing. The webrev shows very odd alignment, maybe tabs expanded to 8 space indent although it's not consistently so. -Alan
Re: Review Request JDK-8202113: Reflection API is causing caller classes to leak
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
Re: Review Request JDK-8202113: Reflection API is causing caller classes to leak
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
