Review Request: 8221530: Caller sensitive methods not handling caller = null when invoked by JNI code with no java frames on stack
mandy.chung at oracle.com
Thu Mar 28 16:25:25 UTC 2019
On 3/28/19 8:46 AM, Peter Levart wrote:
> On 3/28/19 4:08 PM, Alan Bateman wrote:
>> On 28/03/2019 14:48, Peter Levart wrote:
>>> In addition, if access from null caller is granted and it is
>>> performed to a member in a "concealed" package, there's no warning
>> The proposed check is that the package is exported unconditionally so
>> it will fail, no warning needed. I think that is okay. I could
>> imagine someone trying to argue that they run with `--add-exports
>> java.base/<concealed-package>=ALL-UNNAMED` and they expect their JNI
>> code to be able to reflect on the public members of public classes in
>> that package but it hardly seems wroth it as JNI doesn't do access
>> checks so it's pointless writing JNI code to use reflection.
> Right, and it would require deep changes to
> AccessibleObject#logIfExportedForIllegalAccess too, since it assumes
> the presence of non-null caller...
Yes it assumes the non-null caller in the current implementation. There
are several options addressing this. I would prefer to throw
IllegalCallerException if AccessibleObject::setAccessible or
trySetAccessible is called from null caller but this needs to have a
careful investigation. As Alan mentions, we should also do an audit on
all @CS methods and handles them.
My intent is to keep JDK-8221530 to fix the regression w.r.t. existing
member access check. I realized the synopsis implies a bigger scope on
other @CS methods. So I have considered other @CS methods besides
reflective member access check is a separate issue.
> Nevertheless the access checking logic could be encapsulated entirely
> in the Reflection class (for null caller too) and then in
> AccessibleObject, the logIfExportedForIllegalAccess call just skipped
> for null caller... Else the logic is scattered between two classes and
> would have to be repeated for other similar cases.
> Reflection.verifyMemberAccess() is called not only from
> AccessibleObject.slowVerifyAccess() but from elsewhere too.
As you see from the webrev, Reflection.verifyMemberAccess requires
If caller is null, it should not call Reflection.verifyMemberAccess
since the caller does not necessarily allow publicly accessible member
and NPE forces the author to think through it. If needed later, we
> For example, from ReflectUtil.ensureMemberAccess which is used in @CS
> AtomicXxxFieldUpdater(s), or from @CS java.util.ServiceLoader.load...
ServiceLoader does not support null caller.
> By encapsulating it in the common Reflection.verifyMemberAccess()
> method, all those usages get handled at the same time.
The API calling Reflection.verifyMemberAccess() should clearly decide
what behavior it wants when there is no caller. Maybe the same behavior
as Field::get or maybe not.
More information about the core-libs-dev