Nick,
Do you have any information to some of the questions I asked below that
can help the API discussion?
We need to decide on the permission check and also the hotspot change
has to be integrated and the jdk change will need to wait until it shows
in a promoted build. Schedule-wise, to make JDK 8, we should finalize
the API this week so that you can update the patch soon.
Mandy
On 9/3/2013 5:02 PM, Mandy Chung wrote:
Nick,
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 [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?
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).
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
non-issue?
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
