Re: [PATCH] 4851444: Exposing sun.reflect.Reflection#getCallerClass as a public API in Java 8

[Peter Levart]

On 09/09/2013 07:02 PM, David Chase wrote:
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.

Hi,

This is a good idea. It got me thinking that there are a bunch of 
methods in j.l.Class that are not security-sensitive. So instead of 
proxy-ing those in a wrapper SafeClass, let's just identify "unsafe" 
methods 1st. If the security checks that are planned for obtaining 
j.l.Class instances from call-stack are transferred to those unsafe 
methods instead, then holding a reference to a j.l.Class instance 
becomes a security-nonissue.

Just a quick example - presumably the "unsafe" methods of j.l.Class are: 
get(Declared)Method(s), get(Declared)Field(s) and 
get(Declared)Constructor(s), because they enable you to call/access 
public methods/fields/constructors of a class represented by j.l.Class 
object. If this class is from a restricted package (say sun.misc) then 
you could get access to restricted methods/fields/instances. Now if the 
security checks for obtaining reflective object would include checking 
the "class visibility/restrictability", then j.l.Class object of say 
sun.misc.Unsafe class would not represent any security issue. It's just 
about delaying the security check. What do you think? Are there any 
other security-sensitive j.l.Class methods? Are there any public methods 
in JDK that take j.l.Class instances and delegate to internal logic 
assuming that the caller can only pass-in security-pre-checked j.l.Class 
instances?

Regards, Peter

David

On 2013-09-09, at 10:54 AM, Mandy Chung <mandy.ch...@oracle.com> 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