Fix bridge type erasure #89

GregBowyer · 2016-08-26T05:53:11Z

Ok so this is a fun one

Lets say you have a superclass, some subclass and an interface that are generically typed. The interface implicitly defines methods that are implemented in the superclass, but for some reason the interface is associated to the subclass.

Now, just for extra fun, lets have the superclass implement some generic type but have it bounded. Lets have the interface also define some type, that is valid for the bound in the superclass, but have it lack the type bounding.

This leads to something that looks a little like this.

static abstract class Superclass<T extends ErasedType> {
    public T aMethod(T t) { return null; }
    public RetType widenReturn(T t) { return null; }
}

public interface ParameterisedInterfaceNotTypeBounded<T> {
    T aMethod(T obj);
    ErasedType widenReturn(T obj);
}

public static class OtherImpl extends Superclass<Refined> 
    implements ParameterisedInterfaceNotTypeBounded<Refined> {
        public String irrelvantMethod() { return "Not important"; }
}

Notice that the only thing not correctly tying the type hierarchies together is the lack of bound in the interface generics declaration.

This has all the right ‟shape” for javac, and as such it complies this without error.
In the bytecode that is generated, we wind up with a bridge targeted at tightening the type bound from OtherImpl → Superclass such that, OtherImpl can satisfy type erasure from the interface, but still invoke the real implementation from the parent class Superclass.

This bridge has one of the following forms in the wild (it seems to be arbitrary as to where javac places the checkcast).

  public java.lang.Object aMethod(java.lang.Object);
    descriptor: (Ljava/lang/Object;)Ljava/lang/Object;
    flags: ACC_PUBLIC, ACC_BRIDGE, ACC_SYNTHETIC
    Code:
      stack=2, locals=2, args_size=2
         0: aload_0
         1: aload_1
         2: checkcast     #3                  // class net/sf/cglib/proxy/TestEnhancer$Refined
         5: invokespecial #7                  // Method net/sf/cglib/proxy/TestEnhancer$Superclass.aMethod:(Lnet/sf/cglib/proxy/TestEnhancer$ErasedType;)Lnet/sf/cglib/proxy/TestEnhancer$ErasedType;
         8: areturn
      LocalVariableTable:
        Start  Length  Slot  Name   Signature
            0       9     0  this   Lnet/sf/cglib/proxy/TestEnhancer$OtherImpl;
    MethodParameters:
      Name                           Flags
      t                              synthetic

Now, with the way cglib currently handles bridges that contain invokespecial we wind up basically invalidating this statement in Enhancer.java

TODO: this assumes that the target has wider or the same type
parameters than the current.
In reality this should always be true because otherwise we wouldn't
have had a bridge doing an invokespecial.
If it isn't true, we would need to checkcast each argument
against the target's argument types

If we generate a proxy for this, and fail to correctly implement the Object aMethod(Object) bridge we get a VerifyError since we dont honor the superclass correctly.

This pull request contains two things towards this. The first is a unit test testBridgedTypeErasure that tests this. The second is a (probably terrible) fast check to look for bridges that go Object → <T>, and in the presence of such bridges emit a pure invokespecial call rather than having the proxy patch up the bridge with invokevirtual.

This change is

sameb · 2016-08-26T23:08:50Z

Thanks for the excellent analysis and patch. I'll take a closer look next
week and see what we can do.

On Fri, Aug 26, 2016, 1:53 AM Greg Bowyer notifications@github.com wrote:

Ok so this is a fun one

Lets say you have a superclass, some subclass and an interface that are
generically typed. The interface implicitly defines methods that are
implemented in the superclass, but for some reason is defined only on the
subclass.

Now, just for extra fun, lets have the superclass implement some generic
type but have it bounded. Lets have the interface also define some type,
that is valid for the bound in the superclass, but have it lack the type
bounding.

This leads to something that looks a little like this.

static abstract class Superclass {
public T aMethod(T t) { return null; }
public RetType widenReturn(T t) { return null; }
}
public interface ParameterisedInterfaceNotTypeBounded {
T aMethod(T obj);
ErasedType widenReturn(T obj);
}
public static class OtherImpl extends Superclass
implements ParameterisedInterfaceNotTypeBounded {
public String irrelvantMethod() { return "Not important"; }
}

Notice that the only thing not correctly tying the type hierarchies
together is the lack of bound in the interface generics declaration.

This has all the right ‟shape” for javac, and as such it complies this
without error.
In the bytecode that is generated, we wind up with a bridge targeted at
tightening the type bound from OtherImpl → Superclass such that,
OtherImpl can satisfy type erasure from the interface, but still invoke
the real implementation from the parent class Superclass.

This bridge has one of the following forms in the wild (it seems to be
arbitrary as to where javac places the checkcast).

public java.lang.Object aMethod(java.lang.Object);
descriptor: (Ljava/lang/Object;)Ljava/lang/Object;
flags: ACC_PUBLIC, ACC_BRIDGE, ACC_SYNTHETIC
Code:
stack=2, locals=2, args_size=2
0: aload_0
1: aload_1
2: checkcast #3 // class net/sf/cglib/proxy/TestEnhancer$Refined
5: invokespecial #7 // Method net/sf/cglib/proxy/TestEnhancer$Superclass.aMethod:(Lnet/sf/cglib/proxy/TestEnhancer$ErasedType;)Lnet/sf/cglib/proxy/TestEnhancer$ErasedType;
8: areturn
LocalVariableTable:
Start Length Slot Name Signature
0 9 0 this Lnet/sf/cglib/proxy/TestEnhancer$OtherImpl;
MethodParameters:
Name Flags
t synthetic

Now, with the way cglib currently handles bridges that contain
invokespecial we wind up basically invalidating this statement in
Enhancer.java

TODO: this assumes that the target has wider or the same type
parameters than the current.

In reality this should always be true because otherwise we wouldn't
have had a bridge doing an invokespecial.
If it isn't true, we would need to checkcast each argument
against the target's argument types

If we generate a proxy for this, and fail to correctly implement the Object
aMethod(Object) bridge we get a VerifyError since we dont honor the
superclass correctly.

]his pull request contains two things towards this. The first is a unit
test testBridgedTypeErasure that tests this. The second is a (probably
terrible) fast check to look for bridges that go Object → , and in the
presence of such bridges emit a pure invokespecial call rather than

having the proxy patch up the bridge with invokevirtual.

You can view, comment on, or merge this pull request online at:

#89
Commit Summary

[NOCOMMIT] Change version for testing

Ghetto fix for bridge expansions

Unit test for odd bridge interfaces

Somehwat slightly less ghetto fix

File Changes

M cglib-integration-test/pom.xml
https://github.com/cglib/cglib/pull/89/files#diff-0 (2)

M cglib-jmh/pom.xml
https://github.com/cglib/cglib/pull/89/files#diff-1 (2)

M cglib-nodep/pom.xml
https://github.com/cglib/cglib/pull/89/files#diff-2 (2)

M cglib-sample/pom.xml
https://github.com/cglib/cglib/pull/89/files#diff-3 (2)

M cglib/pom.xml
https://github.com/cglib/cglib/pull/89/files#diff-4 (2)

M cglib/src/main/java/net/sf/cglib/proxy/BridgeMethodResolver.java
https://github.com/cglib/cglib/pull/89/files#diff-5 (9)

M cglib/src/main/java/net/sf/cglib/proxy/Enhancer.java
https://github.com/cglib/cglib/pull/89/files#diff-6 (29)

M cglib/src/test/java/net/sf/cglib/proxy/TestEnhancer.java
https://github.com/cglib/cglib/pull/89/files#diff-7 (64)

M pom.xml https://github.com/cglib/cglib/pull/89/files#diff-8
(2)

Patch Links:

https://github.com/cglib/cglib/pull/89.patch

https://github.com/cglib/cglib/pull/89.diff

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#89, or mute the thread
https://github.com/notifications/unsubscribe-auth/ACC7EvJTR51rKlFNy0fssaAKdF7lqL8hks5qjn9JgaJpZM4JtwyI
.

cushon · 2016-09-20T01:12:16Z

I don't think the assumption that the bridge's argument is Object holds in general, e.g.:

class Test {

  interface ErasedType {}
  interface RetType extends ErasedType {}
  interface Bound {}
  interface Refined extends Bound, ErasedType {}

  static abstract class Superclass<T extends ErasedType> {
    public T aMethod(T t) { return null; }
    public RetType widenReturn(T t) { return null; }
  }

  public interface ParameterisedInterfaceNotTypeBounded<T extends Bound> {
    T aMethod(T obj);
    ErasedType widenReturn(T obj);
  }

  public static class OtherImpl extends Superclass<Refined>
      implements ParameterisedInterfaceNotTypeBounded<Refined> {
    public String irrelvantMethod() { return "Not important"; }
  }
}

  public Test$Bound aMethod(Test$Bound);
    descriptor: (LTest$Bound;)LTest$Bound;
    flags: ACC_PUBLIC, ACC_BRIDGE, ACC_SYNTHETIC
    Code:
      stack=2, locals=2, args_size=2
         0: aload_0
         1: aload_1
         2: checkcast     #3                  // class Test$Refined
         5: invokespecial #5                  // Method Test$Superclass.aMethod:(LTest$ErasedType;)LTest$ErasedType;
         8: checkcast     #6                  // class Test$Bound
        11: areturn
      LineNumberTable:
        line 18: 0

I put together a naive fix for the same limitation in #90 before seeing this. What do we lose by just adding checkcasts? Are there cases where that changes overriding behaviour?

sameb · 2016-09-20T02:18:16Z

Don't think we lose much by adding the casts -- I wasn't terribly familiar
with the right way to do the checkcasts when I implemented this. For some
background, see b/4302685 internally. I think I just didn't add the casts
because I didn't know they were necessary, and the other compilers I was
emulating didn't add them in the cases I was looking at.

On Mon, Sep 19, 2016 at 9:12 PM Liam Miller-Cushon notifications@github.com
wrote:

I don't think the assumption that the bridge's argument is Object holds
in general, e.g.:

class Test {

interface ErasedType {}
interface RetType extends ErasedType {}
interface Bound {}
interface Refined extends Bound, ErasedType {}

static abstract class Superclass {
public T aMethod(T t) { return null; }
public RetType widenReturn(T t) { return null

; }
}

public interface ParameterisedInterfaceNotTypeBounded {

T aMethod(T obj);
ErasedType widenReturn(T obj);
}

public static class OtherImpl extends Superclass
implements ParameterisedInterfaceNotTypeBounded {
public String irrelvantMethod() { return "Not important"; }
}
}

public Test$Bound aMethod(Test$Bound);
descriptor: (LTest$Bound;)LTest$Bound;
flags: ACC_PUBLIC, ACC_BRIDGE, ACC_SYNTHETIC
Code:
stack=2, locals=2, args_size=2
0: aload_0
1: aload_1
2: checkcast #3 // class Test$Refined
5: invokespecial #5 // Method Test$Superclass.aMethod:(LTest$ErasedType;)LTest$ErasedType;
8: checkcast #6 // class Test$Bound
11: areturn
LineNumberTable:
line 18: 0

I put together a naive fix for the same limitation in #90
#90 before seeing this. What do we
lose by just adding checkcasts? Are there cases where that changes
overriding behaviour?

—
You are receiving this because you commented.

Reply to this email directly, view it on GitHub
#89 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ACC7EqLcPIj3RM8FskUDRbdrr0rh7O-Gks5qrzLxgaJpZM4JtwyI
.

GregBowyer · 2016-09-20T02:25:36Z

I never said the fix was all that great here, I slammed it together to get started on the conversation. I guess if it always does a checkcast the code itself could actually get simpler (and we would not have to read the classfiles like my version does too).

If it helps, for further information; this appears if the classfile for the target is found on the classloader that cglib resides in (typically the system / root classloader). If that class does not get found, cglib basically generates a version with checkcasts for simplicity.

GregBowyer · 2016-09-20T02:29:32Z

@cushon I much prefer your fix

cushon · 2016-09-21T18:12:09Z

I wasn't sure whether we want to preserve invokesuper in this case - this patch does, mine doesn't. But if everyone's OK with starting with the simple fix (which definitely avoids the VerifyError), that sounds good to me.

cushon · 2016-09-23T16:03:01Z

@GregBowyer do you know if there are cases where preserving invokespecial is necessary here? This patch does, and the other one doesn't.

I wasn't sure if we could ever end up accidentally overriding something. Having the bridge redirect through other proxy methods seems like it's usually the right behaviour.

raphw · 2016-09-25T00:01:56Z

This is just an academic guess, but I assume that the bridge methods might use invokespecial to avoid dynamic dispatch of the original method which would cause any interceptor to be invoked twice. In the worst case, this might result in some form of infinite recursion if the bridge method's interceptor and the original method's interceptor call one another.

cushon · 2016-09-25T23:08:01Z

cglib already leaves invokespecial bridges alone if they target a method with the same signature [1]. If a bridge was using invokespecial to avoid dynamic dispatch on a method with a different signature, that would mean there were two methods with the same erasure that didn't override each other, which would be an error in source.

In this case, invokespecial appears to be a small javac optimization for when the method that needs the bridge isn't implemented in the current class [2]. ecj gets away with using invokevirtual:

interface I<T> { void f(T t); }

static class A { public void f(String s) {} }

static class B extends A implements I<String> {
  // bridge void f(Object) {
  //   javac: invokespecial A.f(String)V
  //   ecj: invokevirtual B.f(String)V
  // }
}

static class C extends B {
  public void f(String s) {}
  // bridge void f(Object) {
  //   javac: invokevirtual C.f(String)V
  //   ecj: invokevirtual C.f(String)V
  // }
}

[1] https://github.com/cglib/cglib/blob/0258a52487421a4b33b9e9426f7b13ed6693c387/cglib/src/main/java/net/sf/cglib/proxy/BridgeMethodResolver.java#L95-100
[2] http://hg.openjdk.java.net/jdk9/dev/langtools/file/d4e74af5616d/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransTypes.java#l274

sameb · 2016-09-26T15:26:13Z

I added the bridge detection in df8bb42 -- the reason is described in the commit log: Fix so that when bridge methods are detected, cglib forces the bridge method to use invokevirtual instead of invokespecial (when possible) to allow the non-bridge methods to be intercepted. Previously, because javac sometimes created bridge methods using invokespecial (effectively calling super.aMethod), if a non-bridge method was intercepted but the call initiated from the bridge method, it wasn't guaranteed the non-bridge would receive the interception, because cglib added it to the subclass but the JVM dispatched the call directly to the superclass.

I added the change primarily to fix an interceptor bug in Guice: google/guice@d4ec8b0.

I figure the pull requests are an improvement no matter what because previously the code just didn't work... but it'd be even better if we can validate that the PR doesn't re-introduce the issues the bridge detection was intended to fix.

raphw · 2016-09-26T19:16:43Z

Just for the record, javac adds so-called visibility bridges to public types that extend non-public types with a public method. These bridges are not real bridge methods but only work to enable reflection.

http://bugs.java.com/view_bug.do?bug_id=6342411
https://bugs.openjdk.java.net/browse/JDK-8133168

javac only compiles such bridges to use invokespecial (as they call their super method). Ideally, cglib should not even override bridge methods but only such visibility bridges.

cushon · 2016-10-02T18:27:46Z

javac only compiles such bridges to use invokespecial (as they call their super method). Ideally, cglib should not even override bridge methods but only such visibility bridges.

cglib needs to override all invokespecial bridges, and javac doesn't use invokespecial exclusively for accessibility bridges.

but it'd be even better if we can validate that the PR doesn't re-introduce the issues the bridge detection was intended to fix

I verified that the guice tests still pass with the changes in #90.

raphw · 2016-10-03T10:27:34Z

You are right. I think however this invokespecial is breaking the spec. If C was compiled on B extends A but later changing it to B extends A implements I<String> (binary compatible), any virtual dispatch on I<String> results in invoking the overridden method what is not supposed to be allowed from outside a class. Chances are that I miss something so I asked on the javac mailing list.

What I mean by that is that I do not think we need to preserve the invokespecial for bridges but I still disagree that overriding all bridges is a good default behavior. Considering the following enhancement:

class Foo<T> {
  void m(T t) { }
}
class Bar extends Foo<String> {
  @Override
  public void m(String s) { }
}

I would consider the following outcome unexpected:

Enhancer enhancer = new Enhancer();
enhancer.setSuperclass(Bar.class);
enhancer.setCallback(new MethodInterceptor() {
  @Override
  public Object intercept(Object o, Method method, Object[] objects, MethodProxy methodProxy) throws Throwable {
    System.out.println("Intercepted");
    return methodProxy.invokeSuper(o, objects);
  }
});
System.out.println("Intercepted once");
Bar bar = (Bar) enhancer.create();
bar.m("");
System.out.println("Intercepted twice");
Foo<String> foo = bar;
foo.m("");

Of course, it is always possible to exclude any methods with the bridge modifier which does however also exclude visibility bridges what I would neither expect cglib to do.

This said, the type casting is required and this PR is definetly an improvement. I do however suggest merging @cushon's fix.

raphw · 2016-10-03T15:44:03Z

Just FYI, it is kind of a bug that was introduced to avoid another bug; have a look at http://mail.openjdk.java.net/pipermail/compiler-dev/2016-October/010369.html if you are interested.

In conclusion, it is not necessary to retain invokespecial as cglib mock classes are final anyways. Is everybody good with merging #90 and closing this PR (thanks @GregBowyer for your patch, too, if nobody had raced you to a fix, your suggestion would surely have made it after a refactor to cover the actual bridge erasure).

sameb · 2016-10-03T15:45:16Z

+1 merging #90.

GregBowyer added 2 commits August 25, 2016 19:01

Attempted fix for bridges that are created to do Object -> Type

62a06c5

[NOCOMMIT] Change version for testing

b6a7a11

GregBowyer force-pushed the fix-bridge-type-erasure-stuff branch from eca6ad0 to b6a7a11 Compare August 26, 2016 05:53

sameb mentioned this pull request Sep 19, 2016

Add checkcasts for arguments of invoked bridges #90

Merged

raphw closed this Oct 3, 2016

Fix bridge type erasure #89

Fix bridge type erasure #89

Uh oh!

Conversation

GregBowyer commented Aug 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sameb commented Aug 26, 2016

having the proxy patch up the bridge with invokevirtual.

Uh oh!

cushon commented Sep 20, 2016

Uh oh!

sameb commented Sep 20, 2016

Uh oh!

GregBowyer commented Sep 20, 2016

Uh oh!

GregBowyer commented Sep 20, 2016

Uh oh!

cushon commented Sep 21, 2016

Uh oh!

cushon commented Sep 23, 2016

Uh oh!

raphw commented Sep 25, 2016

Uh oh!

cushon commented Sep 25, 2016

Uh oh!

sameb commented Sep 26, 2016

Uh oh!

raphw commented Sep 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cushon commented Oct 2, 2016

Uh oh!

raphw commented Oct 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

raphw commented Oct 3, 2016

Uh oh!

sameb commented Oct 3, 2016

Uh oh!

Uh oh!

GregBowyer commented Aug 26, 2016 •

edited

Loading

raphw commented Sep 26, 2016 •

edited

Loading

raphw commented Oct 3, 2016 •

edited

Loading