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

Unable to transform AccessibleObject class #150

Closed
johnlcox opened this issue Jul 1, 2016 · 19 comments
Closed

Unable to transform AccessibleObject class #150

johnlcox opened this issue Jul 1, 2016 · 19 comments
Assignees
Milestone

Comments

@johnlcox
Copy link

johnlcox commented Jul 1, 2016

I was hoping to be able to use byte-buddy to transform the setAccessible method of the AccessibleObject class to allow more fine grained access check permissions. Since the AccessibleObject is a standard Java class I know this is likely a little bit of a stretch, but I was hoping it would be possible.

It seems like I'm pretty close but I end up with a class redefinition failed: attempted to change the schema error. As far as I can tell I just have a class I'm delegating two with exactly one matching method, so I'm not sure why the JVM thinks I'm adding/removing a field.

I've put my delegate class in a separate jar and use the instrumentation API to append it to the bootstrap classloader, and prior to doing that I would get ClassNotFoundExceptions, so I don't think this most recent error is a classpath issue.

I've put together a very small sample project where I'm trying to replace the setAccessible method with a new one that just prints a string to the console. If it's possible to get this sample project working, then I should be able to translate that into the more complicated permissions checks case.

My sample project is at https://github.com/johnlcox/bytebuddy-accessibleobject

I guess my questions are, am I doing something wrong? Is this simply not possible with current JVMs? Could it possibly be a bug?

Below is the error stack trace I am getting:

Exception in thread "main" java.lang.IllegalStateException: Could not install class file transformer
    at net.bytebuddy.agent.builder.AgentBuilder$InstallationStrategy$Default$1.onError(AgentBuilder.java:2586)
    at net.bytebuddy.agent.builder.AgentBuilder$Default.installOn(AgentBuilder.java:5142)
    at net.bytebuddy.agent.builder.AgentBuilder$Default.installOnByteBuddyAgent(AgentBuilder.java:5156)
    at net.bytebuddy.agent.builder.AgentBuilder$Default$Delegator.installOnByteBuddyAgent(AgentBuilder.java:6770)
    at com.leacox.example.bytebuddy.Main.instrument(Main.java:69)
    at com.leacox.example.bytebuddy.Main.main(Main.java:24)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:498)
    at com.intellij.rt.execution.application.AppMain.main(AppMain.java:144)
Caused by: java.lang.UnsupportedOperationException: class redefinition failed: attempted to change the schema (add/remove fields)
    at sun.instrument.InstrumentationImpl.retransformClasses0(Native Method)
    at sun.instrument.InstrumentationImpl.retransformClasses(InstrumentationImpl.java:144)
    at net.bytebuddy.agent.builder.AgentBuilder$RedefinitionStrategy$Collector$ForRetransformation$Cumulative.apply(AgentBuilder.java:3107)
    at net.bytebuddy.agent.builder.AgentBuilder$Default.installOn(AgentBuilder.java:5138)
    ... 9 more
@raphw raphw added the question label Jul 1, 2016
@raphw raphw self-assigned this Jul 1, 2016
@raphw raphw added this to the 1.4.6 milestone Jul 1, 2016
@raphw
Copy link
Owner

raphw commented Jul 1, 2016

You actually delegate to the TypePool.Resolution instance as you do not call resolve after reading the class from the TypePool. (The class might not exist.) As a consequence, you do not delegate to a class but to an object which requires Byte Buddy to add a field for this object in the instrumented class. This additional field causes the rejection of the transformation.

@raphw raphw closed this as completed Jul 1, 2016
@johnlcox
Copy link
Author

johnlcox commented Jul 1, 2016

When I call resolve I get an IllegalStateException saying it can't find the type description. How would I properly create a TypePool for the bootstrap class loader that includes classes from my appended jar?

I have tried:

  • TypePool.Default.of(Main.class.getClassLoader()) - I thought this would get the main class loader which would delegate up to system and bootstrap.
  • TypePool.Default.of(ClassFileLocator.ForClassLoader.of(null)) - The javadoc indicates this will use the system loader which I also thought would delegate up to the bootstrap loader.

@raphw
Copy link
Owner

raphw commented Jul 1, 2016

There is a bug in the VM where classes on the bootstrap class path cannot be located as a class file due to some caching issue such that Byte Buddy (or any library) cannot locate it.

You can work around this by resolving the jar file explicitly:

TypePool typePool = TypePool.Default.of(new ClassFileLocator.Compound(
    new ClassFileLocator.ForJarFile(new JarFile(file)),
    ClassFileLocator.ForClassLoader.of(Main.class.getClassLoader())));

The file in this context is your reference to the jar added to the bootstrap class path.

Note that you need to align your matcher accordingly

.method(named("setAccessible").and(takesArguments(boolean.class)))

to avoid instrumenting the internal implementation.

@raphw
Copy link
Owner

raphw commented Jul 1, 2016

By the way, after adfing a class to the bootratrap class path, you can simply refer to the loaded version, no need to use the type pool.

@johnlcox
Copy link
Author

johnlcox commented Jul 1, 2016

Awesome. Using the loaded version is working now. I was still having an issue using the type pool as you had shown earlier. Thank you so much for your help.

I'm running into some reflection problems now, so my ultimate goal still may not be possible, but I'm glad I was able to get the byte-buddy parts working with your help.

Edit: Actually I think I should be good to go. I had forgotten how simple I had made my sample implementation. I believe I can get things working with my full implementation.

@johnlcox
Copy link
Author

johnlcox commented Jul 1, 2016

Is it possible for me to use the @This annotation in my delegate class and put the delegate's jar into the bootstrap loader?

@johnlcox
Copy link
Author

johnlcox commented Jul 1, 2016

Looks like I can use the @This annotation as long as I make sure byte-buddy is on the bootstrap classpath and that I don't load any byte-buddy classes elsewhere until I've updated the bootstrap classpath.

Would it be possible in a future release to have a separate annotations module so that not all of byte-buddy needs to be included to use the annotations? If this is something you are open to, I could probably do a pull request.

@raphw
Copy link
Owner

raphw commented Jul 2, 2016

If you want to use annotations, I would recommend you to use the type pool again as suggested before.

What is your problem with it? I had your example working with the suggestion above.

@johnlcox
Copy link
Author

johnlcox commented Jul 2, 2016

I've got it working now with the type pool and with the @This annotation. I had only tried the type pool briefly yesterday, so I'm not sure what I was doing it wrong, but everything seems to be working great today.

If I can get more fine grained access check permissions working by interjecting custom setAccessible implementations I hope to write up a blog post about how it can be done using byte-buddy.

Thanks for your help.

@raphw
Copy link
Owner

raphw commented Jul 2, 2016

You are welcome. Maybe, you would also be interested in using the Advice class for your purposes. It allows you to add code rather than replacing it. Also, since it is adding the code rather than delegating, you can skip the bootstrap injection.

Let me know of any write-up!

@johnlcox
Copy link
Author

johnlcox commented Jul 2, 2016

For the security checks I think it can only be done through interjection. My plan is to replace to the setAccessible method to check the classloader of the object that setAccessible is being called on; if the classloade isr a custom sandboxed classloader that is also the same instance as the classloader of the caller, then it will completely replace the default logic to use a new custom permission instead of ReflectPermission("suppressAccessChecks") whereas if the classloader is any other classloader or a different instance, then it will fallback to the standard implementation.

Unless Advice allows conditionally not continuing to the original implementation, I think I need to replace the whole method.

@raphw
Copy link
Owner

raphw commented Jul 2, 2016

Currently, Advice does not allow skipping the original code, even-though I consider adding this feature jn a future version.

For invoking the setAccessible0 method, check out the MethodCall implementation.

@johnlcox
Copy link
Author

johnlcox commented Jul 2, 2016

MethodCall sounds like what I need. I'm not totally clear on how to use it. I tried transforming my SetAccessibleImplementation to use it like below, but I'm getting an error that it cannot see the private method. I have pushed this to my sample project as well.

class SetAccessibleImplementation {
private static void setAccessible1(AccessibleObject ao, boolean flag){

  }
}
String setAccessible0MethodName = "setAccessible0";
    Class[] paramTypes = new Class[2];
    paramTypes[0] = AccessibleObject.class;
    paramTypes[1] = boolean.class;
    Method setAccessible0Method = AccessibleObject.class.getDeclaredMethod(setAccessible0MethodName, paramTypes);

// Previous code omitted

.type(named("com.leacox.example.bytebuddy.instrument.java.lang.reflect.SetAccessibleImplementation"))
        .transform(new AgentBuilder.Transformer() {
          @Override
          public DynamicType.Builder<?> transform(
              DynamicType.Builder<?> builder, TypeDescription typeDescription,
              ClassLoader classLoader) {
            return builder.method(named("setAccessible1"))
                .intercept(
                    MethodCall.invoke(new MethodDescription.ForLoadedMethod(setAccessible0Method)).withAllArguments());
          }
        })

@johnlcox
Copy link
Author

johnlcox commented Jul 2, 2016

Looking through a lot of the tests it looks like MethodCall can work for package private visibility but not fully private visibility. I also attempted to use FieldProxy and SuperCall but both of those can't be used for re-transformations and it's basically guaranteed that the AccessibleObject class will be loaded long before the transformation happens.

The override field on the AccessibleObject is package private, so in the meantime I can put my intercepting class in the java.lang.reflect package and directly set AccessibleObject.override. This isn't great though since the implementation of setAccessible0 may change in the future if a security bug is found in the JVM and it would be nice to get security fixes automatically by depending on that method. I realize that method could go away entirely or be renamed too, but at least then I'll some tests that will fail rather than an unknown missing security fix.

Hopefully I'm just missing something about how you are thinking I should use MethodCall because getting that to work does seem like the ideal solution.

@raphw raphw reopened this Jul 4, 2016
@raphw
Copy link
Owner

raphw commented Jul 4, 2016

I thought more about chaining the calls. I assume that you are throwing an exception in your real interceptor if the security check fails? In the case, the following would work:

builder.method(named("setAccessible").and(takesArguments(boolean.class)))
.intercept(MethodDelegation.to(typePool.describe(
"com.leacox.example.bytebuddy.instrument.java.lang.reflect.SetAccessibleImplementation"
).resolve()).andThen(MethodCall.invoke(setAccessible0Method).withThis.withAllArguments()
));

The setAccessible0Method is only called if the interceptor does not throw an exception. If the method disappeared, this would already thrown an exception if the method is attempted to be located. (Byte Buddy would have thrown an exception if the field did not exist as well).

That said, I am looking into adding a skip instruction to the Advice component in the future to make this even easier.

@johnlcox
Copy link
Author

johnlcox commented Jul 4, 2016

Your assumption is correct; my interceptor implementation will throw a SecurityException if the security check fails. Chaining the calls as in your example does work. This didn't occur to me because I didn't realize that an exception would short-circuit the chaining; is this documented somewhere or can you point me to some lines of code where I can see this?

@raphw
Copy link
Owner

raphw commented Jul 4, 2016

Well, the documentation does indeed not make this explicit but Byte Buddy generates standard Java code. If an exception is thrown, unless you do not explicitly catch the exception from an interceptor, the chained byte code is never reached.

@johnlcox
Copy link
Author

johnlcox commented Jul 4, 2016

Got it. Thanks for all your help.

@raphw
Copy link
Owner

raphw commented Jul 4, 2016

No problem and thanks for the idea of creating a property to skip the original method.

@raphw raphw closed this as completed Jul 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants