Issue with StackMap.Shifter not correctly shifting stack map table offsets.

[danysantiago]

Hi - It seems that when using a CodeIterator to insert some new instructions, the StackMapTable#shiftPc stack map table shifter is not correctly updating the offset_delta of the stack map frames.

The transform I am trying to do is a simple addition of an invokespecial at the start of a method, from this:

public class Foo extends Bar {
  @Override
  public void method(String arg0) {
    // Complex code
  }
}

to this:

public class Foo extends Bar {
  @Override
  public void method(String arg0) {
    super.method(arg0); // invokespecial
    // Complex code
  }
}

My approach is to use a CodeIterator as follow:

Bytecode newCode = new Bytecode(constPool);
newCode.addAload(0); // Loads 'this'
newCode.addAload(1); // Loads method param 1 (String)
newCode.addInvokespecial(superClassname, "method", "(Ljava/lang/String;)V");

codeIterator.insertEx(newCode.get());

I also update the max stack and max locals, but following through the code of insertEx it seems both the exception table and the stack map table should be properly updated, but I still get a java.lang.VerifyError: StackMapTable error: bad offset when executing the code.

I am aware there is a rebuildStackMap and when used things work, but I have the limitation that my ClassPool is limited and does not contain the full classpath that was used during compilation, therefore it might be that when rebuilding the stack map table, types have to be fixed and loaded from the pool which will fail to happen. I hope that makes sense.

My first question is, should I be relying on insertEx to update the exception table and stack map table or should I try to do that myself? If the former is the case, then might there be a bug with the StackMapTable Shifter?

I have a sample project to demonstrate my issue: javassit-bad-stack-map.zip

In it you'll find a bash script called run.sh that will execute the necessary commands to reproduce the issue (working directory must be the script's dir, you might need to chmod +x both gradlew and run.sh). It will:

• Compile Foo.java and Bar.java which are the sources that will produce the bytecode I wish to transform.
• Via Gradle, it will compile and run Main.java which is where I use javassit to perform the transform.
• It will then run the transformed Foo.class

The transform code is in javassit-bad-stack-map/src/main/java/Main.java
The sources and classes transformed are in javassit-bad-stack-map/src/main/resources

The relevant section of the bytecode is as follow:

• When using rebuildStackMap:

javassit-bad-stack-map: javap -v Foo 
Classfile /Users/danysantiago/javassit-bad-stack-map/Foo.class
  ...
  public void method(java.lang.String);
    descriptor: (Ljava/lang/String;)V
    flags: (0x0001) ACC_PUBLIC
    Code:
      stack=3, locals=5, args_size=2
         0: aload_0
         1: aload_1
         2: invokespecial #43                 // Method Bar.method:(Ljava/lang/String;)V
         5: aload_1
         6: ifnull        37
         9: iconst_0
        10: istore_2
        11: aload_1
        12: iconst_0
        13: invokevirtual #6                  // Method java/lang/String.charAt:(I)C
        16: invokestatic  #7                  // Method java/lang/Character.valueOf:(C)Ljava/lang/Character;
        19: astore_3
        20: new           #8                  // class java/lang/IllegalStateException
        23: dup
        24: ldc           #9                  // String
        26: invokespecial #10                 // Method java/lang/IllegalStateException."<init>":(Ljava/lang/String;)V
        29: athrow
        30: astore        4
        32: iconst_1
        33: istore_2
        34: goto          43
        37: iconst_0
        38: istore_2
        39: aload_0
        40: astore_3
        41: iconst_0
        42: istore_2
        43: return
      Exception table:
         from    to  target type
            20    30    30   Class java/lang/IllegalStateException
      LineNumberTable:
        line 15: 5
        line 16: 9
        line 17: 11
        line 19: 20
        line 20: 30
        line 21: 32
        line 23: 34
        line 24: 37
        line 25: 39
        line 26: 41
        line 28: 43
      StackMapTable: number_of_entries = 3
        frame_type = 255 /* full_frame */
          offset_delta = 30
          locals = [ class Foo, class java/lang/String, int, class java/lang/Character ]
          stack = [ class java/lang/IllegalStateException ]
        frame_type = 249 /* chop */
          offset_delta = 6
        frame_type = 253 /* append */
          offset_delta = 5
          locals = [ int, class java/lang/Object ]
}
SourceFile: "Foo.java"

• Bytecode produce by javac if the original Foo.java had the super call.

javassit-bad-stack-map: javap -v src/main/resources/Foo.class
Classfile /Users/danysantiago/javassit-bad-stack-map/src/main/resources/Foo.class
  ...
  public void method(java.lang.String);
    descriptor: (Ljava/lang/String;)V
    flags: (0x0001) ACC_PUBLIC
    Code:
      stack=3, locals=5, args_size=2
         0: aload_0
         1: aload_1
         2: invokespecial #6                  // Method Bar.method:(Ljava/lang/String;)V
         5: aload_1
         6: ifnull        37
         9: iconst_0
        10: istore_2
        11: aload_1
        12: iconst_0
        13: invokevirtual #7                  // Method java/lang/String.charAt:(I)C
        16: invokestatic  #8                  // Method java/lang/Character.valueOf:(C)Ljava/lang/Character;
        19: astore_3
        20: new           #9                  // class java/lang/IllegalStateException
        23: dup
        24: ldc           #10                 // String
        26: invokespecial #11                 // Method java/lang/IllegalStateException."<init>":(Ljava/lang/String;)V
        29: athrow
        30: astore        4
        32: iconst_1
        33: istore_2
        34: goto          43
        37: iconst_0
        38: istore_2
        39: aload_0
        40: astore_3
        41: iconst_0
        42: istore_2
        43: return
      Exception table:
         from    to  target type
            20    30    30   Class java/lang/IllegalStateException
      LineNumberTable:
        line 10: 0
        line 15: 5
        line 16: 9
        line 17: 11
        line 19: 20
        line 20: 30
        line 21: 32
        line 23: 34
        line 24: 37
        line 25: 39
        line 26: 41
        line 28: 43
      StackMapTable: number_of_entries = 3
        frame_type = 255 /* full_frame */
          offset_delta = 30
          locals = [ class Foo, class java/lang/String, int, class java/lang/Object ]
          stack = [ class java/lang/IllegalStateException ]
        frame_type = 249 /* chop */
          offset_delta = 6
        frame_type = 252 /* append */
          offset_delta = 5
          locals = [ int ]
}
SourceFile: "Foo.java"

• When not rebuilding the stack map (which causes the verify error), the transformed bytecode is:

javassit-bad-stack-map: javap -v Foo.class
Classfile /Users/danysantiago/javassit-bad-stack-map/Foo.class
  ...
  public void method(java.lang.String);
    descriptor: (Ljava/lang/String;)V
    flags: (0x0001) ACC_PUBLIC
    Code:
      stack=3, locals=5, args_size=2
         0: aload_0
         1: aload_1
         2: invokespecial #43                 // Method Bar.method:(Ljava/lang/String;)V
         5: aload_1
         6: ifnull        37
         9: iconst_0
        10: istore_2
        11: aload_1
        12: iconst_0
        13: invokevirtual #6                  // Method java/lang/String.charAt:(I)C
        16: invokestatic  #7                  // Method java/lang/Character.valueOf:(C)Ljava/lang/Character;
        19: astore_3
        20: new           #8                  // class java/lang/IllegalStateException
        23: dup
        24: ldc           #9                  // String
        26: invokespecial #10                 // Method java/lang/IllegalStateException."<init>":(Ljava/lang/String;)V
        29: athrow
        30: astore        4
        32: iconst_1
        33: istore_2
        34: goto          43
        37: iconst_0
        38: istore_2
        39: aload_0
        40: astore_3
        41: iconst_0
        42: istore_2
        43: return
      Exception table:
         from    to  target type
            20    30    30   Class java/lang/IllegalStateException
      LineNumberTable:
        line 15: 5
        line 16: 9
        line 17: 11
        line 19: 20
        line 20: 30
        line 21: 32
        line 23: 34
        line 24: 37
        line 25: 39
        line 26: 41
        line 28: 43
      StackMapTable: number_of_entries = 3
        frame_type = 255 /* full_frame */
          offset_delta = 25
          locals = [ class Foo, class java/lang/String, int, class java/lang/Object ]
          stack = [ class java/lang/IllegalStateException ]
        frame_type = 249 /* chop */
          offset_delta = 6
        frame_type = 252 /* append */
          offset_delta = 5
          locals = [ int ]
}
SourceFile: "Foo.java"

Let me know if you need more information, thanks!

[danysantiago]

I wanted to add, that I am willing to write my own StackMapTable.Walker that performs the offset shift operation, but I don't know enough about the JVM Specs to feel confident to get it right.

I somewhat understand the frames are ordered and the affected bytecode location is calculated with previous frames, but it seems frames have both explicit and implicit offset_delta and therefore I don't know which frames exactly I should be updating their offset_delta or if I should be doing it for them all. Any extra advice here would be appreciate it.

[chibash]

Have you tried javassist.bytecode.MethodInfo#rebuildStackMap​(ClassPool pool) ?
It automatically rebuilds the stack map.

[danysantiago]

@chibash Yes I have and it works well, I mentioned this in my original comment but it might have gotten lost in the rest of it:

I am aware there is a rebuildStackMap and when used things work, but I have the limitation that my ClassPool is limited and does not contain the full classpath that was used during compilation, therefore it might be that when rebuilding the stack map table, types have to be fixed and loaded from the pool which will fail to happen. I hope that makes sense.

In other words, I want to avoid using rebuildStackMap since it will try to use the ClassPool which is limited in my environment.

I was able to build a StackMapTable.Walker similar to the one Javassist has for shifting, it is really just for my use-case (inserting a simple invokespecial at the start of a method. You Can find it here: https://gist.github.com/danysantiago/e76dad4114148b56d2be40068008be7a

I do think there is still a bug in Javassist's stack map table shifter, specifically I think the exclusive case is not being well considered here:

javassist/src/main/javassist/bytecode/StackMapTable.java

Lines 883 to 886 in b70a41b

	if (exclusive)
	match = oldPos < where && where <= position;
	else
	match = oldPos <= where && where < position;

When inserting a new gap at the beginning of the code via insertEx, the where value is 0 and exclusive is true, only the first frame should be updated, subsequent frames don't have to be updated because of the formula offset_delta + 1 mentioned in the JVM Spec section 4.7.4. When we enter the first frame via the Walker, oldPos is 0, making the the statement oldPos < where false. I believe exclusive here means a bit the opposite, exclusive comes from the premise that the gap will be inserted 'exclusively' before the where location, therefore if the range of bytecode positions covered by the frame includes the gap location, then it must be shifted. Which makes me believe that all we need to do is invert the check:

if (exclusive)
    match = oldPos <= where  && where < position;
else
    match = oldPos < where  && where <= position;

Let me know if that makes sense, if you agree I can try to make a Pull Request.

[chibash]

Sorry, I didn't read your first post. I'll check your suggestion is correct.

[danysantiago]

Hi @chibash, just wanted to ping you back if you had any updates on this issue? It puts us in an awkward position because if we release a version of our library with the workaround and this ends up getting fixed in Javassit, the workaround will break since we will over adjust the offsets. So in a way we have to either wait for this fix in Javassit or deal and explain to our users some versioning nightmare to avoid the issue. Thank you so much!

[chibash]

Ah, OK, I'll.

[chibash]

I believe that StackMapTable is not wrong. This issue is more complicated than you expect. Let me try to explain this.

First of all, when you insert a code fragment exclusively at byte position 0, you must insert a new stack map entry for the old first block. Inserting a fragment at position 0 is necessary when the original code is like this:

void foo(i) {
    // you want to insert something here.
    while (i > 0) {
       --i;
    }
}

The bytecode will include GOTO for going back to the first instruction in the body. So inserting something at position 0 creates a new basic block and the old first basic block is turned into the second basic block.
Since only the stack map entry for the first basic block is omitted, you must create a new stack map entry for the new second basic block, which was originally the first one. StackMap.Shifter assumes that you insert a new stack map entry for the old first basic block. This is why StackMap.Shifter did not work for your code, which did not insert a new stack map entry.

Note that the reason your fix worked is that the first statement in your example was not a loop. In this case, you don't have to create a new stack map entry since the insertion is accidentally equivalent to inserting inclusively.

To create a new stack map entry, you must learn the details of the Java bytecode lots, so I don't recommend you to do that.

A possible workaround is to insert a code fragment inclusively. It works unless the first statement is a loop.

[danysantiago]

Thanks, yeah I definitely don't want to be creating my own stack map frames, that is very complex.

I am really relying on inserting exclusively (insertEx) so that branch statements such as goto and if_icmp<cond> get updated accordingly. Therefore inserting inclusively is not a good workaround for me, sorry.

If you think StackMapTable is not wrong, then my question to you is: If the code fragment that I want to insert is simple enough, do you still think I need a new stack map frame? My hypothesis is that if the code I am inserting at the start of a method is always simple enough then I can get away by only updating the first frame in the stack map since subsequent frames are based of the previous one.

Using your example:

void method(int i) {
    // you want to insert something here.
    while (i > 0) {
       --i;
    }
}

which produces:

  public void method(int);
    descriptor: (I)V
    flags: (0x0001) ACC_PUBLIC
    Code:
      stack=1, locals=2, args_size=2
         0: iload_1
         1: ifle          10
         4: iinc          1, -1
         7: goto          0
        10: return
      StackMapTable: number_of_entries = 2
        frame_type = 0 /* same */
        frame_type = 9 /* same */

I would transform it to:

  public void method(int);
    descriptor: (I)V
    flags: (0x0001) ACC_PUBLIC
    Code:
      stack=2, locals=2, args_size=2
         0: aload_0
         1: iload_1
         2: invokespecial #21                 // Method Bar.method:(I)V
         5: iload_1
         6: ifle          15
         9: iinc          1, -1
        12: goto          5
        15: return
      StackMapTable: number_of_entries = 2
        frame_type = 5 /* same */
        frame_type = 9 /* same */

Notice how I only update the first frame of the stack map (the first non-implicit frame), since the code to be inserted does not need to be covered by a frame.

[chibash]

OK, thank you for the information. The stack map produced by javac was different what I expect.
I agree that this is a bug of Javassist. I'll fix it soon.