Replacement of sun.reflect.Reflection#getCallerClass

Tue Sep 3 16:28:19 UTC 2013

I'd just like to add to this thread that Log4j is copying behavior that has been in Logback for years.  The default behavior for both is to print a stack trace that includes the jar or directory where the class was located and its version. User's find this extremely helpful when diagnosing problems.

In addition Log4j is using getCallerClass to determine the caller of LogManager.getLogger() so it can determine which LoggerContext to associate it with (based on the ClassLoader of the caller).  This is to solve the issues raised at http://logback.qos.ch/manual/loggingSeparation.html#tamingStaticRefs.

Ralph

On Sep 3, 2013, at 5:52 AM, Nick Williams wrote:

> 
> On Sep 2, 2013, at 10:04 PM, Mandy Chung wrote:
> 
>> Hi Nick,
>> 
>> Thanks for the patch.
>> 
>> JEP 176 [1] describes the caller-sensitive method and the need for a mechanical checking of caller-sensitive methods.  Also Peter Levart in [2] explained the change in MethodHandles.Lookup related to @CS.  I assume you understand the rationale behind and the potential security issues.  In general defining caller-sensitive API is discouraged.
> 
> Do, I don't understand the rationale. Alan said the security issues couldn't be discussed openly. I can get a Class object MANY different ways without a security check. I don't see or understand any vulnerabilities here. I'm going to need much more information in order to contribute to the discussion in an informed manner.
> 
> As I told Alan in a separate thread (I wish we had kept this in one thread):
> 
>> If applications want to change their behavior based on the caller, let them! Is it bad practice, bad design, and likely all kinds of dumb? Heck yea. But there are legitimate uses of this. Just because there are bad uses of this feature doesn't mean you must omit it. If that's the case, then we need to get rid of all input/output in Java--it could be used to write viruses to the file system! Oh, and we should remove Random, too, because applications might Randomly change their behavior! We must protect people from their own mistakes.
> 
> 
> My sarcasm aside, I hope the point is clear. There are also legitimate uses for things for which there are illegitimate uses. Farmers need diesel for their tractors and fertilizer for their fields, but mix them and you can create a bomb. A caller-sensitive API, discouraged as it may be, is sometimes 100% necessary, and logging frameworks are the prime example. I also said in the other thread:
> 
>> Think of the performance improvements that could be had if, while determining the source of an event, loggers could get the exact one frame they needed (via StackTraceFrame#getCallerFrame, ~100ms per 1,000,000 calls) instead of having to generate an entire stack trace and loop through it to find the one frame (Thread#getStackTrace(), ~3,000ms per 1,000,000 calls).
> 
> I hope this is all articulated well. We need a caller-sensitive API. That's just the whole of the story. Why should java.util.logging.Logger get to use getCallerClass and Log4j not get to use it!? That is neither open nor fair.
> 
>>  Defining a SE supported @CallerSensitive and also getCallerClass API poses the risk of "encouraging" developers to implement more @CS methods while not fully understand its implication.
> 
> And that would be their mistake. Document the heck out of it! Put big red warnings on it. Whatever makes you feel that you have disclaimed the user enough, do it. Providing a file access API poses the risk of encouraging developers to read and write files while not fully understanding the potential security issues, too.
> 
> I'm just going to point out something, again, that I pointed out twice in June. C#/.NET has the ability to A) get the caller Type (equiv. of our class) and B) get the stack trace as StackFrames (the equivalent of the StackTraceFrame I'm adding), which contain the Type. In fact, you can ONLY get the stack trace as StackFrames. There's no StackTraceElement equivalent in C#/.NET. Has the world blown up yet?
> 
>> It was a non-goal of JEP 176 to provide @CallerSensitive as a public API and I would suggest not to define it as a public API in JDK 8.
> 
> And, has been stated many, many times, this non-goal is incompatible with the community's needs. Now, there /is/ a way to avoid making @CallerSensitive public (which the community doesn't care about) while still making getCallerClass public (which is really what the community needs). In order to do so, you must remove the check that requires the method calling getCallerClass/getCallerFrame to be annotated with @CallerSensitive. Once you remove that check, you don't need @CallerSensitive to be public. To be clear, though, once you remove that check, you don't need @CallerSensitive /at all/. It can simply go away.
> 
>> While I'll take the time to look at your patch, I would like to restart the discussion from the use cases (in which led to what you summarized the need of your proposed API [3]):
>> 
>> 1. Groovy 1.x and 2.x use the sun.reflect.Reflection.getCallerClass(int depth) method to:
>> 	• emulates the way ResourceBundle.getBundle(String, Locale) works. Groovy runtime introduces intermediary stack frame between a caller and the call to getBundle, these frames needs to be filtered out; when looking for the caller classloader.
>> 	• support the annotation @Grab, @Grab allows to specify a dependency between a code and a module (using apache ivy under the hood). The resolution is done at runtime and require a specific Classloader (the GroovyClassLoader), getCallerClass is used to find the class loader of the caller, again filtering out the intermediary stack frame.
>> Groovy 3.x has a different implementation that doesn't need to do stack walk to filter its runtime frames and find the caller
>> 
> 
> While I'm sure you're probably correct, I don't have enough Groovy knowledge to know for sure.
> 
>> 2. Log4j
>> 
>> Log4j offers "extended stack trace" patterns that requires access to a Class object when exceptions or stack traces are logged to provide additional information for troubleshooting such as the implementation version of the Package, code source, etc. It currently uses the sun.reflect.Reflection.getCallerClass(int depth) method to find the Class object matching the StackTraceElement in a Throwable or stack trace. If the sun.reflect.Reflection.getCallerClass(int depth) method doesn't exist, it will use SecurityManager.getClassContext().
> 
> Correct, mostly. And if it can't use SecurityManager#getClassContext(), it must use Class#forName() on the array of StackTraceElements (slooooowwww). To be clear, though, there are three needs:
> 
> 1) Get the class name and class loader for the method getting a Logger via getLogger.
> 2) Get the stack trace for a Throwable such that it contains Classes for obtaining the CodeSource information.
> 3) Get the class, method, file name, and line number of code calling a Logger method to determine the source of a log event.
> 
> We solve 2) by resolving /most/ of the Throwable's stack trace elements to Classes returned by getCallerClass with increasingly incremented indexes. This isn't perfect, because the Throwable stack trace is never identical to the current back trace. We have to fill in the missing pieces still with Class#forName. This is why we need to get StackTraceFrame[]s for Throwables instead of StackTraceElement[]s.
> 
>> This approach is not reliable since the HotSpot implementation of getCallerClass filters out the frames corresponding to reflection machinery (its intent is to find the true caller) whereas a stack trace contains all Java frames up to MaxJavaStackTraceDepth (default is 1024).
> 
> This is correct, and another reason why getting StackTraceFrame[]s for Throwables would be preferable to us. 
> 
>> When there is no Class object matching a StackTraceElement, it will fall back and load the class (does Log4j know which class loader to use?)
> 
> We do not know which class loader to use, so we can't always get the Class. We simply display no extra information for these frames. This is rarely a problem, though, because in our use case Throwable stack traces rarely have Classes that belong to sibling class loaders. They almost always belong to the current class loader, the parent class loader, or one of the children class loaders.
> 
>> Log4j currently works in the absence of the sun.reflect.Reflection.getCallerClass(int depth) method but performance is a very major issue.
> 
> Yep.
> 
>> 3. APIs on behalf of the caller
>> 
>> For example, locating a resource on behalf of the caller class to avoid explicit additional Class and/or ClassLoader parameters.
> 
> Yep.
> 
> Nick
> 
>> Please correct/add if I miss anything.
>> 
>> More will be discussed tomorrow.
>> 
>> Mandy
>> [1] http://openjdk.java.net/jeps/176
>> [2] http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-July/019397.html
>> [3] http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-July/019334.html
>> 
>> On 9/1/2013 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+!
>>>  element
>>> s) 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
>>> 
>> 
> 