Thoughts on adding getElementClass() method to StackTraceElement?

[Nick Williams]

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