RFR [9]: 8177036: Class.checkMemberAccess throws NPE when calling Class methods via JNI

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RFR [9]: 8177036: Class.checkMemberAccess throws NPE when calling Class methods via JNI

Claes Redestad
Hi,

please review this fix to avoid NPEs when calling certain Class methods
via JNI:

Bug: https://bugs.openjdk.java.net/browse/JDK-8177036
Webrev: http://cr.openjdk.java.net/~redestad/8177036/jdk.01/

Thanks!

/Claes
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR [9]: 8177036: Class.checkMemberAccess throws NPE when calling Class methods via JNI

Alan Bateman
On 20/03/2017 09:19, Claes Redestad wrote:

> Hi,
>
> please review this fix to avoid NPEs when calling certain Class
> methods via JNI:
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8177036
> Webrev: http://cr.openjdk.java.net/~redestad/8177036/jdk.01/
This looks okay. At some point then we need to see how we can test these
methods called from attached thread (with no caller).

-Alan
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR [9]: 8177036: Class.checkMemberAccess throws NPE when calling Class methods via JNI

Peter Levart


On 03/20/2017 11:41 AM, Alan Bateman wrote:

> On 20/03/2017 09:19, Claes Redestad wrote:
>
>> Hi,
>>
>> please review this fix to avoid NPEs when calling certain Class
>> methods via JNI:
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8177036
>> Webrev: http://cr.openjdk.java.net/~redestad/8177036/jdk.01/
> This looks okay. At some point then we need to see how we can test
> these methods called from attached thread (with no caller).
>
> -Alan

I guess that when only the ClassLoader of the caller is checked in logic
of @CS method, such calls should behave as though the caller was some
class loaded by bootstrap ClassLoader. But what about @CS methods that
inspect the caller class more deeply (such as reflection API) where the
identity of the caller, its package and module play role in decisions?
What behavior is expected in such cases? Should core reflection always
allow access when invoked from "null" caller?

Currently core reflection throws InternalError (because of the check in
Reflection::ensureMemberAccess). But jake repo already contains changes
that remove this check and NPE is thrown later on in verifyModuleAccess...

Regards, Peter

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR [9]: 8177036: Class.checkMemberAccess throws NPE when calling Class methods via JNI

Alan Bateman
On 20/03/2017 11:42, Peter Levart wrote:

>
> I guess that when only the ClassLoader of the caller is checked in
> logic of @CS method, such calls should behave as though the caller was
> some class loaded by bootstrap ClassLoader. But what about @CS methods
> that inspect the caller class more deeply (such as reflection API)
> where the identity of the caller, its package and module play role in
> decisions? What behavior is expected in such cases? Should core
> reflection always allow access when invoked from "null" caller?
>
> Currently core reflection throws InternalError (because of the check
> in Reflection::ensureMemberAccess). But jake repo already contains
> changes that remove this check and NPE is thrown later on in
> verifyModuleAccess...
I think we'll need to create some tests to catch all the cases where @CS
methods are involved from thread attached via JNI and without any caller
frames. In some cases then IllegalCallerException might be best, in
other cases then it may be okay to assume the caller is java.base.

-Alan.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR [9]: 8177036: Class.checkMemberAccess throws NPE when calling Class methods via JNI

Peter Levart
In reply to this post by Peter Levart


On 03/20/2017 12:42 PM, Peter Levart wrote:
> Currently core reflection throws InternalError (because of the check
> in Reflection::ensureMemberAccess). But jake repo already contains
> changes that remove this check and NPE is thrown later on in
> verifyModuleAccess...
>
> Regards, Peter

...it is actually even more erratic. If the invocation of core
reflection is performed from JNI with no caller on a freshly constructed
Member object, then the call often always succeeds (unless the member is
protected instance member accessed from subclass), because
AccessibleObject cache with no cached entry is mistakenly treated as
'null' caller.  If the Member object is 1st used from a non-null caller,
only then InternalError is thrown when such Member is later invoked from
JNI with no caller.

Regards, Peter

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR [9]: 8177036: Class.checkMemberAccess throws NPE when calling Class methods via JNI

Peter Levart


On 03/20/2017 12:54 PM, Peter Levart wrote:

>
>
> On 03/20/2017 12:42 PM, Peter Levart wrote:
>> Currently core reflection throws InternalError (because of the check
>> in Reflection::ensureMemberAccess). But jake repo already contains
>> changes that remove this check and NPE is thrown later on in
>> verifyModuleAccess...
>>
>> Regards, Peter
>
> ...it is actually even more erratic. If the invocation of core
> reflection is performed from JNI with no caller on a freshly
> constructed Member object, then the call often always succeeds (unless
> the member is protected instance member accessed from subclass),
> because AccessibleObject cache with no cached entry is mistakenly
> treated as 'null' caller.  If the Member object is 1st used from a
> non-null caller, only then InternalError is thrown when such Member is
> later invoked from JNI with no caller.
>
> Regards, Peter
>

Perhaps the best way to rectify those problems in one place would be for
Reflection.getCallerClass() to return a special internal class in its
own package, such as:

jdk.internal.solitary.NoCaller

...when there is no caller. This would work correctly for class loader
checks and would only allow invoking public exported members by core
reflection if invoked with no caller...


What do you think?


Regards, Peter

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR [9]: 8177036: Class.checkMemberAccess throws NPE when calling Class methods via JNI

Daniel Fuchs
Hi Peter,

On 20/03/2017 12:01, Peter Levart wrote:
> Perhaps the best way to rectify those problems in one place would be for
> Reflection.getCallerClass() to return a special internal class in its
> own package, such as:
>
> jdk.internal.solitary.NoCaller
>
> ...when there is no caller. This would work correctly for class loader
> checks and would only allow invoking public exported members by core
> reflection if invoked with no caller...

I believe this might be dangerous as it would probably hide bugs
in places where 'null' results in NPE being thrown in today's
implementation.

Allowing the code to succeed is not always the right thing to
do, and I don't believe it can be fixed in one place.
It's probably better to let the caller of Reflection.getCallerClass()
decide what to do when null is returned, even if this means
we might have to analyze all places where @CallerSensitive is
used.

best regards,

-- daniel

>
>
> What do you think?
>
>
> Regards, Peter
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR [9]: 8177036: Class.checkMemberAccess throws NPE when calling Class methods via JNI

Mandy Chung

> On Mar 20, 2017, at 5:15 AM, Daniel Fuchs <[hidden email]> wrote:
>
> Hi Peter,
>
> On 20/03/2017 12:01, Peter Levart wrote:
>> Perhaps the best way to rectify those problems in one place would be for
>> Reflection.getCallerClass() to return a special internal class in its
>> own package, such as:
>>
>> jdk.internal.solitary.NoCaller
>>
>> ...when there is no caller. This would work correctly for class loader
>> checks and would only allow invoking public exported members by core
>> reflection if invoked with no caller...
>
> I believe this might be dangerous as it would probably hide bugs
> in places where 'null' results in NPE being thrown in today's
> implementation.
>
> Allowing the code to succeed is not always the right thing to
> do, and I don't believe it can be fixed in one place.
> It's probably better to let the caller of Reflection.getCallerClass()
> decide what to do when null is returned, even if this means
> we might have to analyze all places where @CallerSensitive is
> used.
>

Exactly.  We ought to examine all @CS methods and determine if it handles the no caller case properly and as intended.  I file a JBS issue to track this:

https://bugs.openjdk.java.net/browse/JDK-8177155

FYI.  Several options were discussed what StackWalker::getCallerClass should return and captured in [1].  

Mandy
[1] https://bugs.openjdk.java.net/browse/JDK-8140450?focusedCommentId=13867764&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13867764

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR [9]: 8177036: Class.checkMemberAccess throws NPE when calling Class methods via JNI

Mandy Chung
In reply to this post by Claes Redestad

> On Mar 20, 2017, at 2:19 AM, Claes Redestad <[hidden email]> wrote:
>
> Hi,
>
> please review this fix to avoid NPEs when calling certain Class methods via JNI:
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8177036
> Webrev: http://cr.openjdk.java.net/~redestad/8177036/jdk.01/


Looks okay.  I agree that at some point we should look at @CS methods and if the no caller case is properly handled.  I filed a JBS issue for it:
  https://bugs.openjdk.java.net/browse/JDK-8177155

Mandy
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR [9]: 8177036: Class.checkMemberAccess throws NPE when calling Class methods via JNI

David Holmes
In reply to this post by Mandy Chung
As I'm discussing in the RFR for

"8177136: Caller sensitive methods Logger.getLogger,
Logger.getAnonymousLogger, and System.getLogger should throw
IllegalCallerException if there is no caller on the stack."

I am quite concerned by this, seemingly sudden, problem that we have a
whole bunch of methods that now rely on there being a Java caller on the
stack and as a result if called from JNI they fail! It's fine for the
internal methods that depend on a caller to throw exceptions, but the
public APIs should have defined semantics about how they depend on any
kind of "caller context" and not just suddenly make the call illegal by
throwing an IllegalCallerException as in 8177136!

Cheers,
David

On 21/03/2017 4:06 AM, Mandy Chung wrote:

>
>> On Mar 20, 2017, at 5:15 AM, Daniel Fuchs <[hidden email]> wrote:
>>
>> Hi Peter,
>>
>> On 20/03/2017 12:01, Peter Levart wrote:
>>> Perhaps the best way to rectify those problems in one place would be for
>>> Reflection.getCallerClass() to return a special internal class in its
>>> own package, such as:
>>>
>>> jdk.internal.solitary.NoCaller
>>>
>>> ...when there is no caller. This would work correctly for class loader
>>> checks and would only allow invoking public exported members by core
>>> reflection if invoked with no caller...
>>
>> I believe this might be dangerous as it would probably hide bugs
>> in places where 'null' results in NPE being thrown in today's
>> implementation.
>>
>> Allowing the code to succeed is not always the right thing to
>> do, and I don't believe it can be fixed in one place.
>> It's probably better to let the caller of Reflection.getCallerClass()
>> decide what to do when null is returned, even if this means
>> we might have to analyze all places where @CallerSensitive is
>> used.
>>
>
> Exactly.  We ought to examine all @CS methods and determine if it handles the no caller case properly and as intended.  I file a JBS issue to track this:
>
> https://bugs.openjdk.java.net/browse/JDK-8177155
>
> FYI.  Several options were discussed what StackWalker::getCallerClass should return and captured in [1].
>
> Mandy
> [1] https://bugs.openjdk.java.net/browse/JDK-8140450?focusedCommentId=13867764&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13867764
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR [9]: 8177036: Class.checkMemberAccess throws NPE when calling Class methods via JNI

Mandy Chung
I saw the thread on JDK-8177136.  I share your concern that throwing ICE does not seem a good choice.  I’ll reply on that thread.  It seems to me that it should default to the unnamed module if no caller frame rather than throwing an exception.

For 8177036, no change in behavior.  This actually fixes a regression that throws NPE that I didn’t catch during the review.

Mandy

> On Mar 20, 2017, at 1:29 PM, David Holmes <[hidden email]> wrote:
>
> As I'm discussing in the RFR for
>
> "8177136: Caller sensitive methods Logger.getLogger, Logger.getAnonymousLogger, and System.getLogger should throw IllegalCallerException if there is no caller on the stack."
>
> I am quite concerned by this, seemingly sudden, problem that we have a whole bunch of methods that now rely on there being a Java caller on the stack and as a result if called from JNI they fail! It's fine for the internal methods that depend on a caller to throw exceptions, but the public APIs should have defined semantics about how they depend on any kind of "caller context" and not just suddenly make the call illegal by throwing an IllegalCallerException as in 8177136!
>
> Cheers,
> David
>
> On 21/03/2017 4:06 AM, Mandy Chung wrote:
>>
>>> On Mar 20, 2017, at 5:15 AM, Daniel Fuchs <[hidden email]> wrote:
>>>
>>> Hi Peter,
>>>
>>> On 20/03/2017 12:01, Peter Levart wrote:
>>>> Perhaps the best way to rectify those problems in one place would be for
>>>> Reflection.getCallerClass() to return a special internal class in its
>>>> own package, such as:
>>>>
>>>> jdk.internal.solitary.NoCaller
>>>>
>>>> ...when there is no caller. This would work correctly for class loader
>>>> checks and would only allow invoking public exported members by core
>>>> reflection if invoked with no caller...
>>>
>>> I believe this might be dangerous as it would probably hide bugs
>>> in places where 'null' results in NPE being thrown in today's
>>> implementation.
>>>
>>> Allowing the code to succeed is not always the right thing to
>>> do, and I don't believe it can be fixed in one place.
>>> It's probably better to let the caller of Reflection.getCallerClass()
>>> decide what to do when null is returned, even if this means
>>> we might have to analyze all places where @CallerSensitive is
>>> used.
>>>
>>
>> Exactly.  We ought to examine all @CS methods and determine if it handles the no caller case properly and as intended.  I file a JBS issue to track this:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8177155
>>
>> FYI.  Several options were discussed what StackWalker::getCallerClass should return and captured in [1].
>>
>> Mandy
>> [1] https://bugs.openjdk.java.net/browse/JDK-8140450?focusedCommentId=13867764&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13867764
>>

Loading...