fix: check variables before merge in finally block (#1592)

jadx-core/src/main/java/jadx/core/dex/visitors/finaly/FinallyExtractInfo.java

@@ -6,6 +6,7 @@ import java.util.List;
 import java.util.Set;
 
 import jadx.core.dex.nodes.BlockNode;
+import jadx.core.dex.nodes.InsnNode;
 import jadx.core.dex.nodes.MethodNode;
 import jadx.core.dex.trycatch.ExceptionHandler;
 import jadx.core.utils.Utils;
@@ -19,6 +20,10 @@ public class FinallyExtractInfo {
 	private final InsnsSlice finallyInsnsSlice = new InsnsSlice();
 	private final BlockNode startBlock;
 
+	private InsnsSlice curDupSlice;
+	private List<InsnNode> curDupInsns;
+	private int curDupInsnsOffset;
+
 	public FinallyExtractInfo(MethodNode mth, ExceptionHandler finallyHandler, BlockNode startBlock, List<BlockNode> allHandlerBlocks) {
 		this.mth = mth;
 		this.finallyHandler = finallyHandler;
@@ -54,6 +59,27 @@ public class FinallyExtractInfo {
 		return startBlock;
 	}
 
+	public InsnsSlice getCurDupSlice() {
+		return curDupSlice;
+	}
+
+	public void setCurDupSlice(InsnsSlice curDupSlice) {
+		this.curDupSlice = curDupSlice;
+	}
+
+	public List<InsnNode> getCurDupInsns() {
+		return curDupInsns;
+	}
+
+	public int getCurDupInsnsOffset() {
+		return curDupInsnsOffset;
+	}
+
+	public void setCurDupInsns(List<InsnNode> insns, int offset) {
+		this.curDupInsns = insns;
+		this.curDupInsnsOffset = offset;
+	}
+
 	@Override
 	public String toString() {
 		return "FinallyExtractInfo{"

jadx-core/src/main/java/jadx/core/dex/visitors/finaly/MarkFinallyVisitor.java

@@ -12,6 +12,7 @@ import org.slf4j.LoggerFactory;
 import jadx.core.Consts;
 import jadx.core.dex.attributes.AFlag;
 import jadx.core.dex.attributes.AType;
+import jadx.core.dex.attributes.nodes.RegDebugInfoAttr;
 import jadx.core.dex.instructions.InsnType;
 import jadx.core.dex.instructions.args.InsnArg;
 import jadx.core.dex.instructions.args.RegisterArg;
@@ -28,6 +29,7 @@ import jadx.core.dex.visitors.IDexTreeVisitor;
 import jadx.core.dex.visitors.JadxVisitor;
 import jadx.core.dex.visitors.ssa.SSATransform;
 import jadx.core.utils.BlockUtils;
+import jadx.core.utils.InsnList;
 import jadx.core.utils.ListUtils;
 import jadx.core.utils.Utils;
 
@@ -376,6 +378,7 @@ public class MarkFinallyVisitor extends AbstractVisitor {
 	 * 'Finally' instructions can start in the middle of the first block.
 	 */
 	private static InsnsSlice isStartBlock(BlockNode dupBlock, BlockNode finallyBlock, FinallyExtractInfo extractInfo) {
+		extractInfo.setCurDupSlice(null);
 		List<InsnNode> dupInsns = dupBlock.getInstructions();
 		List<InsnNode> finallyInsns = finallyBlock.getInstructions();
 		int dupSize = dupInsns.size();
@@ -386,7 +389,7 @@ public class MarkFinallyVisitor extends AbstractVisitor {
 		int startPos;
 		int endPos = 0;
 		if (dupSize == finSize) {
-			if (!checkInsns(dupInsns, finallyInsns, 0)) {
+			if (!checkInsns(extractInfo, dupInsns, finallyInsns, 0)) {
 				return null;
 			}
 			startPos = 0;
@@ -394,11 +397,11 @@ public class MarkFinallyVisitor extends AbstractVisitor {
 			// dupSize > finSize
 			startPos = dupSize - finSize;
 			// fast check from end of block
-			if (!checkInsns(dupInsns, finallyInsns, startPos)) {
+			if (!checkInsns(extractInfo, dupInsns, finallyInsns, startPos)) {
 				// search start insn
 				boolean found = false;
 				for (int i = 1; i < startPos; i++) {
-					if (checkInsns(dupInsns, finallyInsns, i)) {
+					if (checkInsns(extractInfo, dupInsns, finallyInsns, i)) {
 						startPos = i;
 						endPos = finSize + i;
 						found = true;
@@ -414,6 +417,7 @@ public class MarkFinallyVisitor extends AbstractVisitor {
 		// put instructions into slices
 		boolean complete;
 		InsnsSlice slice = new InsnsSlice();
+		extractInfo.setCurDupSlice(slice);
 		int endIndex;
 		if (endPos != 0) {
 			endIndex = endPos + 1;
@@ -453,11 +457,12 @@ public class MarkFinallyVisitor extends AbstractVisitor {
 		return slice;
 	}
 
-	private static boolean checkInsns(List<InsnNode> remInsns, List<InsnNode> finallyInsns, int delta) {
+	private static boolean checkInsns(FinallyExtractInfo extractInfo, List<InsnNode> dupInsns, List<InsnNode> finallyInsns, int delta) {
+		extractInfo.setCurDupInsns(dupInsns, delta);
 		for (int i = finallyInsns.size() - 1; i >= 0; i--) {
 			InsnNode startInsn = finallyInsns.get(i);
-			InsnNode remInsn = remInsns.get(delta + i);
-			if (!sameInsns(remInsn, startInsn)) {
+			InsnNode dupInsn = dupInsns.get(delta + i);
+			if (!sameInsns(extractInfo, dupInsn, startInsn)) {
 				return false;
 			}
 		}
@@ -509,8 +514,9 @@ public class MarkFinallyVisitor extends AbstractVisitor {
 		if (dupInsnCount < finallyInsnCount) {
 			return false;
 		}
+		extractInfo.setCurDupInsns(dupInsns, 0);
 		for (int i = 0; i < finallyInsnCount; i++) {
-			if (!sameInsns(dupInsns.get(i), finallyInsns.get(i))) {
+			if (!sameInsns(extractInfo, dupInsns.get(i), finallyInsns.get(i))) {
 				return false;
 			}
 		}
@@ -524,26 +530,85 @@ public class MarkFinallyVisitor extends AbstractVisitor {
 		return true;
 	}
 
-	private static boolean sameInsns(InsnNode remInsn, InsnNode fInsn) {
-		if (!remInsn.isSame(fInsn)) {
+	private static boolean sameInsns(FinallyExtractInfo extractInfo, InsnNode dupInsn, InsnNode fInsn) {
+		if (!dupInsn.isSame(fInsn)) {
 			return false;
 		}
 		// TODO: check instance arg in ConstructorInsn
-		// TODO: compare literals
-		for (int i = 0; i < remInsn.getArgsCount(); i++) {
-			InsnArg remArg = remInsn.getArg(i);
+		for (int i = 0; i < dupInsn.getArgsCount(); i++) {
+			InsnArg dupArg = dupInsn.getArg(i);
 			InsnArg fArg = fInsn.getArg(i);
-			if (remArg.isRegister() != fArg.isRegister()) {
+			if (!isSameArgs(extractInfo, dupArg, fArg)) {
 				return false;
 			}
-			boolean remConst = remArg.isConst();
-			if (remConst != fArg.isConst()) {
-				return false;
-			}
-			if (remConst && !remArg.isSameConst(fArg)) {
+		}
+		return true;
+	}
+
+	@SuppressWarnings("RedundantIfStatement")
+	private static boolean isSameArgs(FinallyExtractInfo extractInfo, InsnArg dupArg, InsnArg fArg) {
+		boolean isReg = dupArg.isRegister();
+		if (isReg != fArg.isRegister()) {
+			return false;
+		}
+		if (isReg) {
+			RegisterArg dupReg = (RegisterArg) dupArg;
+			RegisterArg fReg = (RegisterArg) fArg;
+			if (!dupReg.sameCodeVar(fReg)
+					&& !sameDebugInfo(dupReg, fReg)
+					&& assignedOutsideHandler(extractInfo, dupReg, fReg)
+					&& assignInsnDifferent(dupReg, fReg)) {
 				return false;
 			}
 		}
+		boolean remConst = dupArg.isConst();
+		if (remConst != fArg.isConst()) {
+			return false;
+		}
+		if (remConst && !dupArg.isSameConst(fArg)) {
+			return false;
+		}
+		return true;
+	}
+
+	private static boolean sameDebugInfo(RegisterArg dupReg, RegisterArg fReg) {
+		RegDebugInfoAttr fDbgInfo = fReg.get(AType.REG_DEBUG_INFO);
+		RegDebugInfoAttr dupDbgInfo = dupReg.get(AType.REG_DEBUG_INFO);
+		if (fDbgInfo == null || dupDbgInfo == null) {
+			return false;
+		}
+		return dupDbgInfo.equals(fDbgInfo);
+	}
+
+	private static boolean assignInsnDifferent(RegisterArg dupReg, RegisterArg fReg) {
+		InsnNode assignInsn = fReg.getAssignInsn();
+		InsnNode dupAssign = dupReg.getAssignInsn();
+		if (assignInsn == null || dupAssign == null) {
+			return true;
+		}
+		if (!assignInsn.isSame(dupAssign)) {
+			return true;
+		}
+		if (assignInsn.isConstInsn() && dupAssign.isConstInsn()) {
+			return !assignInsn.isDeepEquals(dupAssign);
+		}
+		return false;
+	}
+
+	@SuppressWarnings("RedundantIfStatement")
+	private static boolean assignedOutsideHandler(FinallyExtractInfo extractInfo, RegisterArg dupReg, RegisterArg fReg) {
+		if (InsnList.contains(extractInfo.getFinallyInsnsSlice().getInsnsList(), fReg.getAssignInsn())) {
+			return false;
+		}
+		InsnNode dupAssign = dupReg.getAssignInsn();
+		InsnsSlice curDupSlice = extractInfo.getCurDupSlice();
+		if (curDupSlice != null && InsnList.contains(curDupSlice.getInsnsList(), dupAssign)) {
+			return false;
+		}
+		List<InsnNode> curDupInsns = extractInfo.getCurDupInsns();
+		if (Utils.notEmpty(curDupInsns) && InsnList.contains(curDupInsns, dupAssign, extractInfo.getCurDupInsnsOffset())) {
+			return false;
+		}
 		return true;
 	}
 

jadx-core/src/main/java/jadx/core/utils/InsnList.java

@@ -29,8 +29,12 @@ public final class InsnList implements Iterable<InsnNode> {
 	}
 
 	public static int getIndex(List<InsnNode> list, InsnNode insn) {
+		return getIndex(list, insn, 0);
+	}
+
+	public static int getIndex(List<InsnNode> list, InsnNode insn, int startOffset) {
 		int size = list.size();
-		for (int i = 0; i < size; i++) {
+		for (int i = startOffset; i < size; i++) {
 			if (list.get(i) == insn) {
 				return i;
 			}
@@ -38,6 +42,14 @@ public final class InsnList implements Iterable<InsnNode> {
 		return -1;
 	}
 
+	public static boolean contains(List<InsnNode> list, InsnNode insn) {
+		return getIndex(list, insn, 0) != -1;
+	}
+
+	public static boolean contains(List<InsnNode> list, InsnNode insn, int startOffset) {
+		return getIndex(list, insn, startOffset) != -1;
+	}
+
 	public int getIndex(InsnNode insn) {
 		return getIndex(list, insn);
 	}

jadx-core/src/test/java/jadx/tests/integration/trycatch/TestTryCatchFinally15.java

+package jadx.tests.integration.trycatch;
+
+import org.junit.jupiter.api.Test;
+
+import jadx.tests.api.SmaliTest;
+
+import static jadx.tests.api.utils.assertj.JadxAssertions.assertThat;
+
+/**
+ * Negative test case for finally extract (issue 1592).
+ * Different registers incorrectly merged into one.
+ */
+@SuppressWarnings({ "CommentedOutCode", "GrazieInspection" })
+public class TestTryCatchFinally15 extends SmaliTest {
+
+	// @formatter:off
+	/*
+		protected final Parcel test(int i, Parcel parcel) throws RemoteException {
+			Parcel obtain = Parcel.obtain();
+			try {
+				try {
+					this.zza.transact(i, parcel, obtain, 0);
+					obtain.readException();
+					return obtain;
+				} catch (RuntimeException e) {
+					obtain.recycle();
+					throw e;
+				}
+			} finally {
+				parcel.recycle();
+			}
+		}
+	*/
+	// @formatter:on
+
+	@Test
+	public void test() {
+		disableCompilation();
+		assertThat(getClassNodeFromSmali())
+				.code()
+				.doesNotContain("parcel = Parcel.obtain();")
+				.containsOne("this.zza.transact(i, parcel, obtain, 0);");
+	}
+}

jadx-core/src/test/java/jadx/tests/integration/trycatch/TestTryCatchFinally6.java

@@ -10,6 +10,7 @@ import jadx.core.dex.nodes.ClassNode;
 import jadx.tests.api.IntegrationTest;
 
 import static jadx.tests.api.utils.JadxMatchers.containsLines;
+import static jadx.tests.api.utils.JadxMatchers.containsOne;
 import static org.hamcrest.MatcherAssert.assertThat;
 
 public class TestTryCatchFinally6 extends IntegrationTest {
@@ -54,15 +55,7 @@ public class TestTryCatchFinally6 extends IntegrationTest {
 		ClassNode cls = getClassNode(TestCls.class);
 		String code = cls.getCode().toString();
 
-		assertThat(code, containsLines(2,
-				"FileInputStream fileInputStream = null;",
-				"try {",
-				indent() + "call();",
-				indent() + "fileInputStream = new FileInputStream(\"1.txt\");",
-				"} finally {",
-				indent() + "if (fileInputStream != null) {",
-				indent() + indent() + "fileInputStream.close();",
-				indent() + '}',
-				"}"));
+		// impossible to proof that variables should be merged, so can't restore finally block here
+		assertThat(code, containsOne("if (0 != 0) {"));
 	}
 }

jadx-core/src/test/smali/trycatch/TestTryCatchFinally15.smali

+.class public Ltrycatch/TestTryCatchFinally15;
+.super Ljava/lang/Object;
+
+.implements Landroid/os/IInterface;
+
+.field private final zza:Landroid/os/IBinder;
+.field private final zzb:Ljava/lang/String;
+
+.method protected final test(ILandroid/os/Parcel;)Landroid/os/Parcel;
+    .registers 6
+    .annotation system Ldalvik/annotation/Throws;
+        value = {
+            Landroid/os/RemoteException;
+        }
+    .end annotation
+
+    .line 1
+    invoke-static {}, Landroid/os/Parcel;->obtain()Landroid/os/Parcel;
+    move-result-object v0
+
+    :try_start_4
+    iget-object v1, p0, Ltrycatch/TestTryCatchFinally15;->zza:Landroid/os/IBinder;
+    const/4 v2, 0x0
+
+    .line 2
+    invoke-interface {v1, p1, p2, v0, v2}, Landroid/os/IBinder;->transact(ILandroid/os/Parcel;Landroid/os/Parcel;I)Z
+    .line 3
+    invoke-virtual {v0}, Landroid/os/Parcel;->readException()V
+    :try_end_d
+    .catch Ljava/lang/RuntimeException; {:try_start_4 .. :try_end_d} :catch_13
+    .catchall {:try_start_4 .. :try_end_d} :catchall_11
+
+    .line 6
+    invoke-virtual {p2}, Landroid/os/Parcel;->recycle()V
+    return-object v0
+
+    :catchall_11
+    move-exception p1
+    goto :goto_18
+
+    .line 5
+    :catch_13
+    move-exception p1
+
+    .line 4
+    :try_start_14
+    invoke-virtual {v0}, Landroid/os/Parcel;->recycle()V
+
+    .line 5
+    throw p1
+    :try_end_18
+    .catchall {:try_start_14 .. :try_end_18} :catchall_11
+
+    .line 6
+    :goto_18
+    invoke-virtual {p2}, Landroid/os/Parcel;->recycle()V
+
+    .line 7
+    throw p1
+.end method