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

[Nick Williams]

On Sep 3, 2013, at 7:02 PM, Mandy Chung wrote:

> Nick,
> 
> I skimmed through the changes.  Congratulations for your first patch making changes in both hotspot and jdk code.

Thanks. And, again, apologies for my extended absence.

> In my mind, the Log4J use case accessing Class instance to obtain additional information for diagnosability is different than the use case of obtaining the caller's class / loader to lookup resources.  While the new APIs might be in the same class, I will discuss them individually and keep us focus one at a time.

Ok.

> Ralph has pointed out [1] that Log4j also needs the ability to find an appropriate ClassLoader which is used for logging separation (thank you Ralph). That will be the caller-sensitivity (i.e. obtain caller's class/loader) discussion.
> 
> There are a couple of RFEs:
> https://bugs.openjdk.java.net/browse/JDK-4942697
> https://bugs.openjdk.java.net/browse/JDK-6349551
> 
> Performance is critical for Log4j to traverse the stack trace and obtain Class information.  I like your patch to speed up the generation of StackTraceElement[] (and StackTraceFrame[] - essentially same code with different type). java.util.logging.LogRecord has workaround the performance overhead and go to a specific frame directly and avoid the cost of generating the entire array. JDK-6349551 requests to lazily instantiate the StackTraceElement object unless that frame is requested.  Does Log4J always walk all frames and log all information?  Do you just log only some max number of frames rather than the entire stack trace?

It depends on what part of Log4j is using the stack trace. *Sometimes* it walks the entire stack. For example, if an exception is logged, Log4j walks the entire stack for that exception and any causing and suppressed exceptions to print out the stack trace. Most of the time, it only walks back part of the stack. For example, if the logging configuration includes the printing of the class/method/file/line that issued the log event, every time an event is logged the layout walks back the stack until it finds the caller of the trace/info/debug/warn/error/fatal method. This happens *a lot* (every time a log event is written), so performance is critical. Then there's the case where we just find the immediate caller--LogManager.getLogger() uses the immediate caller class to determine the name of the Logger. These are all important use cases of the stack in Log4j.

> Class<?> getDeclaringClass() method is the key method you need to enhance the diagnosability.  This method opens up a new way to access a Class instance that untrusted code wouldn't be able in the past.  A permission check is needed as Alan points out early.  Performance as well as logging framework can't access all class loaders are two factors to be considered when defining the permission check.

I still don't understand this assertion. Let's say I write malicious code that somehow gets a Class<?> it doesn't have access to, even with a SecurityManager installed. Will I be allowed to instantiate this class? Will I be allowed to invoke static methods on this class. If so, I see *THAT* as the big problem. Not the fact that I got the Class<?> and can inspect its properties. I can't really do anything menacing with that. I shouldn't be able to instantiate the class or invoke methods. If I can, I think *THAT* needs to be fixed.

> I saw your other comment about permission check (cut-n-paste below).  It seems to me that you don't agree a permission check is needed for the getDeclaringClass() method (regardless of which class it belongs to) and if that's the case, no point to continue.

Yes, that's my stance, as per my statement immediately above. I'm sure I could be convinced otherwise, but I haven't yet been.

> I want to make it very clear that I have agreed to take this on and provide a replacement of sun.reflect.Reflection.getCallerClass(int) in JDK 8 to address the use cases.  It will take time for the API and security discussion and please be prepared for that (also I am working on other things at the same time).

That is good news. Thank you for agreeing to take it on.

> The second comment on your patch is that there are lot of duplicated code in hotspot in order to support two similar but different types (StackTraceFrame and StackTraceElement). Serialization is the reason leading you to have a new StackTraceFrame class.

Yes. In the original discussion about this back in June, it was made *very* clear to me that we could not include the Class<?> in StackTraceElement due to serialization issues. That insistence is what I based my patch on.

> Maybe some refactoring can help but this is the cost of having the VM directly creating the instance.  One other option, as suggested in the previous thread, is to keep the declaring class in the StackTraceElement as a transient field.

It was also made *very* clear to me that transience was not an option. I was told that the Class<?> needed to be in a separate class that wasn't serializable. This insistence is another thing I based my patch on.

> If we add the getDeclaringClass method in the StackTraceElement class, it would need to specify to be optional that it only returns the Class instance when it's available.

Agreed. That's what I originally argued for because it was going to be a vastly simpler change. I'm not saying it's not the right answer, but I must admit I'll be very dismayed if we end up reverting to my original suggestion...

> There are currently three different ways to get a stack trace:
> 1. Throwable.getStackTrace()
> 2. Thread.getStackTrace() and Thread.getAllStackTraces()
> 3. java.lang.management.ThreadMXBean.getThreadInfo(long id, int maxDepth).getStackTrace() and multiple thread IDs version
>  (a) local (b) remote
> 
> Since it's a new StackTraceFrame class, you have to provide a new method replacing #1 and #2.  I don't see any need to provide a new API equivalent to Thread.getAllStackTraces() and java.lang.management.

Agreed. And that's why I didn't.

> The proposal I originally have last week was to keep declaring class as transient and add a method in StackTraceElement with a permission check at every call. Tom raises a good question about the cost of permission check.  Would that be a concern to Log4j?

Anything that has cost is a concern to Log4j. Additionally, please see my earlier reservations about a security check when calling getDeclaringClass. I see no need for a permission check to determine whether they can obtain the Class<?> if there are additional security checks that keep them from instantiating and/or using the class.

> Is log4j on bootclasspath or extension directory?  I assume not.

Not usually. Typically it's just on the app class path. In a Java EE application server / Servlet container, it may be on the container class path (if the container is using Log4j), or it may be on the application's WEB-INF/lib class path (if the container is not using Log4j but the application is).

> So for log4j to work with security manager installed, it would have torequire the application to grant certain permissions - can you confirm?

Maybe one of the other Log4j guys listening in can provide input on this. Ralph? I'm relatively new to the Log4j team (just joined in last 6 months). Personally, I've never heard anyone complain about having issues with the SecurityManager. However, Log4j 1 did not use getCallerClass, and many users are still on that version. Log4j 2 started using getCallerClass.

> For example it calls sun.reflect.Reflection.getCallerClass method that will require RuntimePermission("accessClassInPackage.sun.reflect") permission.  Calling Class.getProtectionDomain and Class.getClassLoader() requires RuntimePermission("getProtectionDomain") and
> RuntimePermission("getClassLoader") respectively.  That gives me an impression that permission check on individual stack frame might be a non-issue?

I'm not sure exactly what you mean here, but okay. There are security restrictions. So, you're probably right, we're bound to run into trouble eventually. I'd like to eliminate that trouble, including for users that have no control over their security manager.

> 
> Mandy
> [1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-September/020525.html
> 
> On 9/3/13 5:24 AM, Nick Williams wrote:
>> On Sep 3, 2013, at 4:07 AM, Alan Bateman wrote:
>>>> I'm not voicing any objection to any kind of security/permissions checks in these methods. Before I can pass judgement one way or another, I'd want to know 1) specifically what type of permissions check you are looking for, and 2) what you're looking to achieve with said permissions check.
>>> I would say this is TBD and start by asking the question as to whether there are concerns about leaking reference to Class objects that untrusted code wouldn't normally be able to get a reference to. Tom brings up the cost of the permission check and also whether any API should be tied to class loader. There are clearly discussion points here that could potentially influence the API.
>> As I have said before, there are MANY ways to get a Class object that aren't security checked. It's when you try to USE that class object to impersonate it or invoke methods that security checks begin to happen. As they should!
>> 
>> Nick
> 
> 
> 
> On 9/1/13 1:16 AM, Nick Williams wrote:
>> I have completed and am proposing a patch for replacing sun.reflect.Reflection#getCallerClass(...) with a public API in Java 8. I saw no point in duplicating an issue, even though it's over 10 years old, so I'm indicating that this is a patch for 4851444 (http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=4851444).
>> 
>> I welcome and solicit support/+1s and a sponsor. Someone about a month ago had mentioned that they would be willing to be a sponsor if the patch looked good, but I can't remember who it was and I can't find the email. I want to say it was someone with RedHat, but my memory could be faulty, so please don't hold it against me if I'm wrong.
>> 
>> *Summary of Changes*
>> Added the new class java.lang.StackTraceFrame. It's a virtual mirror of StackTraceElement, except that it contains a Class<?> declaringClass property instead of a String className property. Since the list members expressed reluctance to add methods to Thread and Throwable, StackTraceFrame also contains several static methods for retrieving Classes and StackTraceFrames from the stack. These methods are as follows:
>> 
>> Class<?> getCallerClass(): Retrieves the class of the caller of the method calling getCallerClass(). This is identical to the new Reflection#getCallerClass() added in Java 7u25/8.
>> 
>> Class<?> getCallerClass(int): Retrieves the class of the caller n frames down the stack. This is identical to the old Reflection#getCallerClass(int) that was deprecated in Java 7u25 and removed in Java 8.
>> 
>> StackTraceFrame getCallerFrame(): Retrieves the StackTraceFrame of the line of code that called the method calling getCallerClass(). This is similar to the new Reflection#getCallerClass() added in Java 7u25/8, except that it returns a StackTraceFrame.
>> 
>> StackTraceFrame getCallerFrame(int): Retrieves the StackTraceFrame of the caller n frames down the stack. This is similar to the old Reflection#getCallerClass(int), except that it returns a StackTraceFrame.
>> 
>> StackTraceFrame[] getCurrentStackTrace(): Functionally equivalent to Thread#getStackTrace(), except that it returns an array of StackTraceFrames.
>> 
>> StackTraceFrame[] getStackTrace(Throwable throwable): Functionally equivalent to Throwable#getStackTrace(), except that it returns an array of StackTraceFrames. It uses the same save point (backtrace) created when the Throwable is created that Throwable#getStackTrace() uses when it's first called. It caches the array of StackTraceFrames in the Throwable just like the array of StackTraceElements are cached, so that multiple calls for the same Throwable are fast.
>> 
>> As a result of this change, sun.reflect.CallerSensitive has been moved to java.lang.CallerSensitive.
>> 
>> I spent considerable time reviewing, revising, considering, and testing these changes. I included several unit tests that establish the proper behavior. I also spent considerable time benchmarking the changes. While benchmarking, I noticed some things. First, getCallerClass() and getCallerClass(int) are as fast as their counterparts in sun.reflect.Reflection, and they easily inline when appropriate. Second, getCallerFrame() and getCallerFrame(int) are /almost/ as fast as the Class versions, but there is some additional overhead for the construction of the StackTraceFrame. This is minuscule (1,000,000 invocations consume around 500 ms total on my machine). At this point, all of the benchmarking was as expected.
>> 
>> However, I then encountered a surprise. The getCurrentStackTrace() and getStackTrace(Throwable) methods executed (again, 1,000,000 times) in about 70% of the time that Thread#getStackTrace() and Throwable#getStackTrace() did, respectively. Theoretically, they should have executed in the same amount of time, not faster. After extensive analysis, I discovered (what I considered to be) a serious flaw in how the stack trace is filled in within Throwable (which also affects how Thread#getStackTrace() works).
>> 
>> Instead of simply iterating over the entire save point and filling in the Throwable stack trace in native code (which is what I did when I implemented the StackTraceFrame methods), the Java code in Throwable first called a native method to figure out how deep the stack was, then called another native method once for every frame in the stack to retrieve each element individually. This native method that is called repeatedly iterates over the entire backtrace once for each call, stopping only when it finds the matching element (so it's O(1) for the first call, O(2) for the second call, O(3) for the third call, and so on). While my StackTraceFrame methods were iterating over the backtrace exactly 1 time (O(n)), the Throwable methods were iterating over the backtrace 1+(n/2) times (worse than O(nlogn) but not as bad as O(n^2)). This problem would not have been extremely apparent over small stack traces (the 30% improvement was a stack trace of 6 elements), but over a large (200+ elements) stack traces the performance difference would be huge and noticeable. Seeing a room for improvement, I refactored the code that fills in the stack trace for Throwable, improving its performance accordingly to match the performance of the StackTraceFrame methods.
>> 
>> I'm very pleased with how this turned out, and both the unit tests and my extensive functional testing show that the new class and its methods are working great. I just need someone willing to review and sponsor my patch!
>> 
>> *The Code Changes*
>> I couldn't get WebRev to run without all kinds of errors. So I ran `hg diff -g` on every repository in the forest with changes. Here are the four patch files for the four repositories that had changes (no other repositories had changes):
>> 
>> https://java.nicholaswilliams.net/Java8GetCallerClass/jdk8.patch
>> https://java.nicholaswilliams.net/Java8GetCallerClass/jdk8_jdk.patch
>> https://java.nicholaswilliams.net/Java8GetCallerClass/jdk8_hotspot.patch
>> https://java.nicholaswilliams.net/Java8GetCallerClass/jdk8_nashorn.patch
>> 
>> I believe I have followed all of the procedures as closely as possible. I await feedback and hope for some support on this, so that we can get a public replacement for this method in Java 8. Let me know if you have any questions.
>> 
>> Thanks!
>> 
>> Nick
> 
>