Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correct class loader lost in ClassWriter.getCommonSuperClass(String,String) when using TransformingClassLoader #117

Open
mrswadge opened this issue Jan 10, 2018 · 3 comments

Comments

@mrswadge
Copy link

We are transforming a class structure from an old format to a new one. We make use of the TransformingClassLoader to do this. We recently moved to Java 8 from Java 6, so we had to update our version of cglib from 2.2.1 to a more recent version, 3.2.5.

The code (which was previously working) has stopped working. It looks like the culprit is the class loader in the ClassWriter class (from ASM) is finding class loader which loaded the TransformingClassLoader and not the one which we passed into the constructor. The class loader which loaded TransformingClassLoader is unable to see this class and so we get a java.lang.ClassNotFoundException.

In our case the class loader we pass into the TransformingClassLoader has the class loader that loaded the TransformingClassLoader as it's parent, but this does not help us.

[15][Tue Nov 28 12:48:27 GMT 2017] java.lang.RuntimeException: java.lang.ClassNotFoundException: com.company.base.app.validation.TwinsValidationException
[15][Tue Nov 28 12:48:27 GMT 2017] 	at net.sf.cglib.asm.$ClassWriter.getCommonSuperClass(Unknown Source)
[15][Tue Nov 28 12:48:27 GMT 2017] 	at net.sf.cglib.asm.$ClassWriter.a(Unknown Source)
[15][Tue Nov 28 12:48:27 GMT 2017] 	at net.sf.cglib.asm.$Frame.a(Unknown Source)
[15][Tue Nov 28 12:48:27 GMT 2017] 	at net.sf.cglib.asm.$Frame.a(Unknown Source)
[15][Tue Nov 28 12:48:27 GMT 2017] 	at net.sf.cglib.asm.$MethodWriter.visitMaxs(Unknown Source)
[15][Tue Nov 28 12:48:27 GMT 2017] 	at net.sf.cglib.core.CodeEmitter.visitMaxs(CodeEmitter.java:842)
[15][Tue Nov 28 12:48:27 GMT 2017] 	at net.sf.cglib.asm.$ClassReader.a(Unknown Source)
[15][Tue Nov 28 12:48:27 GMT 2017] 	at net.sf.cglib.asm.$ClassReader.b(Unknown Source)
[15][Tue Nov 28 12:48:27 GMT 2017] 	at net.sf.cglib.asm.$ClassReader.accept(Unknown Source)
[15][Tue Nov 28 12:48:27 GMT 2017] 	at net.sf.cglib.transform.ClassReaderGenerator.generateClass(ClassReaderGenerator.java:39)
[15][Tue Nov 28 12:48:27 GMT 2017] 	at net.sf.cglib.transform.TransformingClassGenerator.generateClass(TransformingClassGenerator.java:33)
[15][Tue Nov 28 12:48:27 GMT 2017] 	at net.sf.cglib.transform.AbstractClassLoader.loadClass(AbstractClassLoader.java:90)
[15][Tue Nov 28 12:48:27 GMT 2017] 	at java.lang.Class.getDeclaredFields0(Native Method)
[15][Tue Nov 28 12:48:27 GMT 2017] 	at java.lang.Class.privateGetDeclaredFields(Class.java:2583)
[15][Tue Nov 28 12:48:27 GMT 2017] 	at java.lang.Class.getDeclaredFields(Class.java:1916)
[15][Tue Nov 28 12:48:27 GMT 2017] 	at java.io.ObjectStreamClass.getDefaultSerialFields(ObjectStreamClass.java:1783)
[15][Tue Nov 28 12:48:27 GMT 2017] 	at java.io.ObjectStreamClass.getSerialFields(ObjectStreamClass.java:1705)
[15][Tue Nov 28 12:48:27 GMT 2017] 	at java.io.ObjectStreamClass.access$800(ObjectStreamClass.java:79)
[15][Tue Nov 28 12:48:27 GMT 2017] 	at java.io.ObjectStreamClass$2.run(ObjectStreamClass.java:496)
[15][Tue Nov 28 12:48:27 GMT 2017] 	at java.io.ObjectStreamClass$2.run(ObjectStreamClass.java:482)
[15][Tue Nov 28 12:48:27 GMT 2017] 	at java.security.AccessController.doPrivileged(Native Method)
[15][Tue Nov 28 12:48:27 GMT 2017] 	at java.io.ObjectStreamClass.<init>(ObjectStreamClass.java:482)
[15][Tue Nov 28 12:48:27 GMT 2017] 	at java.io.ObjectStreamClass.lookup(ObjectStreamClass.java:379)
[15][Tue Nov 28 12:48:27 GMT 2017] 	at java.io.ObjectStreamClass.initNonProxy(ObjectStreamClass.java:669)
[15][Tue Nov 28 12:48:27 GMT 2017] 	at java.io.ObjectInputStream.readNonProxyDesc(ObjectInputStream.java:1880)
[15][Tue Nov 28 12:48:27 GMT 2017] 	at java.io.ObjectInputStream.readClassDesc(ObjectInputStream.java:1746)
[15][Tue Nov 28 12:48:27 GMT 2017] 	at java.io.ObjectInputStream.readOrdinaryObject(ObjectInputStream.java:2037)
[15][Tue Nov 28 12:48:27 GMT 2017] 	at java.io.ObjectInputStream.readObject0(ObjectInputStream.java:1568)
[15][Tue Nov 28 12:48:27 GMT 2017] 	at java.io.ObjectInputStream.readObject(ObjectInputStream.java:428)
[15][Tue Nov 28 12:48:27 GMT 2017] 	at com.company.tools.classinvalid.ClassInvalidFrig.internalReadObject(ClassInvalidFrig.java:635)
[15][Tue Nov 28 12:48:27 GMT 2017] 	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
[15][Tue Nov 28 12:48:27 GMT 2017] 	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
[15][Tue Nov 28 12:48:27 GMT 2017] 	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
[15][Tue Nov 28 12:48:27 GMT 2017] 	at java.lang.reflect.Method.invoke(Method.java:498)
[15][Tue Nov 28 12:48:27 GMT 2017] 	at com.company.tools.classinvalid.ClassInvalidFrig.readObject(ClassInvalidFrig.java:622)
[15][Tue Nov 28 12:48:27 GMT 2017] 	at com.company.tools.classinvalid.ClassInvalidFrig.getRows(ClassInvalidFrig.java:546)
[15][Tue Nov 28 12:48:27 GMT 2017] 	at com.company.tools.classinvalid.ClassInvalidFrig$3.call(ClassInvalidFrig.java:220)
[15][Tue Nov 28 12:48:27 GMT 2017] 	at com.company.tools.classinvalid.ClassInvalidFrig$3.call(ClassInvalidFrig.java:217)
[15][Tue Nov 28 12:48:27 GMT 2017] 	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
[15][Tue Nov 28 12:48:27 GMT 2017] 	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
[15][Tue Nov 28 12:48:27 GMT 2017] 	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
[15][Tue Nov 28 12:48:27 GMT 2017] 	at java.lang.Thread.run(Thread.java:748)

We pinned this down to the following line in ASM which is using the class loader which created the ClassWriter, but this is created by the parent class loader of the one we passed into the TransformingClassLoader.

https://gitlab.ow2.org/asm/asm/blob/master/asm/src/main/java/org/objectweb/asm/ClassWriter.java#L827

protected String getCommonSuperClass(final String type1, final String type2) {
  ClassLoader classLoader = getClass().getClassLoader();

i.e. the class loaders are in the following structure.
[A] Java Class Loader <- [B] Our Class Loader <- [C] TransformingClassLoader

The problem comes when ClassWriter finds class loader [A] instead of the expected class loader [B] due to the getClass().getClassLoader() call.

We found this is setup via the line below using the DebuggingClassWriter which creates the ClassWriter in it's constructor with no reference to the class loader we provided to the TransformingClassLoader.

DebuggingClassWriter w =
new DebuggingClassWriter(ClassWriter.COMPUTE_FRAMES);

To work around this we have had to extend ClassWriter with a custom version that allows us to pass in the correct class loader (so it doesn't use the implied one via getClass().getClassLoader()) and then to override the method AbstractClassLoader.loadClass(String) with a custom implementation that sets the

i.e.

import net.sf.cglib.asm.$ClassReader;
import net.sf.cglib.asm.$ClassWriter;

public class ClassLoaderClassWriter extends $ClassWriter {
  public ClassLoaderClassWriter( $ClassReader classReader, int flags ) {
    super( classReader, flags );
  }

  public ClassLoaderClassWriter( int flags ) {
    super( flags );
  }

  private ClassLoader classLoader;

  public void setClassLoader( ClassLoader classLoader ) {
    this.classLoader = classLoader;
  }
  
  @Override
  protected String getCommonSuperClass( String type1, String type2 ) {
    Class<?> c, d;

    try {
        c = Class.forName(type1.replace('/', '.'), false, classLoader);
        d = Class.forName(type2.replace('/', '.'), false, classLoader);
    } catch (Exception e) {
        throw new RuntimeException(e.toString());
    }
    if (c.isAssignableFrom(d)) {
        return type1;
    }
    if (d.isAssignableFrom(c)) {
        return type2;
    }
    if (c.isInterface() || d.isInterface()) {
        return "java/lang/Object";
    } else {
        do {
            c = c.getSuperclass();
        } while (!c.isAssignableFrom(d));
        return c.getName().replace('.', '/');
    }
  }
}

We then had to override TransformingClassLoader.loadClass(String) and use reflection to get to the ProtectionDomain.

@Override
public Class loadClass( String name ) throws ClassNotFoundException {
	Class loaded = findLoadedClass(name);

	if( loaded != null ){
		if( loaded.getClassLoader() == this ){
			return loaded;
		}//else reload with this class loader
	}

	if (!filter.accept(name)) {
		return super.loadClass(name);
	}
	
	$ClassReader r;
	try {
		java.io.InputStream is = jcl.getResourceAsStream( name.replace('.','/') + ".class" ); 

		if (is == null) {
			throw new ClassNotFoundException(name);
		}
		try { 
			r = new $ClassReader(is);
		} finally {
			is.close();
		}
	} catch (IOException e) {
		throw new ClassNotFoundException(name + ":" + e.getMessage());
	}

	try {
		ClassLoaderClassWriter  w = new ClassLoaderClassWriter ( $ClassWriter.COMPUTE_FRAMES );
		w.setClassLoader( jcl );

		getGenerator(r).generateClass(w);
		byte[] b = w.toByteArray();

		Field fieldDomain = AbstractClassLoader.class.getDeclaredField( "DOMAIN" );
		fieldDomain.setAccessible( true );
		ProtectionDomain domain = (ProtectionDomain) fieldDomain.get( this );

		Class c = super.defineClass(name, b, 0, b.length, domain);
		postProcess(c);
		return c;
	} catch (RuntimeException e) {
		throw e;
	} catch (Error e) {
		throw e;
	} catch (Exception e) {
		throw new CodeGenerationException(e);
	}						
}

Whilst we have a work around, we also feel it is a bug as the class loader [B] falls out of the picture during the transformation.

Cheers,
Stuart

@mrswadge
Copy link
Author

mrswadge commented Jan 10, 2018

I've created a test case to demonstrate the workaround and the issue.
It requires Maven to run.
cglib-issue-117.zip

@raphw
Copy link
Member

raphw commented Jan 11, 2018

This is probably not expected behavior, you are right but I am a bit scared to change that code, especially since you found a workaround. cglib was written in a time long before stack-map frames and use case around problems with them are therefore not really considered. cglib is used in a lot of applications and any change of implicit state can have rather far reaching consequences, unfortunately, and this particular change, I would need to analyze quite a bit before doing anything.

cglib is no longer under active development, please consider alternative code generation libraries if you rely on stack-map frame construction.

@Lerm
Copy link

Lerm commented Mar 12, 2018

This problem has a broader impact - it's not only confined to TransformingClassLoader/AbstractClassLoader, but is also applicable to any class generated with DebuggingClassWriter (i.e. Enhancer with DefaultGeneratorStrategy).

Cglib 3.2+ specifies COMPUTE_FRAMES during creation of ClassWriter object to enforce recalculation of stack map frames (which is probably the right thing to do), which causes invocation of ClassWriter.getCommonSuperClass method during incoming stack frames merging. Default ClassWriter implementation uses current class-loader (i.e. classloader which was used to load ClassWriter itself) to load classes of merging types - so, if actual classes are unavailable in this class-loader, then the code generation fails. This problem can be observed in any configuration with multi-level class loader configuration - one example is JEE environment in which cglib itself is located on EAR application level, but is used to instrument classes in children WAR-modules (which usually have their own classloaders under EAR to allow hot reloading of classes).

In order to fix this problem, we can override the 'getCommonSuperClass' method of the ClassWriter class and load classes using the same classloader which is used to define the resulting generated class (i.e. Enhancer.getClassLoader - which is either explicitly defined or derived from super-class/interfaces). I cannot think now of any case in which we may want NOT to use this classloader to calculate common super-class during stackmap calculation.

Implementation of possible fix (with unit tests for both Enhancer and TransformingClassLoader cases) is included into the following pull request: #118. The way of how class loader is injected into the DebuggingClassLoader in DefaultGeneratorStrategy is a little clumsy as I'd like to minimize changes to the interfaces. Unit tests uses special filtering class-loader to create complex class-loader configuration.

I'd like also to mention, that this problem should also affect most implementations of 'AbstractTransformTask' - Ant-tasks which use CGLIB for bulk processing of classes or jar files. In this case we don't have any meaningful classloader at all, so if classes which are processed by the task are not already somehow present in the classpath of current JVM, then transformation will fail on the same attempt to resolve common super-class. However, I see no easy fix here (and I'm unsure whether anybody is really using this functionality at all). cglib doesn't provide the end-user implementation of such Ant tasks, so it seems that every user of 'AbstractTransformTask' should review transformations performed and provide a way to override ClassWriter creation or implementation - maybe it's useful to extract creation of DebuggingClassLoader in AbstractTransformTask into a separate protected method to allow its customization in sub-classes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants