Merge branch 'remove-checkdataflow-restrictions' into 'master'

Remove the restrictions for the checkDataFlow option in CheckClassAdapter.

See merge request !335

asm-util/src/main/java/org/objectweb/asm/util/CheckClassAdapter.java

@@ -39,6 +39,7 @@ import org.objectweb.asm.AnnotationVisitor;
 import org.objectweb.asm.Attribute;
 import org.objectweb.asm.ClassReader;
 import org.objectweb.asm.ClassVisitor;
+import org.objectweb.asm.ClassWriter;
 import org.objectweb.asm.FieldVisitor;
 import org.objectweb.asm.Label;
 import org.objectweb.asm.MethodVisitor;
@@ -161,7 +162,7 @@ public class CheckClassAdapter extends ClassVisitor {
    * @param classVisitor the class visitor to which this adapter must delegate calls.
    */
   public CheckClassAdapter(final ClassVisitor classVisitor) {
-    this(classVisitor, true);
+    this(classVisitor, /* checkDataFlow = */ true);
   }
 
   /**
@@ -169,8 +170,7 @@ public class CheckClassAdapter extends ClassVisitor {
    * Instead, they must use the {@link #CheckClassAdapter(int, ClassVisitor, boolean)} version.
    *
    * @param classVisitor the class visitor to which this adapter must delegate calls.
-   * @param checkDataFlow whether to perform basic data flow checks. This option requires valid
-   *     maxLocals and maxStack values.
+   * @param checkDataFlow whether to perform basic data flow checks.
    * @throws IllegalStateException If a subclass calls this constructor.
    */
   public CheckClassAdapter(final ClassVisitor classVisitor, final boolean checkDataFlow) {
@@ -187,8 +187,7 @@ public class CheckClassAdapter extends ClassVisitor {
    *     ASM}<i>x</i> values in {@link Opcodes}.
    * @param classVisitor the class visitor to which this adapter must delegate calls.
    * @param checkDataFlow {@literal true} to perform basic data flow checks, or {@literal false} to
-   *     not perform any data flow check (see {@link CheckMethodAdapter}). This option requires
-   *     valid maxLocals and maxStack values.
+   *     not perform any data flow check (see {@link CheckMethodAdapter}).
    */
   protected CheckClassAdapter(
       final int api, final ClassVisitor classVisitor, final boolean checkDataFlow) {
@@ -460,21 +459,17 @@ public class CheckClassAdapter extends ClassVisitor {
       }
     }
     CheckMethodAdapter checkMethodAdapter;
+    MethodVisitor methodVisitor =
+        super.visitMethod(access, name, descriptor, signature, exceptions);
     if (checkDataFlow) {
+      if (cv instanceof ClassWriter) {
+        methodVisitor =
+            new CheckMethodAdapter.MethodWriterWrapper(api, (ClassWriter) cv, methodVisitor);
+      }
       checkMethodAdapter =
-          new CheckMethodAdapter(
-              api,
-              access,
-              name,
-              descriptor,
-              super.visitMethod(access, name, descriptor, signature, exceptions),
-              labelInsnIndices);
+          new CheckMethodAdapter(api, access, name, descriptor, methodVisitor, labelInsnIndices);
     } else {
-      checkMethodAdapter =
-          new CheckMethodAdapter(
-              api,
-              super.visitMethod(access, name, descriptor, signature, exceptions),
-              labelInsnIndices);
+      checkMethodAdapter = new CheckMethodAdapter(api, methodVisitor, labelInsnIndices);
     }
     checkMethodAdapter.version = version;
     return checkMethodAdapter;

asm-util/src/main/java/org/objectweb/asm/util/CheckMethodAdapter.java

@@ -38,6 +38,7 @@ import java.util.Map;
 import java.util.Set;
 import org.objectweb.asm.AnnotationVisitor;
 import org.objectweb.asm.Attribute;
+import org.objectweb.asm.ClassWriter;
 import org.objectweb.asm.ConstantDynamic;
 import org.objectweb.asm.Handle;
 import org.objectweb.asm.Label;
@@ -448,13 +449,20 @@ public class CheckMethodAdapter extends MethodVisitor {
           public void visitEnd() {
             Analyzer<BasicValue> analyzer = new Analyzer<>(new BasicVerifier());
             try {
-              analyzer.analyze("dummy", this);
-            } catch (IndexOutOfBoundsException e) {
-              if (maxLocals == 0 && maxStack == 0) {
-                throw new IllegalArgumentException(
-                    "Data flow checking option requires valid, non zero maxLocals and maxStack.",
-                    e);
+              // If 'methodVisitor' is a MethodWriter of a ClassWriter with no flags to compute the
+              // max stack and locals nor the stack map frames, we know that valid max stack and
+              // locals must be provided. Otherwise we assume they are not needed at this stage.
+              // TODO(ebruneton): similarly, check that valid stack map frames are provided if the
+              // class writer has no flags to compute them, and the class version is V1_7 or more.
+              boolean checkMaxStackAndLocals =
+                  (methodVisitor instanceof MethodWriterWrapper)
+                      && !((MethodWriterWrapper) methodVisitor).computesMaxs();
+              if (checkMaxStackAndLocals) {
+                analyzer.analyze("dummy", this);
+              } else {
+                analyzer.analyzeAndComputeMaxs("dummy", this);
               }
+            } catch (IndexOutOfBoundsException e) {
               throwError(analyzer, e);
             } catch (AnalyzerException e) {
               throwError(analyzer, e);
@@ -1439,4 +1447,18 @@ public class CheckMethodAdapter extends MethodVisitor {
       throw new IllegalArgumentException(INVALID + message + " (must be visited first)");
     }
   }
+
+  static class MethodWriterWrapper extends MethodVisitor {
+
+    private final ClassWriter owner;
+
+    MethodWriterWrapper(final int api, final ClassWriter owner, final MethodVisitor methodWriter) {
+      super(api, methodWriter);
+      this.owner = owner;
+    }
+
+    boolean computesMaxs() {
+      return owner.hasFlags(ClassWriter.COMPUTE_MAXS) || owner.hasFlags(ClassWriter.COMPUTE_FRAMES);
+    }
+  }
 }

asm-util/src/test/java/org/objectweb/asm/util/CheckClassAdapterTest.java

@@ -398,6 +398,66 @@ class CheckClassAdapterTest extends AsmTest implements Opcodes {
     assertEquals("<T::LI.J<*+LA;>;>()V^LA;X: error at index 24", exception.getMessage());
   }
 
+  @Test
+  void testVisitMethod_checkDataFlowByDefault() {
+    CheckClassAdapter checkClassAdapter = new CheckClassAdapter(null);
+    checkClassAdapter.visit(V1_1, ACC_PUBLIC, "C", null, "java/lang/Object", null);
+    MethodVisitor methodVisitor =
+        checkClassAdapter.visitMethod(ACC_PUBLIC, "m", "(I)I", null, null);
+    methodVisitor.visitCode();
+    methodVisitor.visitVarInsn(ILOAD, 1);
+    methodVisitor.visitVarInsn(ASTORE, 0);
+    methodVisitor.visitInsn(IRETURN);
+    methodVisitor.visitMaxs(0, 0);
+
+    Executable visitEnd = () -> methodVisitor.visitEnd();
+
+    Exception exception = assertThrows(IllegalArgumentException.class, visitEnd);
+    assertTrue(
+        exception
+            .getMessage()
+            .startsWith(
+                "Error at instruction 1: Expected an object reference or a return address, but"
+                    + " found I m(I)I"));
+  }
+
+  @Test
+  void testVisitMethod_checkMaxStackAndLocalsIfClassWriterWithoutComputeMaxs() {
+    ClassWriter classWriter = new ClassWriter(0);
+    CheckClassAdapter checkClassAdapter = new CheckClassAdapter(classWriter);
+    checkClassAdapter.visit(V1_1, ACC_PUBLIC, "C", null, "java/lang/Object", null);
+    MethodVisitor methodVisitor =
+        checkClassAdapter.visitMethod(ACC_PUBLIC, "m", "(I)I", null, null);
+    methodVisitor.visitCode();
+    methodVisitor.visitVarInsn(ILOAD, 1);
+    methodVisitor.visitInsn(IRETURN);
+    methodVisitor.visitMaxs(0, 2);
+
+    Executable visitEnd = () -> methodVisitor.visitEnd();
+
+    Exception exception = assertThrows(IllegalArgumentException.class, visitEnd);
+    assertTrue(
+        exception
+            .getMessage()
+            .startsWith("Error at instruction 0: Insufficient maximum stack size. m(I)I"));
+  }
+
+  @Test
+  void testVisitMethod_noDataFlowCheckIfDisabled() {
+    CheckClassAdapter checkClassAdapter = new CheckClassAdapter(null, /* checkDataFlow = */ false);
+    checkClassAdapter.visit(V1_1, ACC_PUBLIC, "C", null, "java/lang/Object", null);
+    MethodVisitor methodVisitor = checkClassAdapter.visitMethod(ACC_PUBLIC, "m", "()V", null, null);
+    methodVisitor.visitCode();
+    methodVisitor.visitVarInsn(ILOAD, 1);
+    methodVisitor.visitVarInsn(ASTORE, 0);
+    methodVisitor.visitInsn(IRETURN);
+    methodVisitor.visitMaxs(0, 0);
+
+    Executable visitEnd = () -> methodVisitor.visitEnd();
+
+    assertDoesNotThrow(visitEnd);
+  }
+
   @Test
   void testVisitTypeAnnotation_illegalAnnotation1() {
     CheckClassAdapter checkClassAdapter = new CheckClassAdapter(null);

asm-util/src/test/java/org/objectweb/asm/util/CheckMethodAdapterTest.java

@@ -35,6 +35,7 @@ import static org.junit.jupiter.api.Assertions.assertTrue;
 import java.util.HashMap;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.function.Executable;
+import org.objectweb.asm.ClassWriter;
 import org.objectweb.asm.Handle;
 import org.objectweb.asm.Label;
 import org.objectweb.asm.MethodVisitor;
@@ -1085,8 +1086,10 @@ class CheckMethodAdapterTest extends AsmTest implements Opcodes {
     MethodVisitor dataFlowCheckMethodAdapter =
         new CheckMethodAdapter(ACC_PUBLIC, "m", "(I)I", null, new HashMap<>());
     dataFlowCheckMethodAdapter.visitCode();
-    dataFlowCheckMethodAdapter.visitInsn(RETURN);
-    dataFlowCheckMethodAdapter.visitMaxs(0, 2);
+    dataFlowCheckMethodAdapter.visitVarInsn(ILOAD, 1);
+    dataFlowCheckMethodAdapter.visitVarInsn(ASTORE, 0);
+    dataFlowCheckMethodAdapter.visitInsn(IRETURN);
+    dataFlowCheckMethodAdapter.visitMaxs(0, 0);
 
     Executable visitEnd = () -> dataFlowCheckMethodAdapter.visitEnd();
 
@@ -1094,17 +1097,65 @@ class CheckMethodAdapterTest extends AsmTest implements Opcodes {
     assertTrue(
         exception
             .getMessage()
-            .startsWith("Error at instruction 0: Incompatible return type m(I)I"));
+            .startsWith(
+                "Error at instruction 1: Expected an object reference or a return address, but"
+                    + " found I m(I)I"));
   }
 
   @Test
-  void testVisitEnd_invalidReturnType() {
+  void testVisitEnd_noInvalidMaxStackIfClassWriterWithComputeMaxs() {
+    ClassWriter classWriter = new ClassWriter(ClassWriter.COMPUTE_MAXS);
+    MethodVisitor methodVisitor =
+        new CheckMethodAdapter.MethodWriterWrapper(
+            /* latest api = */ Opcodes.ASM9,
+            classWriter,
+            new MethodVisitor(/* latest api = */ Opcodes.ASM9) {});
     MethodVisitor dataFlowCheckMethodAdapter =
-        new CheckMethodAdapter(ACC_PUBLIC, "m", "(I)V", null, new HashMap<>());
+        new CheckMethodAdapter(ACC_PUBLIC, "m", "(I)I", methodVisitor, new HashMap<>());
     dataFlowCheckMethodAdapter.visitCode();
     dataFlowCheckMethodAdapter.visitVarInsn(ILOAD, 1);
     dataFlowCheckMethodAdapter.visitInsn(IRETURN);
-    dataFlowCheckMethodAdapter.visitMaxs(1, 2);
+    dataFlowCheckMethodAdapter.visitMaxs(0, 2);
+
+    Executable visitEnd = () -> dataFlowCheckMethodAdapter.visitEnd();
+
+    assertDoesNotThrow(visitEnd);
+  }
+
+  @Test
+  void testVisitEnd_noInvalidMaxStackIfClassWriterWithComputeFrales() {
+    ClassWriter classWriter = new ClassWriter(ClassWriter.COMPUTE_FRAMES);
+    MethodVisitor methodVisitor =
+        new CheckMethodAdapter.MethodWriterWrapper(
+            /* latest api = */ Opcodes.ASM9,
+            classWriter,
+            new MethodVisitor(/* latest api = */ Opcodes.ASM9) {});
+    MethodVisitor dataFlowCheckMethodAdapter =
+        new CheckMethodAdapter(ACC_PUBLIC, "m", "(I)I", methodVisitor, new HashMap<>());
+    dataFlowCheckMethodAdapter.visitCode();
+    dataFlowCheckMethodAdapter.visitVarInsn(ILOAD, 1);
+    dataFlowCheckMethodAdapter.visitInsn(IRETURN);
+    dataFlowCheckMethodAdapter.visitMaxs(0, 2);
+
+    Executable visitEnd = () -> dataFlowCheckMethodAdapter.visitEnd();
+
+    assertDoesNotThrow(visitEnd);
+  }
+
+  @Test
+  void testVisitEnd_invalidMaxStackIfClassWriterWithoutComputeMaxsOrComputeFrames() {
+    ClassWriter classWriter = new ClassWriter(0);
+    MethodVisitor methodVisitor =
+        new CheckMethodAdapter.MethodWriterWrapper(
+            /* latest api = */ Opcodes.ASM9,
+            classWriter,
+            new MethodVisitor(/* latest api = */ Opcodes.ASM9) {});
+    MethodVisitor dataFlowCheckMethodAdapter =
+        new CheckMethodAdapter(ACC_PUBLIC, "m", "(I)I", methodVisitor, new HashMap<>());
+    dataFlowCheckMethodAdapter.visitCode();
+    dataFlowCheckMethodAdapter.visitVarInsn(ILOAD, 1);
+    dataFlowCheckMethodAdapter.visitInsn(IRETURN);
+    dataFlowCheckMethodAdapter.visitMaxs(0, 2);
 
     Executable visitEnd = () -> dataFlowCheckMethodAdapter.visitEnd();
 
@@ -1112,24 +1163,26 @@ class CheckMethodAdapterTest extends AsmTest implements Opcodes {
     assertTrue(
         exception
             .getMessage()
-            .startsWith(
-                "Error at instruction 1: Incompatible return type: expected null, but found I m(I)V"));
+            .startsWith("Error at instruction 0: Insufficient maximum stack size. m(I)I"));
   }
 
   @Test
-  void testVisitEnd_dataflowCheckRequiresMaxLocalsAndMaxStack() {
-    CheckMethodAdapter dataFlowCheckMethodAdapter =
-        new CheckMethodAdapter(0, "m", "()V", null, new HashMap<>());
+  void testVisitEnd_invalidReturnType() {
+    MethodVisitor dataFlowCheckMethodAdapter =
+        new CheckMethodAdapter(ACC_PUBLIC, "m", "(I)V", null, new HashMap<>());
     dataFlowCheckMethodAdapter.visitCode();
-    dataFlowCheckMethodAdapter.visitVarInsn(ALOAD, 0);
-    dataFlowCheckMethodAdapter.visitInsn(RETURN);
-    dataFlowCheckMethodAdapter.visitMaxs(0, 0);
+    dataFlowCheckMethodAdapter.visitVarInsn(ILOAD, 1);
+    dataFlowCheckMethodAdapter.visitInsn(IRETURN);
+    dataFlowCheckMethodAdapter.visitMaxs(1, 2);
 
     Executable visitEnd = () -> dataFlowCheckMethodAdapter.visitEnd();
 
     Exception exception = assertThrows(IllegalArgumentException.class, visitEnd);
-    assertEquals(
-        "Data flow checking option requires valid, non zero maxLocals and maxStack.",
-        exception.getMessage());
+    assertTrue(
+        exception
+            .getMessage()
+            .startsWith(
+                "Error at instruction 1: Incompatible return type: expected null, but found I"
+                    + " m(I)V"));
   }
 }

asm/src/main/java/org/objectweb/asm/ClassWriter.java

@@ -65,6 +65,12 @@ public class ClassWriter extends ClassVisitor {
    */
   public static final int COMPUTE_FRAMES = 2;
 
+  /**
+   * The flags passed to the constructor. Must be zero or more of {@link #COMPUTE_MAXS} and {@link
+   * #COMPUTE_FRAMES}.
+   */
+  private final int flags;
+
   // Note: fields are ordered as in the ClassFile structure, and those related to attributes are
   // ordered as in Section 4.7 of the JVMS.
 
@@ -248,23 +254,39 @@ public class ClassWriter extends ClassVisitor {
    * @param classReader the {@link ClassReader} used to read the original class. It will be used to
    *     copy the entire constant pool and bootstrap methods from the original class and also to
    *     copy other fragments of original bytecode where applicable.
-   * @param flags option flags that can be used to modify the default behavior of this class.Must be
-   *     zero or more of {@link #COMPUTE_MAXS} and {@link #COMPUTE_FRAMES}. <i>These option flags do
-   *     not affect methods that are copied as is in the new class. This means that neither the
+   * @param flags option flags that can be used to modify the default behavior of this class. Must
+   *     be zero or more of {@link #COMPUTE_MAXS} and {@link #COMPUTE_FRAMES}. <i>These option flags
+   *     do not affect methods that are copied as is in the new class. This means that neither the
    *     maximum stack size nor the stack frames will be computed for these methods</i>.
    */
   public ClassWriter(final ClassReader classReader, final int flags) {
     super(/* latest api = */ Opcodes.ASM9);
+    this.flags = flags;
     symbolTable = classReader == null ? new SymbolTable(this) : new SymbolTable(this, classReader);
     if ((flags & COMPUTE_FRAMES) != 0) {
-      this.compute = MethodWriter.COMPUTE_ALL_FRAMES;
+      compute = MethodWriter.COMPUTE_ALL_FRAMES;
     } else if ((flags & COMPUTE_MAXS) != 0) {
-      this.compute = MethodWriter.COMPUTE_MAX_STACK_AND_LOCAL;
+      compute = MethodWriter.COMPUTE_MAX_STACK_AND_LOCAL;
     } else {
-      this.compute = MethodWriter.COMPUTE_NOTHING;
+      compute = MethodWriter.COMPUTE_NOTHING;
     }
   }
 
+  // -----------------------------------------------------------------------------------------------
+  // Accessors
+  // -----------------------------------------------------------------------------------------------
+
+  /**
+   * Returns true if all the given flags were passed to the constructor.
+   *
+   * @param flags some option flags. Must be zero or more of {@link #COMPUTE_MAXS} and {@link
+   *     #COMPUTE_FRAMES}.
+   * @return true if all the given flags, or more, were passed to the constructor.
+   */
+  public boolean hasFlags(final int flags) {
+    return (this.flags & flags) == flags;
+  }
+
   // -----------------------------------------------------------------------------------------------
   // Implementation of the ClassVisitor abstract class
   // -----------------------------------------------------------------------------------------------

asm/src/main/java/org/objectweb/asm/MethodWriter.java

@@ -466,7 +466,8 @@ final class MethodWriter extends MethodVisitor {
 
   /**
    * Indicates what must be computed. Must be one of {@link #COMPUTE_ALL_FRAMES}, {@link
-   * #COMPUTE_INSERTED_FRAMES}, {@link #COMPUTE_MAX_STACK_AND_LOCAL} or {@link #COMPUTE_NOTHING}.
+   * #COMPUTE_INSERTED_FRAMES}, {@link COMPUTE_MAX_STACK_AND_LOCAL_FROM_FRAMES}, {@link
+   * #COMPUTE_MAX_STACK_AND_LOCAL} or {@link #COMPUTE_NOTHING}.
    */
   private final int compute;
 

asm/src/test/java/org/objectweb/asm/ClassWriterTest.java

@@ -30,6 +30,7 @@ package org.objectweb.asm;
 import static java.util.stream.Collectors.toSet;
 import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.junit.jupiter.api.Assertions.fail;
@@ -79,6 +80,7 @@ class ClassWriterTest extends AsmTest {
     Set<String> expectedFields =
         new HashSet<String>(
             Arrays.asList(
+                "flags",
                 "version",
                 "symbolTable",
                 "accessFlags",
@@ -510,6 +512,8 @@ class ClassWriterTest extends AsmTest {
 
     classReader.accept(classWriter, attributes(), ClassReader.SKIP_CODE);
 
+    assertFalse(classWriter.hasFlags(ClassWriter.COMPUTE_MAXS));
+    assertFalse(classWriter.hasFlags(ClassWriter.COMPUTE_FRAMES));
     assertTrue(
         new ClassFile(classWriter.toByteArray())
             .toString()
@@ -563,6 +567,8 @@ class ClassWriterTest extends AsmTest {
 
     classReader.accept(classWriter, attributes(), 0);
 
+    assertTrue(classWriter.hasFlags(ClassWriter.COMPUTE_MAXS));
+    assertFalse(classWriter.hasFlags(ClassWriter.COMPUTE_FRAMES));
     assertEquals(new ClassFile(classFile), new ClassFile(classWriter.toByteArray()));
   }
 
@@ -604,6 +610,8 @@ class ClassWriterTest extends AsmTest {
 
     byte[] newClassFile = classWriter.toByteArray();
 
+    assertFalse(classWriter.hasFlags(ClassWriter.COMPUTE_MAXS));
+    assertTrue(classWriter.hasFlags(ClassWriter.COMPUTE_FRAMES));
     // The computed stack map frames should be equal to the original ones, if any (classes before
     // JDK8 don't have ones). This is not true in general (the valid frames for a given method are
     // not unique), but this should be the case with our precompiled classes.