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

[Nick Williams]

On Sep 4, 2013, at 6:23 PM, Mandy Chung wrote:

> On 9/1/2013 1:16 AM, Nick Williams wrote:
>> 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.
>> 
> 
> The other part of this patch is about the ability to obtain the caller information.  The use case includes Groovy 2.x and GUI app to look up resource bundles on behalf on the caller and Log4j for logging isolation depending on its caller (isolation on ClassLoader for the app).
> 
> Such methods are caller-sensitive and must obtain the right caller; otherwise it might get surprises.
> 
> The proposed getCallerClass() method, no-arg version, looks reasonable and it will return the immediate effective caller of the method calling the getCallerClass method. Use your example (I add the class for the discussion later).  Let's ignore the @CallerSensitive annotation for now.
> 
>    getCallerClass();
>    Foo.someMethod1();
>    Bar.someMethod2();
>    Gee.someMethod3();
>    App.actualCallerMethod()
> 
> The caller is Bar.someMethod2() and getCallerClass() method will return Bar.  It will return the same result even if Foo is called via reflection (the VM already handles this and skips the reflection machinery).
> 
> When security manager is installed, under what condition can Foo class access Bar class (any permission check)?  What if Bar is in a restricted package?

I would say "under all circumstances." For example, if some bit of code is able to call Log4j's LogManager.getLogger(), I see no reason that LogManager should ever be restricted from seeing Bar. It should be a two-way street. Restricting access to Bar would be like an email provider allowing people to email me but not allowing me to email them back. Makes no sense. If I'm a method, and some other method can call me, I should have visibility to that calling method.

> The standard way to determine if class A has access to class B is based on their class loaders and also it is a restricted class (configured in the package.access property in JRE/lib/security/java.security).  Code has full access to its own class loader and any class loader that is a descendent.  There are several methods in java.lang.Class that are performing this security check:
> 
>     * @throw  SecurityException
>     *         If a security manager, <i>s</i>, is present and the caller's
>     *         class loader is not the same as or an ancestor of the class
>     *         loader for the returning Class object and invocation of {@link
>     *         SecurityManager#checkPackageAccess s.checkPackageAccess()}
>     *         denies access to the package of returning Class object
> 
> I think the above security check should be applicable to the getCallerClass method.

I disagree, for reasons I stated above. I think the security check should happen later, preventing the holder of the Class<?> from instantiating or invoking a restricted class or a class in a restricted package or restricted ClassLoader.

> It means that
> Foo can access Bar if Foo's class loader is the same or an ancestor of Bar's class loader; otherwise, it will perform the package access check. BTW - including an example in the javadoc would be helpful since the term "caller" might be confusing.

Agreed--an example should go in the Javadoc for these methods.

> For the known use cases, they are interested in getting the caller's class loader.  Class.getClassLoader method has the following security check:
> 
>     * <p> If a security manager is present, and the caller's class loader is
>     * not null and the caller's class loader is not the same as or an ancestor of
>     * the class loader for the class whose class loader is requested, then
>     * this method calls the security manager's {@code checkPermission}
>     * method with a {@code RuntimePermission("getClassLoader")}
>     * permission to ensure it's ok to access the class loader for the class.
> 
> I wonder if Class instance is really needed while I understand we can't anticipate all cases.  Most of the caller-sensitive methods in the JDK only need to get caller's ClassLoader.

But not most of the caller-sensitve methods in Log4j--it needs to access the class name, package, ClassLoader, and CodeSource.

> There are very few cases that require the caller class (used in the reflection implementation that I have yet to understand deeper).   Perhaps all you need is getCallerClassLoader method?

Nope. See above.

> In that case, you will be able to get the caller's class loader if you have the access.  This is one thing I'd like to explore further (either one or both).
> 
> Performance - I believe in common cases Foo's class loader should be the same or ancestor of Bar's clas loader.  Bar calls Foo which should be visible to Bar's class loader.  It'd be very useful if Groovy and Log4j can measure the performance overhead with security manager installed when it's available and see if it is a concern.

I can't guarantee that I can do this within the next month. I'm still not done with my book, and it has to come first. Maybe one of the other Log4j devs can. Ralph? Can you take this on?

> About the proposed getCallerClass(int) method to find a frame at a specific depth is essentially putting back the sun.reflect.Reflection.getCallerClass(int depth) method.  This method is very flexible but brittle.

It's what Log4j is currently using to fill in as much of the stack trace as it can for Throwables. It calls getCallerClass repeatedly with increasing depth to build the stack trace. If we have a way to get the stack trace with Class<?>es (StackTraceFrame.getCurrentStackTrace(), StackTraceFrame.getStackTrace(Throwable)), Log4j no longer needs to use getCallerClass(int). However, there could be other code out there using getCallerClass(int) where getCallerClass() would not work and getting a stack trace with Class<?>es would be a performance hindrance. I wanted to ensure these use cases were supported, but admittedly I can't immediately enumerate what these uses cases might be.

> I still think it's more reliable that a caller-sensitive method should capture the caller and pass it to the runtime properly. Groovy 3.x doesn't do the stack walk. Groovy 2.x needs a temporary solution to filter out the intermediate frames of groovy runtime, would StackTraceFrame.getDeclaringClass be an adequate interim solution?

I can't speak to this.

> More on @CallerSensitive and s.r.Reflection.getCallerClass(int):
> 
> We had found severe bugs in the code to call s.r.Reflection.getCallerClass(int) when prototying the fix for JEP 176 but they were not noticed and no test uncovered them.  e.g. wrong depth was used (easy to miscount the depth or the depth is modified due to refactoring but difficult to be caught during review).  As a caller-sensitive method, it's important to find the right caller; otherwise unexpected behavior.
> 
> In the first prototype, Chris Thalinger did implement JVM_GetCallerClass to walk past all @CS frames and find the immediate caller.  All methods in the method chain invoked by the actual caller must be annotated by @CS which ends up something like the example you used earlier:
> 
> @CallerSensitive getCallerClass(int)
> @CallerSensitive someMethod1()
> @CallerSensitive someMethod2()
> @CallerSensitive someMethod3()
> @CallerSensitive someMethod4()
> actualCallerMethod()
> 
> With more analysis, we identified that there was no absolute need (in the JDK) to walk the stack but instead a more reliable way is to capture the caller at the entry point of the caller-sensitive methods.  In the example above, someMethodxx() are internal implementation in the JDK case and thus we modified them to take the caller class parameter (instead of looking it up at its execution point).  This greatly simplifies the VM enforcement to detect if the method calling JVM_GetCallerClass is a caller-sensitive method.

I do agree with how it was ultimately implemented. The main theoretical problem I see with the first prototype is that if the caller method is itself annotated @CallerSensitive for its own reasons, it breaks the method it's calling. The way it was ultimately implemented prevents this from happening. Of course, that eliminates the need for the specific @CallerSensitive annotation completely, in my opinion. But that's just my $0.02.

> My feedback is that the getCallerClass() method seems to be adequate for all use cases except Groovy 2.x.  I would suggest traversing the stack trace (with the new getDeclaringClass method) be the alternative to adding getCallerClass(int) method.  Jochen and others - what do you think?
> 
> On the @CS subject, if we keep @CallerSensitive in sun.reflect in JDK8, the new getCallerClass() method is a caller-sensitive method and if it is called by the system classes, it will enforce that the caller must be annotated with @s.r.CS; if it called by non-system classes, the VM will return the caller and no check on the annotation.

I'm fine with this approach, I just see little point in using @CallerSensitive at all once the API is public. The native code as written would still work identically to the way it does now if we removed the @CallerSensitive annotation.

> The implication is in the 292 method handles. The current behavior is that if a MethodHandle for a caller-sensitive method is requested, it will bind the method handle with the lookup class (i.e. as if it were called from an instruction contained in the lookup class).  There is no change to the current behavior since the current implementation only allows JVM_GetCallerClass be called from system classes.  If an app (e.g. Foo.m method) calls the new getCallerClass method, a MethodHandle for Foo.m will not bind with the lookup class.  This will need to get John Rose and other 292 members' feedback on it.

I don't think I understand what you're saying/asking here, but maybe it wasn't meant for me.

> 
> This is my thinking so far.  Feedback and comments?
> 
> Mandy