diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/finaly/FinallyExtractInfo.java b/jadx-core/src/main/java/jadx/core/dex/visitors/finaly/FinallyExtractInfo.java index 44b44b6b6..7974cbe24 100644 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/finaly/FinallyExtractInfo.java +++ b/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 curDupInsns; + private int curDupInsnsOffset; + public FinallyExtractInfo(MethodNode mth, ExceptionHandler finallyHandler, BlockNode startBlock, List 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 getCurDupInsns() { + return curDupInsns; + } + + public int getCurDupInsnsOffset() { + return curDupInsnsOffset; + } + + public void setCurDupInsns(List insns, int offset) { + this.curDupInsns = insns; + this.curDupInsnsOffset = offset; + } + @Override public String toString() { return "FinallyExtractInfo{" diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/finaly/MarkFinallyVisitor.java b/jadx-core/src/main/java/jadx/core/dex/visitors/finaly/MarkFinallyVisitor.java index 354c82adf..f7f57385a 100644 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/finaly/MarkFinallyVisitor.java +++ b/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 dupInsns = dupBlock.getInstructions(); List 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 remInsns, List finallyInsns, int delta) { + private static boolean checkInsns(FinallyExtractInfo extractInfo, List dupInsns, List 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 curDupInsns = extractInfo.getCurDupInsns(); + if (Utils.notEmpty(curDupInsns) && InsnList.contains(curDupInsns, dupAssign, extractInfo.getCurDupInsnsOffset())) { + return false; + } return true; } diff --git a/jadx-core/src/main/java/jadx/core/utils/InsnList.java b/jadx-core/src/main/java/jadx/core/utils/InsnList.java index 39c37abe1..96f44ae96 100644 --- a/jadx-core/src/main/java/jadx/core/utils/InsnList.java +++ b/jadx-core/src/main/java/jadx/core/utils/InsnList.java @@ -29,8 +29,12 @@ public final class InsnList implements Iterable { } public static int getIndex(List list, InsnNode insn) { + return getIndex(list, insn, 0); + } + + public static int getIndex(List 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 { return -1; } + public static boolean contains(List list, InsnNode insn) { + return getIndex(list, insn, 0) != -1; + } + + public static boolean contains(List list, InsnNode insn, int startOffset) { + return getIndex(list, insn, startOffset) != -1; + } + public int getIndex(InsnNode insn) { return getIndex(list, insn); } diff --git a/jadx-core/src/test/java/jadx/tests/integration/trycatch/TestTryCatchFinally15.java b/jadx-core/src/test/java/jadx/tests/integration/trycatch/TestTryCatchFinally15.java new file mode 100644 index 000000000..6860e7721 --- /dev/null +++ b/jadx-core/src/test/java/jadx/tests/integration/trycatch/TestTryCatchFinally15.java @@ -0,0 +1,44 @@ +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);"); + } +} diff --git a/jadx-core/src/test/java/jadx/tests/integration/trycatch/TestTryCatchFinally6.java b/jadx-core/src/test/java/jadx/tests/integration/trycatch/TestTryCatchFinally6.java index 8ea6f84c9..8f3e127e5 100644 --- a/jadx-core/src/test/java/jadx/tests/integration/trycatch/TestTryCatchFinally6.java +++ b/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) {")); } } diff --git a/jadx-core/src/test/smali/trycatch/TestTryCatchFinally15.smali b/jadx-core/src/test/smali/trycatch/TestTryCatchFinally15.smali new file mode 100644 index 000000000..cc75fad10 --- /dev/null +++ b/jadx-core/src/test/smali/trycatch/TestTryCatchFinally15.smali @@ -0,0 +1,60 @@ +.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