Fix obfuscated libs instrumentation (adcolony) (#307)

CHANGELOG.md

@@ -2,6 +2,8 @@
 
 ## Unreleased
 
+* Fix: obfuscated libs instrumentation (adcolony) (#307)
+
 ## 3.0.1
 
 * Fix: obfuscated libs instrumentation (gms) (#303)

plugin-build/src/main/kotlin/io/sentry/android/gradle/instrumentation/wrap/visitor/WrappingVisitor.kt

@@ -15,6 +15,7 @@ import org.objectweb.asm.commons.Method
 import org.objectweb.asm.tree.InsnNode
 import org.objectweb.asm.tree.MethodNode
 import org.objectweb.asm.tree.TypeInsnNode
+import org.objectweb.asm.tree.VarInsnNode
 import org.slf4j.Logger
 
 class WrappingVisitor(
@@ -46,18 +47,26 @@ class WrappingVisitor(
     }
     private val className = classContext.className.replace('.', '/')
 
-    private var newWithoutDupInsn = false
+    private var manuallyDuped = false
     private var varIndex = -1
 
     override fun visitTypeInsn(opcode: Int, type: String?) {
         super.visitTypeInsn(opcode, type)
         if (opcode == Opcodes.NEW && type in targetTypes) {
-            val nextInsn = newInsnsForTargets.poll()?.next
+            val nextInsn = newInsnsForTargets.poll()?.next ?: return
             // in case the next insn after NEW is not a DUP, we inject our own DUP and flip a flag
             // to later store our wrapping instance with the same index as the original instance
-            if (nextInsn != null && (nextInsn as? InsnNode)?.opcode != Opcodes.DUP) {
+            val isNextInsnDup = (nextInsn as? InsnNode)?.opcode == Opcodes.DUP
+            // in case the next insn after NEW is DUP, but is followed by ASTORE, we do the same
+            // thing as above. This case cannot be written by a developer, but rather is produced
+            // by a code obfuscator/compiler, because the usage of uninitialized instance
+            // is prohibited by the java verifier (NEW should be followed by INVOKESPECIAL before
+            // using it, which is not the case with ASTORE)
+            val isDupFollowedByStore =
+                isNextInsnDup && (nextInsn.next as? VarInsnNode)?.opcode == Opcodes.ASTORE
+            if (!isNextInsnDup || isDupFollowedByStore) {
                 dup()
-                newWithoutDupInsn = true
+                manuallyDuped = true
             }
         }
     }
@@ -66,7 +75,7 @@ class WrappingVisitor(
         super.visitVarInsn(opcode, `var`)
         // capture the variable index of the instrumented type to later store our wrapped type
         // with the same index
-        if (opcode == Opcodes.ASTORE && newWithoutDupInsn && varIndex == -1) {
+        if (opcode == Opcodes.ASTORE && manuallyDuped && varIndex == -1) {
             varIndex = `var`
         }
     }
@@ -177,10 +186,10 @@ class WrappingVisitor(
             replacement.descriptor,
             false
         )
-        if (newWithoutDupInsn && varIndex >= 0) {
+        if (manuallyDuped && varIndex >= 0) {
             mv.visitVarInsn(Opcodes.ASTORE, varIndex)
             varIndex = -1
-            newWithoutDupInsn = false
+            manuallyDuped = false
         }
     }
 }

plugin-build/src/test/kotlin/io/sentry/android/gradle/SentryPluginWithMinifiedLibsTest.kt

@@ -21,6 +21,7 @@ class SentryPluginWithMinifiedLibsTest :
               implementation 'com.google.android.play:core-ktx:1.8.1'
               implementation 'com.google.android.gms:play-services-vision:20.1.3'
               implementation 'com.google.android.gms:play-services-mlkit-text-recognition:18.0.0'
+              implementation 'com.adcolony:sdk:4.7.1'
             }
             """.trimIndent()
         )

plugin-build/src/test/kotlin/io/sentry/android/gradle/instrumentation/wrap/visitor/WrappingVisitorTest.kt

@@ -319,6 +319,49 @@ class WrappingVisitorTest {
                 fixture.visitor.insnVisits.first() == InsnVisit(Opcodes.DUP)
         }
     }
+
+    @Test
+    fun `when NEW insn with DUP followed by ASTORE - modifies operand stack with DUP and ASTORE`() {
+        val methodVisit = MethodVisit(
+            opcode = Opcodes.INVOKESPECIAL,
+            owner = "java/io/FileInputStream",
+            name = "<init>",
+            descriptor = "(Ljava/lang/String;)V",
+            isInterface = false
+        )
+        val firstPassVisitor = MethodNode(Opcodes.ASM9).apply {
+            visitTypeInsn(Opcodes.NEW, "java/io/FileInputStream")
+            visitInsn(Opcodes.DUP)
+            visitVarInsn(Opcodes.ASTORE, 1)
+        }
+
+        fixture.getSut(
+            replacements = mapOf(Replacement.FileInputStream.STRING),
+            firstPassVisitor = firstPassVisitor
+        ).run {
+            visitTypeInsn(Opcodes.NEW, "java/io/FileInputStream")
+            visitInsn(Opcodes.DUP)
+            visitVarInsn(Opcodes.ASTORE, 1)
+            visitMethodInsn(
+                methodVisit.opcode,
+                methodVisit.owner,
+                methodVisit.name,
+                methodVisit.descriptor,
+                methodVisit.isInterface
+            )
+        }
+
+        // DUP was visited by our visitor
+        assertTrue {
+            fixture.visitor.insnVisits.size == 2 &&
+                fixture.visitor.insnVisits.all { it == InsnVisit(Opcodes.DUP) }
+        }
+        // ASTORE was visited by our visitor in the end
+        assertTrue {
+            fixture.visitor.varVisits.size == 5 &&
+                fixture.visitor.varVisits.last() == VarVisit(Opcodes.ASTORE, 1)
+        }
+    }
 }
 
 data class MethodVisit(