Review Request: 8221530: Caller sensitive methods not handling caller = null when invoked by JNI code with no java frames on stack

[Mandy Chung]

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 
>>> displayed
>> 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 
Objects.requiresNonNull(currentClass) and 
Objects.requiresNonNull(memberClass).

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.

Mandy