Take this lightly informed suggestion with a grain of salt, but why not, for
purposes of performance and security,
change the logging-specific getCallerClass methods so that their "class"
references are instead wrapped in some sort of proxy object that only forwards
certain operations quickly without a security check? For example, equals,
hashcode, and toString are probably not security-sensitive.
i.e.
class SafeClass {
private final Class clazz;
public SafeClass(Class clazz) { this.clazz = class; }
public String toString() { return clazz.toString(); }
public int hashCode() { return clazz.hashCode(); }
public boolean equals(Object o) { return clazz.equals(o); }
public Class maybeWeLetYouLookAtTheRealClass() { ... a bunch of security
checks ... }
}
If necessary, do the same for classloaders.
And them, no security checks needed, as long as the "safe" methods are enough
to get the job done.
David
On 2013-09-09, at 10:54 AM, Mandy Chung <[email protected]> wrote:
> 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
>>
>>
>
