RE: Thoughts on adding getElementClass() method to StackTraceElement?
Nick Williams wrote: What if we also added a getStackFrames() method to Throwable? That would meet my needs but it would also satisfy what I'm observing is a desire to have a new API for this (StackFrame) instead of adding it to StackTraceElement. I'm very open to how it's implemented, as long as it satisfies my use case. :-) The stack trace of a Throwable can be filled in on demand when getStackTrace() is called the first time, so that the overhead isn't incurred when creating and throwing the exception. Presumably, we would need to do something similar with getStackFrames(), especially since calling it would be less common. Thoughts on this? Yes that is reasonable, but I'd add a static method to StackFrame instead. Something like StackFrame[] capture(Throwable). Regards, Jeroen
Re: Thoughts on adding getElementClass() method to StackTraceElement?
On 06/17/2013 08:06 AM, Jeroen Frijters wrote: Nick Williams wrote: What if we also added a getStackFrames() method to Throwable? That would meet my needs but it would also satisfy what I'm observing is a desire to have a new API for this (StackFrame) instead of adding it to StackTraceElement. I'm very open to how it's implemented, as long as it satisfies my use case. :-) The stack trace of a Throwable can be filled in on demand when getStackTrace() is called the first time, so that the overhead isn't incurred when creating and throwing the exception. Presumably, we would need to do something similar with getStackFrames(), especially since calling it would be less common. Thoughts on this? Yes that is reasonable, but I'd add a static method to StackFrame instead. Something like StackFrame[] capture(Throwable). New API could be entirely unrelated to Throwable, if there was support for it in native code. Since there would have to be changes to the native code anyway to support this, why not create a separate API? Regards, Peter Regards, Jeroen
Re: Thoughts on adding getElementClass() method to StackTraceElement?
On Jun 17, 2013, at 1:12 AM, Peter Levart wrote:
On 06/17/2013 08:06 AM, Jeroen Frijters wrote:
Nick Williams wrote:
What if we also added a getStackFrames() method to Throwable? That would
meet my needs but it would also satisfy what I'm observing is a desire
to have a new API for this (StackFrame) instead of adding it to
StackTraceElement. I'm very open to how it's implemented, as long as it
satisfies my use case. :-)
The stack trace of a Throwable can be filled in on demand when
getStackTrace() is called the first time, so that the overhead isn't
incurred when creating and throwing the exception. Presumably, we would
need to do something similar with getStackFrames(), especially since
calling it would be less common.
Thoughts on this?
Yes that is reasonable, but I'd add a static method to StackFrame instead.
Something like StackFrame[] capture(Throwable).
New API could be entirely unrelated to Throwable, if there was support for it
in native code. Since there would have to be changes to the native code
anyway to support this, why not create a separate API?
I'm not sure who's misunderstanding who. :-)
If some third party code that I have no control over throws an exception and I
catch that exception, or some other code catches the exception and passes it to
my code (because my code is a logging framework), I need to get the
StackFrame[] for _when the exception was thrown_. Not the StackFrame[] related
to my current method execution. How can that possibly be entirely unrelated to
throwable?
The way I understand Jereon's suggestion, I'm thinking StackFrame would look
like this:
public final class StackFrame {
public Executable method();
public String getFileName();
public int getLineNumber();
/** Shortcut for getModifiers() and Modifiers.NATIVE */
public int isNativeMethod();
/** Format exactly like StackTraceElement#toString() */
public String toString();
/** Gets current executing stack with number of frames skipped and max length
*/
public static StackFrame[] capture(int skipFrames, int maxLength, boolean
includeSourceInfo);
/** Gets current executing stack with no frames skipped and no max length */
public static StackFrame[] capture();
/** Gets stack from when Throwable was created. */
public static StackFrame[] capture(Throwable t);
}
But perhaps I am missing something.
Nick
Re: Thoughts on adding getElementClass() method to StackTraceElement?
On 06/15/2013 09:59 PM, Nick Williams wrote: On Jun 15, 2013, at 2:22 PM, Remi Forax wrote: Could you be a little more clear why you need a class for your logging API, exactly, why the class name is not enough for logging purpose ? Certainly. Log4j 2 offers extended stack trace patterns. When exceptions or stack traces are logged, or when displaying the location that the logging statement originated from, the stack trace/log also reveals the resource (JAR file, directory, etc.) that the class was loaded from and the implementation version from the package the class is in. This helps reveal possible class loading issues (among other things), which can be an extremely valuable tool when troubleshooting problems with an application. Log4j 2 accomplishes this by getting the class associated with a StackTraceElement (as outlined below), and then 1) getting the Package from the Class and the version from the Package, and 2) getting the ProtectionDomain from the Class, getting the CodeSource from the ProtectionDomain, and getting the JAR/URL from the CodeSource. The last two parts are easy. It's getting the Class from the StackTrace that is extremely inefficient if not done right. Java 8 took away the best way we had found to do that (which, admittedly, was because we were relying on a non-standard class, which isn't recommended). On 06/17/2013 08:12 AM, Peter Levart wrote: New API could be entirely unrelated to Throwable, if there was support for it in native code. Since there would have to be changes to the native code anyway to support this, why not create a separate API? If I understand this correctly, one part of your code is displaying the resource location of each method's declaring class in the stack trace when such extended stack trace is configured. Therefore you need this API to capture the class-enriched-stack-trace at the time the Throwable instance is constructed. No special Throwable-unrelated API to return the stack trace would be suitable for this purpose then. But for the purpose of displaying the resource location of the class invoking the logging API, you don't need the entire stack trace, just the immediate caller class, right? When @CallerSensitive internal annotation was introduced (and Reflection.getCallerClass(int frames) was dropped), there was some debate about possible introduction of public API that could be used to reveal the caller's class. Perhaps there's still time to create such API for JDK8? At least one part of your logging functionality would be covered with such API. Suppose the API to obtain the immediate caller class (j.l.Class object) was public and fast, so you could do that for each call to the logging API eagerly (after initial check for logging level). Now, could you delay the evaluation of extended stack trace pattern to the latest time possible, when you must format the stack-trace? Would calling Class.forName(name, false, classLoader) for each individual StackTraceElement, using the ClassLoader of the caller class, still present a performance problem? You would only get locations for classes that the caller has access to, so no inaccessible information could be revealed. Regards, Peter
Re: Thoughts on adding getElementClass() method to StackTraceElement?
On Jun 17, 2013, at 1:55 AM, Peter Levart wrote: On 06/15/2013 09:59 PM, Nick Williams wrote: On Jun 15, 2013, at 2:22 PM, Remi Forax wrote: Could you be a little more clear why you need a class for your logging API, exactly, why the class name is not enough for logging purpose ? Certainly. Log4j 2 offers extended stack trace patterns. When exceptions or stack traces are logged, or when displaying the location that the logging statement originated from, the stack trace/log also reveals the resource (JAR file, directory, etc.) that the class was loaded from and the implementation version from the package the class is in. This helps reveal possible class loading issues (among other things), which can be an extremely valuable tool when troubleshooting problems with an application. Log4j 2 accomplishes this by getting the class associated with a StackTraceElement (as outlined below), and then 1) getting the Package from the Class and the version from the Package, and 2) getting the ProtectionDomain from the Class, getting the CodeSource from the ProtectionDomain, and getting the JAR/URL from the CodeSource. The last two parts are easy. It's getting the Class from the StackTrace that is extremely inefficient if not done right. Java 8 took away the best way we had found to do that (which, admittedly, was because we were relying on a non-standard class, which isn't recommended). On 06/17/2013 08:12 AM, Peter Levart wrote: New API could be entirely unrelated to Throwable, if there was support for it in native code. Since there would have to be changes to the native code anyway to support this, why not create a separate API? If I understand this correctly, one part of your code is displaying the resource location of each method's declaring class in the stack trace when such extended stack trace is configured. Therefore you need this API to capture the class-enriched-stack-trace at the time the Throwable instance is constructed. No special Throwable-unrelated API to return the stack trace would be suitable for this purpose then. Yes, this is correct. This is the primary use case that needs satisfying. But for the purpose of displaying the resource location of the class invoking the logging API, you don't need the entire stack trace, just the immediate caller class, right? When @CallerSensitive internal annotation was introduced (and Reflection.getCallerClass(int frames) was dropped), there was some debate about possible introduction of public API that could be used to reveal the caller's class. Perhaps there's still time to create such API for JDK8? At least one part of your logging functionality would be covered with such API. I would love for this to be part of the public API, but since it's annotation-based we wouldn't be able to use it for probably six or seven years. For the foreseeable future Log4j must compile on Java 6. Suppose the API to obtain the immediate caller class (j.l.Class object) was public and fast, so you could do that for each call to the logging API eagerly (after initial check for logging level). Now, could you delay the evaluation of extended stack trace pattern to the latest time possible, when you must format the stack-trace? Would calling Class.forName(name, false, classLoader) for each individual StackTraceElement, using the ClassLoader of the caller class, still present a performance problem? Yes. Our _primary_ concern here is locating the source information for Throwable stack traces. (In fact, if you want, just forget about the source info for the calling class. That's easy. Let's focus JUST on Throwables.) Class.forName() is orders of magnitude slower that SecurityManager#getClassContext() and Reflection#getCallerClass(). And that's not even considering the fact that getClassContext() and getCallerClass() can only fill in _part_ of the stack trace (because they don't line up; one is from Throwable, the other is from current stack). If we could actually obtain the entire stack trace's source info without Class.forName(), it would be even faster. That's why I thought the _simplest_ approach would be to add the getElementClass() method to StackTraceElement ... it works everywhere. If the proposed solution is a StackFrame class instead, that's fine by me. I just need to be able to get the StackFrame[] for a Throwable. Nick
Re: Thoughts on adding getElementClass() method to StackTraceElement?
On Jun 16, 2013, at 1:25 AM, Jeroen Frijters wrote:
Nick Williams wrote:
I'm going to stick by my original assessment that I'm not convinced
there's a security issue. It's possible that getClassContext() filters
out classes the caller can't access, but nothing in the documentation
indicates that's the case, so I'm operating under the assumption that it
doesn't.
SecurityManager.getClassContext() is not available to unpriviliged callers,
so I don't think this a valid argument.
Can you define what is not available to unprivileged callers means a little
better? We use it in Log4j 2 when getCallerClass() isn't available, and it
works just fine. I took a look at the Java code, and it's native, so there's no
check there. (You can't instantiate a SecurityManager unless there's not
currently a SecurityManager installed _OR_ the installed SecurityManager allows
the creation (installation privilege isn't checked) of another SecurityManager.
Is that what you're talking about?) I took a look at the native code, and I
don't see anything there that would represent a privilege check that I
recognized, but if I understood what you meant a little better I might see
where I'm wrong.
Given the security implications, the serialization issue and the need for
weak references, it seems to me that adding this to Throwable would be way
too expensive.
The only expense I'm seeing here is the security implications, and I'm still
not convinced that they exist. We have all agreed now that making this
transient is okay, and adding the transient keyword to a field only takes 5
seconds. And I certainly don't believe that declaring the field as
WeakReferenceClass? elementClass instead of Class? elementClass takes
more than 30 seconds longer, but maybe I'm missing something, so that's not
expensive either.
Also, to be clear, I'm not suggesting adding it to Throwable specifically, I'm
suggesting adding it to StackTraceElement (which would apply anywhere that a
stack trace is filled in, including Throwable). A stack trace is filled in with
one native code method, so updating that method will make this work in
Throwable, Thread, and anywhere else that you can get a stack trace. I know
this is a nitpicky distinction, but it could be important.
Adding a new API to collect a stack trace seems like a far better approach.
Here's my off the cuff proposal:
public final class StackFrame {
public Executable method();
public String getFileName();
public int getLineNumber();
public static StackFrame[] capture(int skipFrames, int maxLength, boolean
includeSourceInfo);
}
I'm not opposed to the StackFrame class as suggested, it just doesn't meet my
needs. At all. I can already successfully get this information using the stack
trace and getClassContext(), so I wouldn't be compelled to switch. I need to
efficiently get the Class from a stack trace generated by a Throwable, which
this does not solve. Right now I'm having to resort to getting that Class using
inefficient methods.
Nick
Re: Thoughts on adding getElementClass() method to StackTraceElement?
On 16/06/2013 15:29, Nick Williams wrote: On Jun 16, 2013, at 1:25 AM, Jeroen Frijters wrote: : SecurityManager.getClassContext() is not available to unpriviliged callers, so I don't think this a valid argument. Can you define what is not available to unprivileged callers means a little better? We use it in Log4j 2 when getCallerClass() isn't available, and it works just fine. I took a look at the Java code, and it's native, so there's no check there. (You can't instantiate a SecurityManager unless there's not currently a SecurityManager installed _OR_ the installed SecurityManager allows the creation (installation privilege isn't checked) of another SecurityManager. Is that what you're talking about?) I took a look at the native code, and I don't see anything there that would represent a privilege check that I recognized, but if I understood what you meant a little better I might see where I'm wrong. The main thing is that the hack to extend SecurityManager (so that you can call the protected getClassContext method) is unlikely to work when there is security manager set. I can't tell what environments you have tested with but usually it means setting up the security policy so that the code base of your library is granted the permission. Even then you might still have an issue because there will likely be frames associated with other untrusted code on the stack. Maybe your code attempts to create the security manager does do with so privileges enabled, therwise everything that could potentially call you would also needed to be granted this permission. -Alan.
Re: Thoughts on adding getElementClass() method to StackTraceElement?
On Jun 16, 2013, at 9:29 AM, Nick Williams wrote:
On Jun 16, 2013, at 1:25 AM, Jeroen Frijters wrote:
Nick Williams wrote:
I'm going to stick by my original assessment that I'm not convinced
there's a security issue. It's possible that getClassContext() filters
out classes the caller can't access, but nothing in the documentation
indicates that's the case, so I'm operating under the assumption that it
doesn't.
SecurityManager.getClassContext() is not available to unpriviliged callers,
so I don't think this a valid argument.
Can you define what is not available to unprivileged callers means a little
better? We use it in Log4j 2 when getCallerClass() isn't available, and it
works just fine. I took a look at the Java code, and it's native, so there's
no check there. (You can't instantiate a SecurityManager unless there's not
currently a SecurityManager installed _OR_ the installed SecurityManager
allows the creation (installation privilege isn't checked) of another
SecurityManager. Is that what you're talking about?) I took a look at the
native code, and I don't see anything there that would represent a privilege
check that I recognized, but if I understood what you meant a little better I
might see where I'm wrong.
Given the security implications, the serialization issue and the need for
weak references, it seems to me that adding this to Throwable would be way
too expensive.
The only expense I'm seeing here is the security implications, and I'm still
not convinced that they exist. We have all agreed now that making this
transient is okay, and adding the transient keyword to a field only takes 5
seconds. And I certainly don't believe that declaring the field as
WeakReferenceClass? elementClass instead of Class? elementClass
takes more than 30 seconds longer, but maybe I'm missing something, so that's
not expensive either.
Also, to be clear, I'm not suggesting adding it to Throwable specifically,
I'm suggesting adding it to StackTraceElement (which would apply anywhere
that a stack trace is filled in, including Throwable). A stack trace is
filled in with one native code method, so updating that method will make this
work in Throwable, Thread, and anywhere else that you can get a stack trace.
I know this is a nitpicky distinction, but it could be important.
Adding a new API to collect a stack trace seems like a far better approach.
Here's my off the cuff proposal:
public final class StackFrame {
public Executable method();
public String getFileName();
public int getLineNumber();
public static StackFrame[] capture(int skipFrames, int maxLength, boolean
includeSourceInfo);
}
I'm not opposed to the StackFrame class as suggested, it just doesn't meet my
needs. At all. I can already successfully get this information using the
stack trace and getClassContext(), so I wouldn't be compelled to switch. I
need to efficiently get the Class from a stack trace generated by a
Throwable, which this does not solve. Right now I'm having to resort to
getting that Class using inefficient methods.
What if we also added a getStackFrames() method to Throwable? That would meet
my needs but it would also satisfy what I'm observing is a desire to have a new
API for this (StackFrame) instead of adding it to StackTraceElement. I'm very
open to how it's implemented, as long as it satisfies my use case. :-)
The stack trace of a Throwable can be filled in on demand when
getStackTrace() is called the first time, so that the overhead isn't incurred
when creating and throwing the exception. Presumably, we would need to do
something similar with getStackFrames(), especially since calling it would be
less common.
Thoughts on this?
Thoughts on adding getElementClass() method to StackTraceElement?
Bug 4851444 [1] was filed just over 10 years ago and has languished without so much as a comment ever since. I hate to revive something so old, but I'd like to see what everyone thinks. I'm working on the Apache project Log4j2. Our problem is we need to get the Class object that belongs to a particular element on the stack trace. We use three different methods, failing over from one to the next, in order to get the Class object. 1. If sun.reflect.Reflection#getCallerClass(int) exists, we repeatedly call it--increasing the depth each time--to fill up a java.util.Stack, then compare it to the exception stack trace and match as many of the elements as we can to Classes returned by getCallerClass. This is surprisingly fast, but some elements don't match and we have to fail over to 3. 2. If sun.reflect.Reflection.getCallerClass(int) does not exist, we create a SecurityManager and call getClassContext(), use it to fill up a java.util.Stack, and do the same thing we would normally do with 1. This is about two times slower than 1--not terrible. 3. If the element in a stack trace doesn't match a class found in methods 1 or 2, we load the class with the ClassLoader using the class name in the StackTraceElement. This is more than TEN TIMES SLOWER than 1--terrible. As you can see, it greatly benefits us to have some kind of efficient mechanism to match a StackTraceElement to a Class, so that's why we resort to using a non-public API (with the understanding that we must have backup options since this class will not always be present). Where I came in is when the change was made in 1.8.0-ea-b87 to remove the parameter from sun.reflect.Reflection#getCallerClass(). Now getCallerClass() can ONLY be used to retrieve the immediate caller, not callers at various depth, making method 1 unusable in Java 8. This led me to look for an alternative. Looking at Throwable.java and Throwable.c, getStackTrace() ultimately calls JVM_GetStackTraceElement. Looking at jvm.c from Java 6 (I can't find JVM_GetStackTraceElement in newer versions of Java), JVM_GetStackTraceElement calls CVMgetStackTraceElement. CVMgetStackTraceElement, in turn, then gets a copy of the native version of the Class object and uses it to populate the StackTraceElement's data. This leads me to believe that the task of adding a getElementClass() method to StackTraceElement would be as simple as: 1) Adding a new constructor to StackTraceElement to preserve backwards compatibility, with the Class being null by default. 2) Add the getStackTraceElement() method and backing private field. 3) Add a CVMID_fieldWriteRef call to set the class to the backing private field. I'm sure there are some idiosyncrasies I'm missing here, but overall this seems like a pretty trivial change. What do people think? Can this be worked in? .NET has had this ability since .NET 1.1 with the StackFrame class [2] and the StackTrace [3], which contains StackFrames. It provides more than just the Class (Type in .NET), it also provides the Method (MethodBase in .NET). It would greatly improve the efficiency of certain tasks not only in Log4j, but also in many other projects that I have found using getCallerClass() for the same purpose. [1] http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=4851444 [2] http://msdn.microsoft.com/en-us/library/system.diagnostics.stackframe.aspx [3] http://msdn.microsoft.com/en-us/library/system.diagnostics.stacktrace.aspx
Re: Thoughts on adding getElementClass() method to StackTraceElement?
On 06/15/2013 08:33 PM, Nick Williams wrote: Bug 4851444 [1] was filed just over 10 years ago and has languished without so much as a comment ever since. I hate to revive something so old, but I'd like to see what everyone thinks. Hi Nick, first 4851444 should be closed in my opinion, given that 1.6 introduces the ServiceLoader class which is the way to load implementation of a service. JAXP and JAXB now use the ServiceLoader. Could you be a little more clear why you need a class for your logging API, exactly, why the class name is not enough for logging purpose ? And about adding a way to store the class in the StackTraceElement, one issue is that stack trace element are Serializable, so if an error occurs on a server, the exception can be serialized to the client which is a really nice feature, but if a stack trace element embed the class object, then it means that all the class that are available at server side should also be available at client side. I have no idea how Microsoft solve this issue. cheers, Rémi I'm working on the Apache project Log4j2. Our problem is we need to get the Class object that belongs to a particular element on the stack trace. We use three different methods, failing over from one to the next, in order to get the Class object. 1. If sun.reflect.Reflection#getCallerClass(int) exists, we repeatedly call it--increasing the depth each time--to fill up a java.util.Stack, then compare it to the exception stack trace and match as many of the elements as we can to Classes returned by getCallerClass. This is surprisingly fast, but some elements don't match and we have to fail over to 3. 2. If sun.reflect.Reflection.getCallerClass(int) does not exist, we create a SecurityManager and call getClassContext(), use it to fill up a java.util.Stack, and do the same thing we would normally do with 1. This is about two times slower than 1--not terrible. 3. If the element in a stack trace doesn't match a class found in methods 1 or 2, we load the class with the ClassLoader using the class name in the StackTraceElement. This is more than TEN TIMES SLOWER than 1--terrible. As you can see, it greatly benefits us to have some kind of efficient mechanism to match a StackTraceElement to a Class, so that's why we resort to using a non-public API (with the understanding that we must have backup options since this class will not always be present). Where I came in is when the change was made in 1.8.0-ea-b87 to remove the parameter from sun.reflect.Reflection#getCallerClass(). Now getCallerClass() can ONLY be used to retrieve the immediate caller, not callers at various depth, making method 1 unusable in Java 8. This led me to look for an alternative. Looking at Throwable.java and Throwable.c, getStackTrace() ultimately calls JVM_GetStackTraceElement. Looking at jvm.c from Java 6 (I can't find JVM_GetStackTraceElement in newer versions of Java), JVM_GetStackTraceElement calls CVMgetStackTraceElement. CVMgetStackTraceElement, in turn, then gets a copy of the native version of the Class object and uses it to populate the StackTraceElement's data. This leads me to believe that the task of adding a getElementClass() method to StackTraceElement would be as simple as: 1) Adding a new constructor to StackTraceElement to preserve backwards compatibility, with the Class being null by default. 2) Add the getStackTraceElement() method and backing private field. 3) Add a CVMID_fieldWriteRef call to set the class to the backing private field. I'm sure there are some idiosyncrasies I'm missing here, but overall this seems like a pretty trivial change. What do people think? Can this be worked in? .NET has had this ability since .NET 1.1 with the StackFrame class [2] and the StackTrace [3], which contains StackFrames. It provides more than just the Class (Type in .NET), it also provides the Method (MethodBase in .NET). It would greatly improve the efficiency of certain tasks not only in Log4j, but also in many other projects that I have found using getCallerClass() for the same purpose. [1] http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=4851444 [2] http://msdn.microsoft.com/en-us/library/system.diagnostics.stackframe.aspx [3] http://msdn.microsoft.com/en-us/library/system.diagnostics.stacktrace.aspx
Re: Thoughts on adding getElementClass() method to StackTraceElement?
On Jun 15, 2013, at 2:22 PM, Remi Forax wrote: On 06/15/2013 08:33 PM, Nick Williams wrote: Bug 4851444 [1] was filed just over 10 years ago and has languished without so much as a comment ever since. I hate to revive something so old, but I'd like to see what everyone thinks. Hi Nick, first 4851444 should be closed in my opinion, given that 1.6 introduces the ServiceLoader class which is the way to load implementation of a service. JAXP and JAXB now use the ServiceLoader. There are really two parts to this bug: what the user needed to accomplish (which ServiceLoader accomplishes), and the suggestion for exposing getCallerClass() (which has broader implications than just his particular use-case). Could you be a little more clear why you need a class for your logging API, exactly, why the class name is not enough for logging purpose ? Certainly. Log4j 2 offers extended stack trace patterns. When exceptions or stack traces are logged, or when displaying the location that the logging statement originated from, the stack trace/log also reveals the resource (JAR file, directory, etc.) that the class was loaded from and the implementation version from the package the class is in. This helps reveal possible class loading issues (among other things), which can be an extremely valuable tool when troubleshooting problems with an application. Log4j 2 accomplishes this by getting the class associated with a StackTraceElement (as outlined below), and then 1) getting the Package from the Class and the version from the Package, and 2) getting the ProtectionDomain from the Class, getting the CodeSource from the ProtectionDomain, and getting the JAR/URL from the CodeSource. The last two parts are easy. It's getting the Class from the StackTrace that is extremely inefficient if not done right. Java 8 took away the best way we had found to do that (which, admittedly, was because we were relying on a non-standard class, which isn't recommended). And about adding a way to store the class in the StackTraceElement, one issue is that stack trace element are Serializable, so if an error occurs on a server, the exception can be serialized to the client which is a really nice feature, but if a stack trace element embed the class object, then it means that all the class that are available at server side should also be available at client side. I have no idea how Microsoft solve this issue. Classes are Serializable in Java, so it shouldn't cause a problem with serialization. The elementClass property will be properly serialized and deserialized. If I understand you correctly, you're expressing concern that this could actually be a problem; that we don't want Classes transferred to the client and deserialized. The Class class has a special serialized form that is unlike other classes and is very efficient. Only a descriptor for the class (including its serialVersionUID value) is transmitted, just enough to give the endpoint enough information to locate the class. There won't be a performance issue here, as far as I can tell. However, I can foresee one potential problem when deserializing the StackTraceElement: if the Class isn't on the class path on the deserializing JVM. This could be handled two possible ways: 1) Mark the elementClass property on StackTraceElement transient so that it isn't serialized. 2) Add a special deserialization rule (not unprecedented by any means) for StackTraceElement that says that StackTraceElement will not fail to deserialize if the elementClass property can't be deserialized because it couldn't be loaded. (This would be my suggestion.) I'm not sure how .NET handles it either, but it clearly does, as its StackFrame class is the equivalent of Serializable. Nick cheers, Rémi I'm working on the Apache project Log4j2. Our problem is we need to get the Class object that belongs to a particular element on the stack trace. We use three different methods, failing over from one to the next, in order to get the Class object. 1. If sun.reflect.Reflection#getCallerClass(int) exists, we repeatedly call it--increasing the depth each time--to fill up a java.util.Stack, then compare it to the exception stack trace and match as many of the elements as we can to Classes returned by getCallerClass. This is surprisingly fast, but some elements don't match and we have to fail over to 3. 2. If sun.reflect.Reflection.getCallerClass(int) does not exist, we create a SecurityManager and call getClassContext(), use it to fill up a java.util.Stack, and do the same thing we would normally do with 1. This is about two times slower than 1--not terrible. 3. If the element in a stack trace doesn't match a class found in methods 1 or 2, we load the class with the ClassLoader using the class name in the StackTraceElement. This is more than TEN TIMES SLOWER than 1--terrible. As you can see, it greatly benefits us to have
Re: Thoughts on adding getElementClass() method to StackTraceElement?
On 15/06/2013 19:33, Nick Williams wrote: : Looking at Throwable.java and Throwable.c, getStackTrace() ultimately calls JVM_GetStackTraceElement. Looking at jvm.c from Java 6 (I can't find JVM_GetStackTraceElement in newer versions of Java), JVM_GetStackTraceElement calls CVMgetStackTraceElement. CVMgetStackTraceElement, in turn, then gets a copy of the native version of the Class object and uses it to populate the StackTraceElement's data. This leads me to believe that the task of adding a getElementClass() method to StackTraceElement would be as simple as: 1) Adding a new constructor to StackTraceElement to preserve backwards compatibility, with the Class being null by default. 2) Add the getStackTraceElement() method and backing private field. 3) Add a CVMID_fieldWriteRef call to set the class to the backing private field. I'm sure there are some idiosyncrasies I'm missing here, but overall this seems like a pretty trivial change. What do people think? Can this be worked in? .NET has had this ability since .NET 1.1 with the StackFrame class [2] and the StackTrace [3], which contains StackFrames. It provides more than just the Class (Type in .NET), it also provides the Method (MethodBase in .NET). It would greatly improve the efficiency of certain tasks not only in Log4j, but also in many other projects that I have found using getCallerClass() for the same purpose. Here's a discussion from last year on this topic: http://mail.openjdk.java.net/pipermail/core-libs-dev/2012-October/011665.html -Alan.
Re: Thoughts on adding getElementClass() method to StackTraceElement?
On Jun 15, 2013, at 3:21 PM, Alan Bateman wrote: On 15/06/2013 19:33, Nick Williams wrote: : Looking at Throwable.java and Throwable.c, getStackTrace() ultimately calls JVM_GetStackTraceElement. Looking at jvm.c from Java 6 (I can't find JVM_GetStackTraceElement in newer versions of Java), JVM_GetStackTraceElement calls CVMgetStackTraceElement. CVMgetStackTraceElement, in turn, then gets a copy of the native version of the Class object and uses it to populate the StackTraceElement's data. This leads me to believe that the task of adding a getElementClass() method to StackTraceElement would be as simple as: 1) Adding a new constructor to StackTraceElement to preserve backwards compatibility, with the Class being null by default. 2) Add the getStackTraceElement() method and backing private field. 3) Add a CVMID_fieldWriteRef call to set the class to the backing private field. I'm sure there are some idiosyncrasies I'm missing here, but overall this seems like a pretty trivial change. What do people think? Can this be worked in? .NET has had this ability since .NET 1.1 with the StackFrame class [2] and the StackTrace [3], which contains StackFrames. It provides more than just the Class (Type in .NET), it also provides the Method (MethodBase in .NET). It would greatly improve the efficiency of certain tasks not only in Log4j, but also in many other projects that I have found using getCallerClass() for the same purpose. Here's a discussion from last year on this topic: http://mail.openjdk.java.net/pipermail/core-libs-dev/2012-October/011665.html -Alan. Indeed. Looks like the discussion died somewhat after a while. No conclusion was ever made. Based on the discussion it the email, I would add some additional observations/behavior that I think should be considered: 1) The elementClass property (getElementClass method) should be the Class that matches StackTraceElement's className property. It should not be the type of the object that received the method invocation. Perhaps, then, it would be better to call it frameClass (getFrameClass), to make it more obvious that it's the class for the frame on the stack. 2) If elementClass is made to be the type of the object that received the method invocation, then an getMethod() method should also be added so that the Class that defined the method that was executed can be obtained. This might actually be the most versatile option. However, Method is not Serializable, so that must then be considered. 3) I'm okay with making the elementClass transient. I don't have a particular need for it to be serialized, and making it transient is the easiest way to solve some problems. 4) Vitaly's email said that StackFrame exposes the MethodBase (Method), but not the Type (Class). This is correct. I wasn't clear on this earlier. To get the frame Type you would use stackFrameInstance.GetMethod().DeclaringType. 5) I do think the elementClass (or whatever) property should be Weak. If a particular piece of code (foolishly) hangs on to a StackTraceElement instance, it could cause a ClassLoader memory leak, particularly in Java EE containers. Making it Weak is the safest approach. Existing code written for earlier versions of Java may hang on to StackTraceElements for whatever reason, and problems wouldn't begin to arise from that until the code was run on Java 8. 6) I don't think security is a major issue here. There's not a lot you can do with a Class instance without involving existing security checks. For example, you can inspect the Class's name, Package, CodeSource, etc., but you can't invoke methods, manipulate fields, or create an instance of the Class without security checks being performed. Let's consider three possible scenarios: A) A Class loaded by a parent class loader obtains a StackTraceElement containing Classes loaded by a child class loader. I don't see a major issue here. The obvious scenario is a library loaded by a Servlet container obtaining a Class loaded within one of the web application class loaders. It's much more likely that this code will be well behaved and already has some kind of power over the child class loader anyway. B) A Class loaded by a child class loader obtains a StackTraceElement containing Classes loaded by a sibling class loader. This would obviously be the more dangerous scenario (think: one web application accessing another web application's classes), but I don't think it's actually possible. I don't think a stack trace generated in one class loader can possibly make its way into code in a sibling class loader, and I know that an execution stack can't contain classes from two sibling class loaders (only from class loaders with parent/child relationships). C) A Class loaded by a child class loader obtains a StackTraceElement containing Classes loaded by a parent class loader. There's no issue here. Child class loaders can already access classes
Re: Thoughts on adding getElementClass() method to StackTraceElement?
On 06/15/2013 09:59 PM, Nick Williams wrote: And about adding a way to store the class in the StackTraceElement, one issue is that stack trace element are Serializable, so if an error occurs on a server, the exception can be serialized to the client which is a really nice feature, but if a stack trace element embed the class object, then it means that all the class that are available at server side should also be available at client side. I have no idea how Microsoft solve this issue. Classes are Serializable in Java, so it shouldn't cause a problem with serialization. The elementClass property will be properly serialized and deserialized. If I understand you correctly, you're expressing concern that this could actually be a problem; that we don't want Classes transferred to the client and deserialized. The Class class has a special serialized form that is unlike other classes and is very efficient. Only a descriptor for the class (including its serialVersionUID value) is transmitted, just enough to give the endpoint enough information to locate the class. That's true when Class objects are serialized with a purpose and intent that they will be deserialized in an environment where correct versions of those classes are visible by class initiating deserialization. Stack trace can contain references to classes that are not visible even by the class that constructed the Throwable. By exposing Class objects that would otherwise not be obtainable, there could be security issues. There won't be a performance issue here, as far as I can tell. However, I can foresee one potential problem when deserializing the StackTraceElement: if the Class isn't on the class path on the deserializing JVM. This could be handled two possible ways: 1) Mark the elementClass property on StackTraceElement transient so that it isn't serialized. 2) Add a special deserialization rule (not unprecedented by any means) for StackTraceElement that says that StackTraceElement will not fail to deserialize if the elementClass property can't be deserialized because it couldn't be loaded. (This would be my suggestion.) I'm not sure how .NET handles it either, but it clearly does, as its StackFrame class is the equivalent of Serializable. I think that option #1 is the only realistic option. Who cares about classes in the de-serialized stack-trace. The only thing needed is their names so that the stack-trace can be printed. Regards, Peter
Re: Thoughts on adding getElementClass() method to StackTraceElement?
On 06/15/2013 11:06 PM, Nick Williams wrote:
B) A Class loaded by a child class loader obtains a StackTraceElement
containing Classes loaded by a sibling class loader. This would obviously be
the more dangerous scenario (think: one web application accessing another web
application's classes), but I don't think it's actually possible. I don't think
a stack trace generated in one class loader can possibly make its way into code
in a sibling class loader, and I know that an execution stack can't contain
classes from two sibling class loaders (only from class loaders with
parent/child relationships).
Why not? Consider this hypothetical code:
Parent class loader:
public class A {
public static final A INSTANCE = new A();
final PropertyChangeSupport pcs = new PropertyChangeSupport(this);
public void addPropertyChangeListener(PropertyChangeListener
pcl) { pcs.addPropertyChangeListener(pcl); }
String property;
public void setProperty(String property) {
pcs.firePropertyChange(property, this.property,
this.property = property);
}
}
Child class loader 1:
static class B implements PropertyChangeListener {
B() {
A a = A.INSTANCE;
a.addPropertyChangeListener(this);
}
@Override
public void propertyChange(PropertyChangeEvent evt) {
StackTraceElement[] st = new Throwable().getStackTrace();
// can we see C.class in 'st' array?
}
}
Child class loader 2:
static class C {
void m() {
A a = A.INSTANCE;
a.setProperty(XYZ);
}
}
Regards, Peter
Re: Thoughts on adding getElementClass() method to StackTraceElement?
On Jun 15, 2013, at 5:05 PM, Peter Levart wrote:
On 06/15/2013 11:06 PM, Nick Williams wrote:
B) A Class loaded by a child class loader obtains a StackTraceElement
containing Classes loaded by a sibling class loader. This would obviously be
the more dangerous scenario (think: one web application accessing another
web application's classes), but I don't think it's actually possible. I
don't think a stack trace generated in one class loader can possibly make
its way into code in a sibling class loader, and I know that an execution
stack can't contain classes from two sibling class loaders (only from class
loaders with parent/child relationships).
Why not? Consider this hypothetical code:
Parent class loader:
public class A {
public static final A INSTANCE = new A();
final PropertyChangeSupport pcs = new PropertyChangeSupport(this);
public void addPropertyChangeListener(PropertyChangeListener pcl) {
pcs.addPropertyChangeListener(pcl); }
String property;
public void setProperty(String property) {
pcs.firePropertyChange(property, this.property, this.property =
property);
}
}
Child class loader 1:
static class B implements PropertyChangeListener {
B() {
A a = A.INSTANCE;
a.addPropertyChangeListener(this);
}
@Override
public void propertyChange(PropertyChangeEvent evt) {
StackTraceElement[] st = new Throwable().getStackTrace();
// can we see C.class in 'st' array?
}
}
Child class loader 2:
static class C {
void m() {
A a = A.INSTANCE;
a.setProperty(XYZ);
}
}
Regards, Peter
Huh. An interesting quandary. And definitely not desirable...
The first thing I would say is that the code you suggest isn't, in my opinion,
responsible code, and in fact is dangerous. I know some guys over on the
Tomcat mailing list that would possibly shriek in terror if they saw code like
this. If an application server (parent class loader) had a class like A that
allowed actions of one web application (class C) to trigger method calls in
another web application (class B), nightmarish things would happen. In
actuality, class A should have different contexts for different class loaders.
If you write code like this and allow it to be used across sibling class
loaders in this manner, you are asking for more trouble than you're going to
get merely by having a Class instance in the StackTraceElement.
In fact, DriverManager is a perfect example. If a web application loads a JDBC
driver from its WEB-INF/lib, that driver will immediately become visible to
other web applications, and it will cause a memory leak. There was a recent
discussion about this on the Tomcat mailing list, and the general tenor was
DriverManager is hopelessly broken, don't put drivers there, they can only
safely go in the container's class loader. I could argue that DriverManager
has already created the very security issue that you propose exists by added a
Class instance to the StackTraceElement, but that by itself doesn't make it OK
(broken window syndrome/broken windows theory).
Still, in the scenario you propose there could be bigger security consequences
than merely having an instance of Class in StackTraceElement. What if
setProperty() takes an Object instead of a String (class loader leak). MOST
IMPORTANTLY: What if class B extends SecurityManager, instantiates it (but
doesn't install it), and calls SecurityManager#getClassContext() from
B#propertyChange(). The getClassContext() method essential returns the stack
trace of Classes instead of StackTraceElements.
I'm going to stick by my original assessment that I'm not convinced there's a
security issue. It's possible that getClassContext() filters out classes the
caller can't access, but nothing in the documentation indicates that's the
case, so I'm operating under the assumption that it doesn't. If I'm right,
there's no harm in StackTraceElement also having a Class instance. But
understand getClassContext() does not fulfill my use case entirely. I need the
Classes in a stack trace created when an exception is thrown, too, and
getClassContext() only gives me those classes in my current executing stack.
Nick
