Fix verify error when ctor params are used after a call site (#9083)

* Fix verify error when ctor params are used after a call site

* Keep tests separated

Author: manuel-alvarez-alvarez

dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/csi/Advices.java

@@ -370,7 +370,8 @@ void onConstantPool(
         @Nonnull TypeDescription type, @Nonnull ConstantPool pool, final byte[] classFile);
   }
 
-  private interface TypedAdvice {
+  // Kept public only for testing
+  public interface TypedAdvice {
     byte getType();
 
     static CallSiteAdvice withType(final CallSiteAdvice advice, final byte type) {

dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/csi/CallSiteTransformer.java

@@ -360,10 +360,12 @@ public void visitMethodInsn(
     @Override
     public void advice(final String owner, final String name, final String descriptor) {
       if (isSuperCall) {
-        // append this to the stack after super call
-        mv.visitIntInsn(Opcodes.ALOAD, 0);
+        mv.visitIntInsn(Opcodes.ALOAD, 0); // append this to the stack after super call
+        mv.visitMethodInsn(Opcodes.INVOKESTATIC, owner, name, descriptor, false);
+        mv.visitInsn(Opcodes.POP); // pop the result of the advice call
+      } else {
+        mv.visitMethodInsn(Opcodes.INVOKESTATIC, owner, name, descriptor, false);
       }
-      mv.visitMethodInsn(Opcodes.INVOKESTATIC, owner, name, descriptor, false);
     }
 
     @Override

dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/csi/BaseCallSiteTest.groovy

@@ -54,7 +54,7 @@ class BaseCallSiteTest extends DDSpecification {
         advices
         .computeIfAbsent(owner, t -> [:])
         .computeIfAbsent(method, m -> [:])
-        .put(descriptor, advice)
+        .put(descriptor, Advices.TypedAdvice.withType(advice, type))
       }
       addHelpers(_ as String[]) >> {
         Collections.addAll(helpers, it[0] as String[])
@@ -83,6 +83,9 @@ class BaseCallSiteTest extends DDSpecification {
       getHelpers() >> {
         helpers as String[]
       }
+      typeOf(_ as CallSiteAdvice) >> {
+        ((Advices.TypedAdvice) it[0]).type
+      }
     }
   }
 

dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/csi/CallSiteInstrumentationTest.groovy

@@ -68,7 +68,7 @@ class CallSiteInstrumentationTest extends BaseCallSiteTest {
     0 * builder.visit(_ as AsmVisitorWrapper) >> builder
   }
 
-  void 'test call site transformer with super call in ctor'() {
+  void 'test call site transformer with super call in ctor (#test)'() {
     setup:
     SuperInCtorExampleAdvice.CALLS.set(0)
     final source = Type.getType(SuperInCtorExample)
@@ -91,11 +91,16 @@ class CallSiteInstrumentationTest extends BaseCallSiteTest {
     when:
     final transformedClass = transformType(source, target, callSiteTransformer)
     final transformed = loadClass(target, transformedClass)
-    final reader = transformed.newInstance("test")
+    final reader = transformed.newInstance(param)
 
     then:
     reader != null
     SuperInCtorExampleAdvice.CALLS.get() > 0
+
+    where:
+    param                     | test
+    "test"                    | "Operand stack underflow"
+    new StringBuilder("test") | "Inconsistent stackmap frames"
   }
 
   static class StringCallSites implements CallSites, TestCallSites {

dd-java-agent/agent-tooling/src/test/java/datadog/trace/agent/tooling/csi/SuperInCtorExample.java

@@ -5,6 +5,15 @@
 public class SuperInCtorExample extends StringReader {
 
   public SuperInCtorExample(String s) {
+    // triggers APPSEC-55918
     super(s + new StringReader(s + "Test" + new StringBuilder("another test")));
   }
+
+  public SuperInCtorExample(StringBuilder s) {
+    super(s.toString());
+    // triggers APPSEC-58131
+    if (s.length() == 0) {
+      throw new IllegalArgumentException();
+    }
+  }
 }

dd-java-agent/instrumentation/java-io/src/test/groovy/datadog/trace/instrumentation/java/io/InputStreamReaderCallSiteTest.groovy

@@ -2,6 +2,7 @@ package datadog.trace.instrumentation.java.io
 
 import datadog.trace.api.iast.InstrumentationBridge
 import datadog.trace.api.iast.propagation.PropagationModule
+import foo.bar.TestCustomInputStreamReader
 import foo.bar.TestInputStreamReaderSuite
 
 import java.nio.charset.Charset
@@ -27,4 +28,21 @@ class InputStreamReaderCallSiteTest extends  BaseIoCallSiteTest{
       [new ByteArrayInputStream("test".getBytes())]// Reader input
     ]
   }
+
+  void 'test InputStreamReader.<init> with super call and parameter'(){
+    // XXX: Do not modify the constructor call here. Regression test for APPSEC-58131.
+    given:
+    PropagationModule iastModule = Mock(PropagationModule)
+    InstrumentationBridge.registerIastModule(iastModule)
+
+    when:
+    new TestCustomInputStreamReader(*args)
+
+    then:
+    1 * iastModule.taintObjectIfTainted(_ as InputStreamReader, _ as InputStream)
+    0 * _
+
+    where:
+    args << [[new ByteArrayInputStream("test".getBytes()), Charset.defaultCharset()],]
+  }
 }

dd-java-agent/instrumentation/java-io/src/test/java/foo/bar/TestCustomInputStreamReader.java

@@ -0,0 +1,26 @@
+package foo.bar;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.nio.charset.Charset;
+
+public class TestCustomInputStreamReader extends InputStreamReader {
+
+  public TestCustomInputStreamReader(final InputStream in) throws IOException {
+    super(in);
+  }
+
+  public TestCustomInputStreamReader(final InputStream in, final Charset charset)
+      throws IOException {
+    // XXX: DO NOT MODIFY THIS CODE. This is testing a very specific error (APPSEC-58131).
+    // This caused the following error:
+    //   VerifyError: Inconsistent stackmap frames at branch target \d
+    //   Reason: urrent frame's stack size doesn't match stackmap.
+    // To trigger this, it is necessary to consume an argument after the super call.
+    super(in, charset);
+    if (charset != null) {
+      System.out.println("Using charset: " + charset.name());
+    }
+  }
+}