From eedf32d1979cd772f7209d5b00e76d368dfcc2c8 Mon Sep 17 00:00:00 2001 From: Skylot Date: Thu, 9 Sep 2021 18:44:24 +0100 Subject: [PATCH] fix: support nested try/finally blocks (#1249) --- .../dex/visitors/AttachTryCatchVisitor.java | 13 +- .../visitors/finaly/MarkFinallyVisitor.java | 65 ++++++---- .../trycatch/TestTryCatchFinally12.java | 111 ++++++++++++++++++ 3 files changed, 162 insertions(+), 27 deletions(-) create mode 100644 jadx-core/src/test/java/jadx/tests/integration/trycatch/TestTryCatchFinally12.java diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/AttachTryCatchVisitor.java b/jadx-core/src/main/java/jadx/core/dex/visitors/AttachTryCatchVisitor.java index 423e11e44..d3060b5db 100644 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/AttachTryCatchVisitor.java +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/AttachTryCatchVisitor.java @@ -69,7 +69,7 @@ public class AttachTryCatchVisitor extends AbstractVisitor { InsnNode insnAtOffset = insnByOffset[offset]; if (insnAtOffset != null) { insn = insnAtOffset; - insn.addAttr(catchAttr); + attachCatchAttr(catchAttr, insn); if (!tryBlockStarted) { insn.add(AFlag.TRY_ENTER); tryBlockStarted = true; @@ -91,6 +91,17 @@ public class AttachTryCatchVisitor extends AbstractVisitor { } } + private static void attachCatchAttr(CatchAttr catchAttr, InsnNode insn) { + CatchAttr existAttr = insn.get(AType.EXC_CATCH); + if (existAttr != null) { + // merge handlers + List handlers = Utils.concat(existAttr.getHandlers(), catchAttr.getHandlers()); + insn.addAttr(new CatchAttr(handlers)); + } else { + insn.addAttr(catchAttr); + } + } + private static List attachHandlers(MethodNode mth, ICatch catchBlock, InsnNode[] insnByOffset) { int[] handlerOffsetArr = catchBlock.getHandlers(); String[] handlerTypes = catchBlock.getTypes(); 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 51d294891..a3cd46e6c 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 @@ -80,6 +80,7 @@ public class MarkFinallyVisitor extends AbstractVisitor { reThrowInsn = BlockUtils.getLastInsn(excBlock); } } + break; } } if (allHandler != null && reThrowInsn != null) { @@ -108,27 +109,47 @@ public class MarkFinallyVisitor extends AbstractVisitor { BlockNode startBlock = Utils.getOne(handlerBlock.getCleanSuccessors()); FinallyExtractInfo extractInfo = new FinallyExtractInfo(allHandler, startBlock, handlerBlocks); - // collect handlers from this and all inner blocks - List handlers = new ArrayList<>(); - collectAllHandlers(tryBlock, handlers); - + boolean hasInnerBlocks = !tryBlock.getInnerTryBlocks().isEmpty(); + List handlers; + if (hasInnerBlocks) { + // collect handlers from this and all inner blocks (intentionally not using recursive collect for + // now) + handlers = new ArrayList<>(tryBlock.getHandlers()); + for (TryCatchBlockAttr innerTryBlock : tryBlock.getInnerTryBlocks()) { + handlers.addAll(innerTryBlock.getHandlers()); + } + } else { + handlers = tryBlock.getHandlers(); + } + if (handlers.isEmpty()) { + return false; + } // search 'finally' instructions in other handlers - if (!handlers.isEmpty()) { - for (ExceptionHandler otherHandler : handlers) { - if (otherHandler == allHandler) { - continue; - } - for (BlockNode checkBlock : otherHandler.getBlocks()) { - if (searchDuplicateInsns(checkBlock, extractInfo)) { - break; - } else { - extractInfo.getFinallyInsnsSlice().resetIncomplete(); - } + for (ExceptionHandler otherHandler : handlers) { + if (otherHandler == allHandler) { + continue; + } + for (BlockNode checkBlock : otherHandler.getBlocks()) { + if (searchDuplicateInsns(checkBlock, extractInfo)) { + break; + } else { + extractInfo.getFinallyInsnsSlice().resetIncomplete(); } } - if (extractInfo.getDuplicateSlices().size() != handlers.size() - 1) { + } + boolean mergeInnerTryBlocks; + int duplicatesCount = extractInfo.getDuplicateSlices().size(); + boolean fullTryBlock = duplicatesCount == (handlers.size() - 1); + if (fullTryBlock) { + // all collected handlers have duplicate block + mergeInnerTryBlocks = hasInnerBlocks; + } else { + // some handlers don't have duplicated blocks + if (!hasInnerBlocks || duplicatesCount != (tryBlock.getHandlers().size() - 1)) { + // unexpected count of duplicated slices return false; } + mergeInnerTryBlocks = false; } // remove 'finally' from 'try' blocks, check all up paths on each exit (connected with finally exit) @@ -166,9 +187,8 @@ public class MarkFinallyVisitor extends AbstractVisitor { apply(extractInfo); allHandler.setFinally(true); - // merge inner try blocks - List innerTryBlocks = tryBlock.getInnerTryBlocks(); - if (!innerTryBlocks.isEmpty()) { + if (mergeInnerTryBlocks) { + List innerTryBlocks = tryBlock.getInnerTryBlocks(); for (TryCatchBlockAttr innerTryBlock : innerTryBlocks) { tryBlock.getHandlers().addAll(innerTryBlock.getHandlers()); tryBlock.getBlocks().addAll(innerTryBlock.getBlocks()); @@ -188,13 +208,6 @@ public class MarkFinallyVisitor extends AbstractVisitor { return preds.collect(Collectors.toList()); } - private static void collectAllHandlers(TryCatchBlockAttr tryBlock, List handlers) { - handlers.addAll(tryBlock.getHandlers()); - for (TryCatchBlockAttr innerTryBlock : tryBlock.getInnerTryBlocks()) { - collectAllHandlers(innerTryBlock, handlers); - } - } - private static boolean checkSlices(FinallyExtractInfo extractInfo) { InsnsSlice finallySlice = extractInfo.getFinallyInsnsSlice(); List finallyInsnsList = finallySlice.getInsnsList(); diff --git a/jadx-core/src/test/java/jadx/tests/integration/trycatch/TestTryCatchFinally12.java b/jadx-core/src/test/java/jadx/tests/integration/trycatch/TestTryCatchFinally12.java new file mode 100644 index 000000000..f17836e6e --- /dev/null +++ b/jadx-core/src/test/java/jadx/tests/integration/trycatch/TestTryCatchFinally12.java @@ -0,0 +1,111 @@ +package jadx.tests.integration.trycatch; + +import org.junit.jupiter.api.Test; + +import jadx.tests.api.IntegrationTest; + +import static jadx.tests.api.utils.assertj.JadxAssertions.assertThat; + +public class TestTryCatchFinally12 extends IntegrationTest { + + public static class TestCls { + private StringBuilder sb; + + public void test1(int excType) { + try { + try { + call(excType); + } catch (NullPointerException e) { + sb.append("-catch"); + } + sb.append("-out"); + } finally { + sb.append("-finally"); + } + } + + public void test2(int excType) { + try { + try { + call(excType); + } catch (NullPointerException e) { + sb.append("-catch"); + } + } finally { + sb.append("-finally"); + } + } + + public void test3(int excType) { + try { + call(excType); + } catch (NullPointerException e) { + sb.append("-catch"); + } finally { + sb.append("-finally"); + } + } + + public void call(int excType) { + sb.append("call"); + switch (excType) { + case 1: + sb.append("-npe"); + throw new NullPointerException(); + case 2: + sb.append("-iae"); + throw new IllegalArgumentException(); + } + } + + public String runTest(int testNumber, int excType) { + sb = new StringBuilder(); + try { + switch (testNumber) { + case 1: + test1(excType); + break; + case 2: + test2(excType); + break; + case 3: + test3(excType); + break; + } + } catch (IllegalArgumentException e) { + assertThat(excType).isEqualTo(2); + } + return sb.toString(); + } + + public void check() { + assertThat(runTest(1, 0)).isEqualTo("call-out-finally"); + assertThat(runTest(1, 1)).isEqualTo("call-npe-catch-out-finally"); + assertThat(runTest(1, 2)).isEqualTo("call-iae-finally"); + + assertThat(runTest(2, 0)).isEqualTo("call-finally"); + assertThat(runTest(2, 1)).isEqualTo("call-npe-catch-finally"); + assertThat(runTest(2, 2)).isEqualTo("call-iae-finally"); + + assertThat(runTest(3, 0)).isEqualTo("call-finally"); + assertThat(runTest(3, 1)).isEqualTo("call-npe-catch-finally"); + assertThat(runTest(3, 2)).isEqualTo("call-iae-finally"); + } + } + + @Test + public void test() { + assertThat(getClassNode(TestCls.class)) + .code() + .countString(3, "} finally {"); + } + + @Test + public void testWithoutFinally() { + getArgs().setExtractFinally(false); + assertThat(getClassNode(TestCls.class)) + .code() + .doesNotContain("} finally {") + .countString(2 + 2 + 3, "sb.append(\"-finally\");"); + } +}