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

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Daniel Fuchs
Hi,

Please find below a patch for:

8177136: Caller sensitive methods Logger.getLogger,
Logger.getAnonymousLogger, and System.getLogger should throw
IllegalCallerException if there is no caller on the stack.
https://bugs.openjdk.java.net/browse/JDK-8177136

Caller sensitive methods Logger.getLogger, Logger.getAnonymousLogger,
and System.getLogger currently throw an undocumented
NullPointerException if they are called from JNI and there is no
Java frame on the stack.

Throwing NullPointerException is confusing and makes it look like there
is a bug in the implementation.
In truth, these method are @CallerSensitive, and therefore must not
be called in a context where the caller cannot be determined.
Therefore, the right thing to do is to throw IllegalCallerException
and document this.

webrev:
http://cr.openjdk.java.net/~dfuchs/webrev_8177136/webrev.00/

As per Rampdown Phase 2 Process [1] I'd also like to get
confirmation that this is a reasonable proposal to fix in 9.
This fix just transmutes a NullPointerException (which should
never happen at this point in regular usage of the API) into an
IllegalCallerException which will help diagnosing the fact
that the API is called from a context where it's not supposed
to be used. The risk of fixing should therefore be very limited.

best regards,

-- daniel

[1] Rampdown Phase 2 Process
http://mail.openjdk.java.net/pipermail/jdk9-dev/2017-March/005666.html
Reply | Threaded
Open this post in threaded view
|

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

David Holmes
Sorry Daniel but I don't understand how calling a public method via JNI
can be deemed an illegal call ??? This seems to be a hole in the notion
of "caller sensitive" to me.

Also see discussion re: "RFR [9]: 8177036: Class.checkMemberAccess
throws NPE when calling Class methods via JNI"

David
-----

On 20/03/2017 10:08 PM, Daniel Fuchs wrote:

> Hi,
>
> Please find below a patch for:
>
> 8177136: Caller sensitive methods Logger.getLogger,
> Logger.getAnonymousLogger, and System.getLogger should throw
> IllegalCallerException if there is no caller on the stack.
> https://bugs.openjdk.java.net/browse/JDK-8177136
>
> Caller sensitive methods Logger.getLogger, Logger.getAnonymousLogger,
> and System.getLogger currently throw an undocumented
> NullPointerException if they are called from JNI and there is no
> Java frame on the stack.
>
> Throwing NullPointerException is confusing and makes it look like there
> is a bug in the implementation.
> In truth, these method are @CallerSensitive, and therefore must not
> be called in a context where the caller cannot be determined.
> Therefore, the right thing to do is to throw IllegalCallerException
> and document this.
>
> webrev:
> http://cr.openjdk.java.net/~dfuchs/webrev_8177136/webrev.00/
>
> As per Rampdown Phase 2 Process [1] I'd also like to get
> confirmation that this is a reasonable proposal to fix in 9.
> This fix just transmutes a NullPointerException (which should
> never happen at this point in regular usage of the API) into an
> IllegalCallerException which will help diagnosing the fact
> that the API is called from a context where it's not supposed
> to be used. The risk of fixing should therefore be very limited.
>
> best regards,
>
> -- daniel
>
> [1] Rampdown Phase 2 Process
> http://mail.openjdk.java.net/pipermail/jdk9-dev/2017-March/005666.html
Reply | Threaded
Open this post in threaded view
|

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

Daniel Fuchs
Hi David,

On 20/03/2017 12:16, David Holmes wrote:
> Sorry Daniel but I don't understand how calling a public method via JNI
> can be deemed an illegal call ???

In the case of these methods, it's only illegal if there's no
java frame on the stack as the caller's module cannot be determined.
It's perfectly OK to call them from JNI has long as there's
some Java frame (main() or Thread::run) higher up.

> This seems to be a hole in the notion
> of "caller sensitive" to me.

I'd rather say it's a hole in how such a situation is handled.

> Also see discussion re: "RFR [9]: 8177036: Class.checkMemberAccess
> throws NPE when calling Class methods via JNI"

Yes - I am aware of this and this is what triggered my
proposed patch for the few places where we use @CallerSensitive
in logging.

An application that would need can to call these logging methods
from such a context (where no Java frame is on the stack) can
always insert its own wrapper (e.g. a call these methods from an
application class and call that application class from JNI instead).

best regards,

-- daniel

>
> David
> -----
>
> On 20/03/2017 10:08 PM, Daniel Fuchs wrote:
>> Hi,
>>
>> Please find below a patch for:
>>
>> 8177136: Caller sensitive methods Logger.getLogger,
>> Logger.getAnonymousLogger, and System.getLogger should throw
>> IllegalCallerException if there is no caller on the stack.
>> https://bugs.openjdk.java.net/browse/JDK-8177136
>>
>> Caller sensitive methods Logger.getLogger, Logger.getAnonymousLogger,
>> and System.getLogger currently throw an undocumented
>> NullPointerException if they are called from JNI and there is no
>> Java frame on the stack.
>>
>> Throwing NullPointerException is confusing and makes it look like there
>> is a bug in the implementation.
>> In truth, these method are @CallerSensitive, and therefore must not
>> be called in a context where the caller cannot be determined.
>> Therefore, the right thing to do is to throw IllegalCallerException
>> and document this.
>>
>> webrev:
>> http://cr.openjdk.java.net/~dfuchs/webrev_8177136/webrev.00/
>>
>> As per Rampdown Phase 2 Process [1] I'd also like to get
>> confirmation that this is a reasonable proposal to fix in 9.
>> This fix just transmutes a NullPointerException (which should
>> never happen at this point in regular usage of the API) into an
>> IllegalCallerException which will help diagnosing the fact
>> that the API is called from a context where it's not supposed
>> to be used. The risk of fixing should therefore be very limited.
>>
>> best regards,
>>
>> -- daniel
>>
>> [1] Rampdown Phase 2 Process
>> http://mail.openjdk.java.net/pipermail/jdk9-dev/2017-March/005666.html

Reply | Threaded
Open this post in threaded view
|

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

David Holmes
On 20/03/2017 10:29 PM, Daniel Fuchs wrote:

> Hi David,
>
> On 20/03/2017 12:16, David Holmes wrote:
>> Sorry Daniel but I don't understand how calling a public method via JNI
>> can be deemed an illegal call ???
>
> In the case of these methods, it's only illegal if there's no
> java frame on the stack as the caller's module cannot be determined.
> It's perfectly OK to call them from JNI has long as there's
> some Java frame (main() or Thread::run) higher up.

What about those API's says there has to be a Java frame higher up. Why
can't an attached thread request a reference to the logger?

David
-----

>> This seems to be a hole in the notion
>> of "caller sensitive" to me.
>
> I'd rather say it's a hole in how such a situation is handled.
>
>> Also see discussion re: "RFR [9]: 8177036: Class.checkMemberAccess
>> throws NPE when calling Class methods via JNI"
>
> Yes - I am aware of this and this is what triggered my
> proposed patch for the few places where we use @CallerSensitive
> in logging.
>
> An application that would need can to call these logging methods
> from such a context (where no Java frame is on the stack) can
> always insert its own wrapper (e.g. a call these methods from an
> application class and call that application class from JNI instead).
>
> best regards,
>
> -- daniel
>
>>
>> David
>> -----
>>
>> On 20/03/2017 10:08 PM, Daniel Fuchs wrote:
>>> Hi,
>>>
>>> Please find below a patch for:
>>>
>>> 8177136: Caller sensitive methods Logger.getLogger,
>>> Logger.getAnonymousLogger, and System.getLogger should throw
>>> IllegalCallerException if there is no caller on the stack.
>>> https://bugs.openjdk.java.net/browse/JDK-8177136
>>>
>>> Caller sensitive methods Logger.getLogger, Logger.getAnonymousLogger,
>>> and System.getLogger currently throw an undocumented
>>> NullPointerException if they are called from JNI and there is no
>>> Java frame on the stack.
>>>
>>> Throwing NullPointerException is confusing and makes it look like there
>>> is a bug in the implementation.
>>> In truth, these method are @CallerSensitive, and therefore must not
>>> be called in a context where the caller cannot be determined.
>>> Therefore, the right thing to do is to throw IllegalCallerException
>>> and document this.
>>>
>>> webrev:
>>> http://cr.openjdk.java.net/~dfuchs/webrev_8177136/webrev.00/
>>>
>>> As per Rampdown Phase 2 Process [1] I'd also like to get
>>> confirmation that this is a reasonable proposal to fix in 9.
>>> This fix just transmutes a NullPointerException (which should
>>> never happen at this point in regular usage of the API) into an
>>> IllegalCallerException which will help diagnosing the fact
>>> that the API is called from a context where it's not supposed
>>> to be used. The risk of fixing should therefore be very limited.
>>>
>>> best regards,
>>>
>>> -- daniel
>>>
>>> [1] Rampdown Phase 2 Process
>>> http://mail.openjdk.java.net/pipermail/jdk9-dev/2017-March/005666.html
>
Reply | Threaded
Open this post in threaded view
|

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

Daniel Fuchs
On 20/03/2017 12:37, David Holmes wrote:
> What about those API's says there has to be a Java frame higher up. Why
> can't an attached thread request a reference to the logger?

Hi David,

Did you look at the webrev?

1582      * @throws IllegalCallerException if there is no caller frame, i.e.
1583      *         when this {@code getLogger} method is called from JNI
1584      *         and there is no Java frame on the stack.

This says there must be a frame higher up.

In the case of System.getLogger then the reason is that this
method eventually calls  LoggerFinder.getLogger(name, module),
which requires a non null module.

I don't see any reason why we should accept null or why we should
substitute 'null' with a (randomly picked?) module, especially
since this looks like a pretty unusual corner case which can be
easily worked around (in this case) when the behavior of the method
in the presence of a null caller is known.

best regards,

-- daniel

Reply | Threaded
Open this post in threaded view
|

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

David Holmes
On 20/03/2017 10:56 PM, Daniel Fuchs wrote:

> On 20/03/2017 12:37, David Holmes wrote:
>> What about those API's says there has to be a Java frame higher up. Why
>> can't an attached thread request a reference to the logger?
>
> Hi David,
>
> Did you look at the webrev?
>
> 1582      * @throws IllegalCallerException if there is no caller frame,
> i.e.
> 1583      *         when this {@code getLogger} method is called from JNI
> 1584      *         and there is no Java frame on the stack.
>
> This says there must be a frame higher up.

Yes but that is what you are adding. Given the basic method spec:

"Returns an instance of {@link Logger Logger} for the caller's use."

There is nothing about that which suggests any reason why the caller
must have a Java frame on their stack to make the call!

> In the case of System.getLogger then the reason is that this
> method eventually calls  LoggerFinder.getLogger(name, module),
> which requires a non null module.

That is an implementation detail.

> I don't see any reason why we should accept null or why we should
> substitute 'null' with a (randomly picked?) module, especially
> since this looks like a pretty unusual corner case which can be
> easily worked around (in this case) when the behavior of the method
> in the presence of a null caller is known.

I see this as a basic hole in the whole notion of "current module".
Surely if there is no module available then we should be in the
unnamed-module?

Cheers,
David

> best regards,
>
> -- daniel
>
Reply | Threaded
Open this post in threaded view
|

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

Alan Bateman
On 20/03/2017 20:22, David Holmes wrote:

>
> I see this as a basic hole in the whole notion of "current module".
> Surely if there is no module available then we should be in the
> unnamed-module?
There isn't any notion of "current module". If there's no caller then
you can't make any assumptions as to who the caller is. Having these
methods assume java.base or the unnamed module of some class loader
doesn't seem right as there just isn't enough context. So having the
System.getLogger methods throw IllegalCallerException looks like sanest
thing here. It also doesn't prevent taking a different approach in the
future.

-Alan
Reply | Threaded
Open this post in threaded view
|

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

Mandy Chung
In reply to this post by Daniel Fuchs
Hi Daniel,

Have you considered default to the unnamed module when there is no caller frame on the stack?

I don’t think we should make these APIs not callable from JNI attached thread even though very rarely be called from JNI (not sure any report on the current behavior throwing NPE).

Mandy

> On Mar 20, 2017, at 5:08 AM, Daniel Fuchs <[hidden email]> wrote:
>
> Hi,
>
> Please find below a patch for:
>
> 8177136: Caller sensitive methods Logger.getLogger, Logger.getAnonymousLogger, and System.getLogger should throw IllegalCallerException if there is no caller on the stack.
> https://bugs.openjdk.java.net/browse/JDK-8177136
>
> Caller sensitive methods Logger.getLogger, Logger.getAnonymousLogger,
> and System.getLogger currently throw an undocumented
> NullPointerException if they are called from JNI and there is no
> Java frame on the stack.
>
> Throwing NullPointerException is confusing and makes it look like there is a bug in the implementation.
> In truth, these method are @CallerSensitive, and therefore must not
> be called in a context where the caller cannot be determined.
> Therefore, the right thing to do is to throw IllegalCallerException
> and document this.
>
> webrev:
> http://cr.openjdk.java.net/~dfuchs/webrev_8177136/webrev.00/
>
> As per Rampdown Phase 2 Process [1] I'd also like to get
> confirmation that this is a reasonable proposal to fix in 9.
> This fix just transmutes a NullPointerException (which should
> never happen at this point in regular usage of the API) into an
> IllegalCallerException which will help diagnosing the fact
> that the API is called from a context where it's not supposed
> to be used. The risk of fixing should therefore be very limited.
>
> best regards,
>
> -- daniel
>
> [1] Rampdown Phase 2 Process
> http://mail.openjdk.java.net/pipermail/jdk9-dev/2017-March/005666.html

Reply | Threaded
Open this post in threaded view
|

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

Daniel Fuchs
Hi Mandy

On 20/03/17 20:40, Mandy Chung wrote:
> Hi Daniel,
>
> Have you considered default to the unnamed module when there is no caller frame on the stack?
>
> I don’t think we should make these APIs not callable from JNI attached thread even though very rarely be called from JNI (not sure any report on the current behavior throwing NPE).

I don't think we should make assumption or guesses.
As Alan stated: "If there's no caller then you can't make any
assumptions as to who the caller is. Having these methods assume
java.base or the unnamed module of some class loader doesn't seem right
as there just isn't enough context."

There are at least two possibilities if someone wants to get a
System.Logger from a JNI attached thread:

   1. Use an auxilliary class to indicate the implicit caller (that
      will be the auxilliary class module)

   2. Call LoggerFinder.getLoggerFinder().getLogger and explicitly supply
      a module

IMO calling directly System.getLogger is not appropriate in this case.

best,

-- daniel

>
> Mandy
>
>> On Mar 20, 2017, at 5:08 AM, Daniel Fuchs <[hidden email]> wrote:
>>
>> Hi,
>>
>> Please find below a patch for:
>>
>> 8177136: Caller sensitive methods Logger.getLogger, Logger.getAnonymousLogger, and System.getLogger should throw IllegalCallerException if there is no caller on the stack.
>> https://bugs.openjdk.java.net/browse/JDK-8177136
>>
>> Caller sensitive methods Logger.getLogger, Logger.getAnonymousLogger,
>> and System.getLogger currently throw an undocumented
>> NullPointerException if they are called from JNI and there is no
>> Java frame on the stack.
>>
>> Throwing NullPointerException is confusing and makes it look like there is a bug in the implementation.
>> In truth, these method are @CallerSensitive, and therefore must not
>> be called in a context where the caller cannot be determined.
>> Therefore, the right thing to do is to throw IllegalCallerException
>> and document this.
>>
>> webrev:
>> http://cr.openjdk.java.net/~dfuchs/webrev_8177136/webrev.00/
>>
>> As per Rampdown Phase 2 Process [1] I'd also like to get
>> confirmation that this is a reasonable proposal to fix in 9.
>> This fix just transmutes a NullPointerException (which should
>> never happen at this point in regular usage of the API) into an
>> IllegalCallerException which will help diagnosing the fact
>> that the API is called from a context where it's not supposed
>> to be used. The risk of fixing should therefore be very limited.
>>
>> best regards,
>>
>> -- daniel
>>
>> [1] Rampdown Phase 2 Process
>> http://mail.openjdk.java.net/pipermail/jdk9-dev/2017-March/005666.html
>

Reply | Threaded
Open this post in threaded view
|

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

David Holmes
On 21/03/2017 6:56 AM, Daniel Fuchs wrote:

> Hi Mandy
>
> On 20/03/17 20:40, Mandy Chung wrote:
>> Hi Daniel,
>>
>> Have you considered default to the unnamed module when there is no
>> caller frame on the stack?
>>
>> I don’t think we should make these APIs not callable from JNI attached
>> thread even though very rarely be called from JNI (not sure any report
>> on the current behavior throwing NPE).
>
> I don't think we should make assumption or guesses.
> As Alan stated: "If there's no caller then you can't make any
> assumptions as to who the caller is. Having these methods assume
> java.base or the unnamed module of some class loader doesn't seem right
> as there just isn't enough context."

I disagree. The "default" context could be the unnamed-module of the
application classloader - which would be the normal initial context for
a piece of Java code. If the JNI code needs something different to that
then it can make arrangements to load/use other classes in other
loaders/modules.

> There are at least two possibilities if someone wants to get a
> System.Logger from a JNI attached thread:
>
>   1. Use an auxilliary class to indicate the implicit caller (that
>      will be the auxilliary class module)
>
>   2. Call LoggerFinder.getLoggerFinder().getLogger and explicitly supply
>      a module
>
> IMO calling directly System.getLogger is not appropriate in this case.

If you are going to introduce an API with such a constraint then the
constraint needs to be clearly documented and the alternatives also
documented IMHO. System.getLogger relies on a notion of "current module".

Cheers,
David

> best,
>
> -- daniel
>
>>
>> Mandy
>>
>>> On Mar 20, 2017, at 5:08 AM, Daniel Fuchs <[hidden email]>
>>> wrote:
>>>
>>> Hi,
>>>
>>> Please find below a patch for:
>>>
>>> 8177136: Caller sensitive methods Logger.getLogger,
>>> Logger.getAnonymousLogger, and System.getLogger should throw
>>> IllegalCallerException if there is no caller on the stack.
>>> https://bugs.openjdk.java.net/browse/JDK-8177136
>>>
>>> Caller sensitive methods Logger.getLogger, Logger.getAnonymousLogger,
>>> and System.getLogger currently throw an undocumented
>>> NullPointerException if they are called from JNI and there is no
>>> Java frame on the stack.
>>>
>>> Throwing NullPointerException is confusing and makes it look like
>>> there is a bug in the implementation.
>>> In truth, these method are @CallerSensitive, and therefore must not
>>> be called in a context where the caller cannot be determined.
>>> Therefore, the right thing to do is to throw IllegalCallerException
>>> and document this.
>>>
>>> webrev:
>>> http://cr.openjdk.java.net/~dfuchs/webrev_8177136/webrev.00/
>>>
>>> As per Rampdown Phase 2 Process [1] I'd also like to get
>>> confirmation that this is a reasonable proposal to fix in 9.
>>> This fix just transmutes a NullPointerException (which should
>>> never happen at this point in regular usage of the API) into an
>>> IllegalCallerException which will help diagnosing the fact
>>> that the API is called from a context where it's not supposed
>>> to be used. The risk of fixing should therefore be very limited.
>>>
>>> best regards,
>>>
>>> -- daniel
>>>
>>> [1] Rampdown Phase 2 Process
>>> http://mail.openjdk.java.net/pipermail/jdk9-dev/2017-March/005666.html
>>
>
Reply | Threaded
Open this post in threaded view
|

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

Daniel Fuchs
On 20/03/17 21:28, David Holmes wrote:
> If you are going to introduce an API with such a constraint then the
> constraint needs to be clearly documented and the alternatives also
> documented IMHO. System.getLogger relies on a notion of "current module".

Yes:

     Instances returned by this method route messages to loggers
obtained by calling LoggerFinder.getLogger(name, module), where module
is the caller's module.

-- daniel
Reply | Threaded
Open this post in threaded view
|

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

Daniel Fuchs
In reply to this post by Daniel Fuchs
Hi,

I now believe I was wrong in trying to tackle both
java.util.logging.Logger::getLogger and System::getLogger
uses of @CS with a single fix.
Both issues are indeed different in nature and might require
a different fix.

java.util.logging.Logger is an existing API.
the issue already exists in previous releases - and therefore
probably doesn't deserve being fixed in JDK 9 rampdown phased 2.
As David pointed out changing the specification is also probably
not the right answer here.
I have logged https://bugs.openjdk.java.net/browse/JDK-8177325
(P3) to track the possible NPE in java.util.logging.Logger


On the other hand, System::getLogger is a new API and therefore
I believe that how the method behave when there is no caller on
the stack should be specified. I will continue to use 8177136 to
track this - and I have updated the title in consequence.

8177136: Caller sensitive method System::getLogger should specify
          what happens if there is no caller on the stack.
https://bugs.openjdk.java.net/browse/JDK-8177136

I will send a new RFR (with the new title) and an updated
patch for this.

best regards,

-- daniel

On 20/03/2017 12:08, Daniel Fuchs wrote:

> Hi,
>
> Please find below a patch for:
>
> 8177136: Caller sensitive methods Logger.getLogger,
> Logger.getAnonymousLogger, and System.getLogger should throw
> IllegalCallerException if there is no caller on the stack.
> https://bugs.openjdk.java.net/browse/JDK-8177136
>
> Caller sensitive methods Logger.getLogger, Logger.getAnonymousLogger,
> and System.getLogger currently throw an undocumented
> NullPointerException if they are called from JNI and there is no
> Java frame on the stack.
>
> Throwing NullPointerException is confusing and makes it look like there
> is a bug in the implementation.
> In truth, these method are @CallerSensitive, and therefore must not
> be called in a context where the caller cannot be determined.
> Therefore, the right thing to do is to throw IllegalCallerException
> and document this.
>
> webrev:
> http://cr.openjdk.java.net/~dfuchs/webrev_8177136/webrev.00/
>
> As per Rampdown Phase 2 Process [1] I'd also like to get
> confirmation that this is a reasonable proposal to fix in 9.
> This fix just transmutes a NullPointerException (which should
> never happen at this point in regular usage of the API) into an
> IllegalCallerException which will help diagnosing the fact
> that the API is called from a context where it's not supposed
> to be used. The risk of fixing should therefore be very limited.
>
> best regards,
>
> -- daniel
>
> [1] Rampdown Phase 2 Process
> http://mail.openjdk.java.net/pipermail/jdk9-dev/2017-March/005666.html