Instrumenting constructors fails with VerificationError when interacting with classes that lack stack map frames

[kyle-cackett]

The Java Allocation Instrumenter passes the ClassWriter.COMPUTE_MAXS flag to the ASM ClassWriter during constructor instrumentation. According to the ASM Javadocs:

for classes whose version is Opcodes.V1_7 of more, this option requires valid stack map frames. The maximum stack size is then computed from these frames, and from the bytecode instructions in between. If stack map frames are not present or must be recomputed, used COMPUTE_FRAMES instead.

Other Java agents (such as the JaCoCo code coverage agent) may use ASM to write modified class files without computing stack map frames by passing 0 as a flag to the ClassWriter object. This flag is mapped to MethodWriter.COMPUTE_NOTHING which means that neither stack map frames nor the maximum stack size are computed. The interaction between agents which modify bytecode and do not recompute stack map frames and the Allocation Instrumenter leads to VerifyErrors when the JVM attempts to verify the twice-modified classes.

To improve the resiliency of the Allocation Instrumenter, we recommend passing the ClassWriter.COMPUTE_FRAMES flag to the ASM ClassWriter in place of the ClassWriter.COMPUTE_MAXS flag.

I have a minimal reproducible test case which has been used to verify that making the change suggested above does resolve the issue; I can provide you with it if necessary. In parallel, I am also opening an issue with the JaCoCo project to adjust the flags that they pass to their ClassWriter. However, in theory any bytecode-modifying agent could elect not to write stack map frames so it seems most effectively to address the issue here in the Allocation Instrumenter.

[kyle-cackett]

Minimal reproducible example can be found here: https://github.com/kyle-cackett/class-instrumentation

[jhmanson]

Yeah, I think that was just a mistake - we do the right thing in the AllocationInstrumenter class. I'll push a fix. Thanks for reporting it.

[kyle-cackett]

@jhmanson do you have an idea of when you think this fix will be pushed? This issue is keeping us from moving to Java 8 so we're trying to decide whether to wait for a fix upstream or fork the library and apply the patch below:

a/src/main/java/com/google/monitoring/runtime/instrumentation/ConstructorInstrumenter.java b/src/main/java/com/google/monitoring/runtime/instrumentation/ConstructorInstrumenter.java
index 933d4c4..e53ed5a 100644
--- a/src/main/java/com/google/monitoring/runtime/instrumentation/ConstructorInstrumenter.java
+++ b/src/main/java/com/google/monitoring/runtime/instrumentation/ConstructorInstrumenter.java
@@ -116,7 +116,7 @@ public class ConstructorInstrumenter implements ClassFileTransformer {
   public static byte[] instrument(byte[] originalBytes, Class<?> classBeingRedefined) {
     try {
       ClassReader cr = new ClassReader(originalBytes);
-      ClassWriter cw = new ClassWriter(cr, ClassWriter.COMPUTE_MAXS);
+      ClassWriter cw = new ClassWriter(cr, ClassWriter.COMPUTE_FRAMES);
       VerifyingClassAdapter vcw = new VerifyingClassAdapter(cw, originalBytes, cr.getClassName());
       ClassVisitor adapter = new ConstructorClassAdapter(vcw, classBeingRedefined);

[jhmanson]

Sorry about that. I pushed the patch internally, and then my machine died, so it fell off my radar. I can push it tomorrow morning.

[jhmanson]

All done!

[kyle-cackett]

Thank you!

[kyle-cackett]

@jhmanson Do you have any plans for an upcoming release which will contain this fix? We'd like to depend on an official version if possible.

[jhmanson]

Hm... I think I don't have the right permissions to release to sonatype etc. Maybe @cushon or @cgdecker can help?

[kyle-cackett]

@cushon @cgdecker Do you have a sense for when we might expect a new release of the allocation instrumenter containing this fix?

[cgdecker]

Sorry for the long delay... I've released java-allocation-instrumenter 3.3.0 to Maven Central with this fix.