[PATCH] 4851444: Exposing sun.reflect.Reflection#getCallerClass as a public API in Java 8
mandy.chung at oracle.com
Wed Sep 4 01:47:01 UTC 2013
Just realize that I missed a few words in a sentence that could mislead
what I really mean.... correction below.
On 9/3/2013 5:02 PM, Mandy Chung wrote:
> I skimmed through the changes. Congratulations for your first patch
> making changes in both hotspot and jdk code.
> 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.
> Ralph has pointed out  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:
> 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?
> 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 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.
> 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).
What I really meant is:
I want to make it very clear that I have agreed to take this to come to
a conclusion and ensure to 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).
> 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. 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. 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.
> 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.
> 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? Is log4j on
> bootclasspath or extension directory? I assume not. So for log4j to
> work with security manager installed, it would have torequire the
> application to grant certain permissions - can you confirm? 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
> 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!
> 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
>> 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 getCurrentStackTrace(): Functionally equivalent to
>> Thread#getStackTrace(), except that it returns an array of
>> 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):
>> 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.
More information about the core-libs-dev