Re: [PATCH] 4851444: Exposing sun.reflect.Reflection#getCallerClass as a public API in Java 8

[Jörn Huxhorn]

On 2. September 2013 at 10:33:59, Alan Bateman (alan.bateman at oracle.com) wrote:
On 01/09/2013 21:04, Nick Williams wrote: 
> : 
> 
> So David says he thinks he can find someone at RedHat to sponsor this, assuming it looks good. 
> 
> Nick 
I see the patch is accessible now. 

From a quick scan then it appears that you've decided to ignore the 
security concerns so I don't think anyone can (or should) sponsor this 
patch until there is further discussion on the API and the security 
implications. 
I don't think Nick "decided to ignore the security concerns".

I know about [0] but I'm not exactly sure which security measures are necessary. He can simply add security checks as requested.

What exactly are the security concerns, anyway? Calling methods on a class retrieved in that way would still require a potentially installed SecurityManager to allow the call, right? I don't say that there are no security issues, I'd just like to get a better understanding of the issues at hand.

To help things then one suggestion is start small and just focus on the 
no-arg getCallerClass and the method to return the array of stack traces 
(be it an extended StackTraceElement or the new StackTraceFrame that you 
are proposing). The getCallerClass will need a permission check, as will 
the method to get the extended stack trace. I think it would also be 
useful to lay out the points on extending StackTraceEment vs. 
introducing a few StackTraceFrame type. 
I think the rationale for StackTraceFrame vs. enhanced StackTraceElement is serialization. StackTraceElement is Serializable but Class, while being Serializable, would likely fail deserializing on remote systems with a different classpath. It's a common use case to serialize StackTraceElements in case of logging frameworks, which would then be deserialized on a remote server. It would be possible to declare that field transient but this would result in null values in deserialized instances.

StackTraceFrame, on the other hand, does not implement Serializable and declaringClass is a mandatory field. Two distinct classes are the cleaner solution (no impact on Throwable, no null checks required retrieving the class). (A convenience method to transform a StackTraceFrame into a StackTraceElement would probably be handy, though.)

This was discussed in [1] and [2]. I had the impression that going for a separate class was the way to go.

Making sun.reflect.CallerSensitive public was discussed in [3]. Doing so sounds like a good idea to me if going for the "full" JEPS-176 instead of the "alternative".

Nick also explained a performance issue in Thread#getStackTrace() and Throwable#getStackTrace(). Keeping that in mind, I'm not sure if going for "start small" (i.e. leaving that issue alone) would be a wise decision.

The thing is that we really need a proper solution before API freeze on 2013-10-10. This is a proper solution with regards to the API, with as little side effects as possible (due to not changing StackTraceElement, Throwable or Thread regarding declaringClass). I'd suggest to not restart the discussion about whether adding this to StackTraceElement or putting it into a separate class. We should instead try to discuss this existing API and clarify what needs to be changed to get this approved.

I hope I don't come across rude or something. The prospect of a Java 8 without a proper replacement API for sun.reflect.Reflection#getCallerClass is simply giving me serious creeps and time is very pressing if we want to pull this off.



Jörn.

---

[0] http://openjdk.java.net/jeps/176

[1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2012-October/thread.html#11665

[2] http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-June/thread.html#18049

[3] http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-June/thread.html#18353