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
