From 847225a6a94797d4ca1d4dd43efe814e43a0342d Mon Sep 17 00:00:00 2001 From: Skylot <118523+skylot@users.noreply.github.com> Date: Sat, 17 Aug 2024 20:45:40 +0100 Subject: [PATCH] fix: improve try/catch temp edges injection (#2247) --- .../dex/visitors/blocks/BlockSplitter.java | 8 ++- .../java/jadx/core/utils/DebugChecks.java | 51 ++++++++++++++----- .../integration/trycatch/TestTryCatch10.java | 17 +++++++ .../test/smali/trycatch/TestTryCatch10.smali | 44 ++++++++++++++++ 4 files changed, 104 insertions(+), 16 deletions(-) create mode 100644 jadx-core/src/test/java/jadx/tests/integration/trycatch/TestTryCatch10.java create mode 100644 jadx-core/src/test/smali/trycatch/TestTryCatch10.smali diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/blocks/BlockSplitter.java b/jadx-core/src/main/java/jadx/core/dex/visitors/blocks/BlockSplitter.java index 6c89a8131..c10f28c18 100644 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/blocks/BlockSplitter.java +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/blocks/BlockSplitter.java @@ -65,7 +65,6 @@ public class BlockSplitter extends AbstractVisitor { Map blocksMap = splitBasicBlocks(mth); setupConnectionsFromJumps(mth, blocksMap); initBlocksInTargetNodes(mth); - addTempConnectionsForExcHandlers(mth, blocksMap); expandMoveMulti(mth); if (mth.contains(AFlag.RESOLVE_JAVA_JSR)) { @@ -76,6 +75,8 @@ public class BlockSplitter extends AbstractVisitor { removeInsns(mth); removeEmptyDetachedBlocks(mth); mth.getBasicBlocks().removeIf(BlockSplitter::removeEmptyBlock); + + addTempConnectionsForExcHandlers(mth, blocksMap); setupExitConnections(mth); mth.unloadInsnArr(); @@ -257,10 +258,13 @@ public class BlockSplitter extends AbstractVisitor { /** * Connect exception handlers to the throw block. - * This temporary connection needed to build close to final dominators tree. + * This temporary connection is necessary to build close to a final dominator tree. * Will be used and removed in {@code jadx.core.dex.visitors.blocks.BlockExceptionHandler} */ private static void addTempConnectionsForExcHandlers(MethodNode mth, Map blocksMap) { + if (mth.isNoExceptionHandlers()) { + return; + } for (BlockNode block : mth.getBasicBlocks()) { for (InsnNode insn : block.getInstructions()) { CatchAttr catchAttr = insn.get(AType.EXC_CATCH); diff --git a/jadx-core/src/main/java/jadx/core/utils/DebugChecks.java b/jadx-core/src/main/java/jadx/core/utils/DebugChecks.java index 2a223283c..05fb79950 100644 --- a/jadx-core/src/main/java/jadx/core/utils/DebugChecks.java +++ b/jadx-core/src/main/java/jadx/core/utils/DebugChecks.java @@ -4,10 +4,12 @@ import java.util.ArrayList; import java.util.HashSet; import java.util.List; import java.util.Set; +import java.util.function.Supplier; import jadx.core.dex.attributes.AFlag; import jadx.core.dex.attributes.AType; import jadx.core.dex.attributes.nodes.PhiListAttr; +import jadx.core.dex.attributes.nodes.TmpEdgeAttr; import jadx.core.dex.instructions.IfNode; import jadx.core.dex.instructions.InsnType; import jadx.core.dex.instructions.PhiInsn; @@ -61,14 +63,14 @@ public class DebugChecks { } for (BlockNode block : basicBlocks) { for (InsnNode insn : block.getInstructions()) { - checkInsn(mth, insn); + checkInsn(mth, block, insn); } } checkSSAVars(mth); // checkPHI(mth); } - private static void checkInsn(MethodNode mth, InsnNode insn) { + private static void checkInsn(MethodNode mth, BlockNode block, InsnNode insn) { if (insn.getResult() != null) { checkVar(mth, insn, insn.getResult()); } @@ -77,24 +79,45 @@ public class DebugChecks { checkVar(mth, insn, (RegisterArg) arg); } else if (arg.isInsnWrap()) { InsnNode wrapInsn = ((InsnWrapArg) arg).getWrapInsn(); - checkInsn(mth, wrapInsn); + checkInsn(mth, block, wrapInsn); } } - if (insn instanceof TernaryInsn) { - TernaryInsn ternaryInsn = (TernaryInsn) insn; - for (RegisterArg arg : ternaryInsn.getCondition().getRegisterArgs()) { - checkVar(mth, insn, arg); - } - } else if (insn instanceof IfNode) { - IfNode ifNode = (IfNode) insn; - checkBlock(mth, ifNode.getThenBlock()); - checkBlock(mth, ifNode.getElseBlock()); + switch (insn.getType()) { + case TERNARY: + TernaryInsn ternaryInsn = (TernaryInsn) insn; + for (RegisterArg arg : ternaryInsn.getCondition().getRegisterArgs()) { + checkVar(mth, insn, arg); + } + break; + + case IF: + IfNode ifNode = (IfNode) insn; + if (!ifNode.getThenBlock().equals(ifNode.getElseBlock())) { + // exclude temp edges + int branches = (int) block.getSuccessors().stream().filter(b -> !hasTmpEdge(block, b)).count(); + if (branches != 2) { + DebugUtils.dumpRaw(mth, "error"); + throw new JadxRuntimeException( + "Incorrect if block successors count: " + branches + " (expect 2), block: " + block); + } + } + checkBlock(mth, ifNode.getThenBlock(), () -> "then block in if insn: " + ifNode); + checkBlock(mth, ifNode.getElseBlock(), () -> "else block in if insn: " + ifNode); + break; } } - private static void checkBlock(MethodNode mth, BlockNode block) { + private static boolean hasTmpEdge(BlockNode start, BlockNode end) { + TmpEdgeAttr tmpEdgeAttr = end.get(AType.TMP_EDGE); + if (tmpEdgeAttr == null) { + return false; + } + return tmpEdgeAttr.getBlock().equals(start); + } + + private static void checkBlock(MethodNode mth, BlockNode block, Supplier source) { if (!mth.getBasicBlocks().contains(block)) { - throw new JadxRuntimeException("Block not registered in method: " + block); + throw new JadxRuntimeException("Block not registered in method: " + block + " from " + source.get()); } } diff --git a/jadx-core/src/test/java/jadx/tests/integration/trycatch/TestTryCatch10.java b/jadx-core/src/test/java/jadx/tests/integration/trycatch/TestTryCatch10.java new file mode 100644 index 000000000..fae00170c --- /dev/null +++ b/jadx-core/src/test/java/jadx/tests/integration/trycatch/TestTryCatch10.java @@ -0,0 +1,17 @@ +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; + +public class TestTryCatch10 extends SmaliTest { + + @Test + public void test() { + assertThat(getClassNodeFromSmali()) + .code() + .countString(3, "return false;"); + } +} diff --git a/jadx-core/src/test/smali/trycatch/TestTryCatch10.smali b/jadx-core/src/test/smali/trycatch/TestTryCatch10.smali new file mode 100644 index 000000000..b357c65c4 --- /dev/null +++ b/jadx-core/src/test/smali/trycatch/TestTryCatch10.smali @@ -0,0 +1,44 @@ +.class public Ltrycatch/TestTryCatch10; +.super Ljava/lang/Object; + +.field public static VERSION:I + +.method public static test(I)Z + .registers 5 + + sget v0, Ltrycatch/TestTryCatch10;->VERSION:I + const/16 v1, 0x1d + const/4 v2, 0x0 + if-lt v0, v1, :cond_1b + const-string v0, "custom" + invoke-static {v0}, Ltrycatch/TestTryCatch10;->check(Ljava/lang/String;)Z + move-result v0 + if-nez v0, :cond_10 + goto :goto_1b + + :cond_10 + :try_start_10 + invoke-static {p0}, Ltrycatch/TestTryCatch10;->getVar(I)I + move-result p0 + :try_end_18 + .catch Ljava/lang/Exception; {:try_start_10 .. :try_end_18} :catch_1b + + if-eqz p0, :cond_1b + const/4 v2, 0x1 + + :catch_1b + :cond_1b + :goto_1b + return v2 +.end method + +.method public static getVar(I)I + .locals 0 + return p0 +.end method + +.method public static check(Ljava/lang/String;)Z + .locals 1 + const/4 v0, 0x0 + return v0 +.end method