fix: support nested try/finally blocks (#1249)

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<ExceptionHandler> handlers = Utils.concat(existAttr.getHandlers(), catchAttr.getHandlers());
+			insn.addAttr(new CatchAttr(handlers));
+		} else {
+			insn.addAttr(catchAttr);
+		}
+	}
+
 	private static List<ExceptionHandler> attachHandlers(MethodNode mth, ICatch catchBlock, InsnNode[] insnByOffset) {
 		int[] handlerOffsetArr = catchBlock.getHandlers();
 		String[] handlerTypes = catchBlock.getTypes();

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<ExceptionHandler> handlers = new ArrayList<>();
-		collectAllHandlers(tryBlock, handlers);
-
+		boolean hasInnerBlocks = !tryBlock.getInnerTryBlocks().isEmpty();
+		List<ExceptionHandler> 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<TryCatchBlockAttr> innerTryBlocks = tryBlock.getInnerTryBlocks();
-		if (!innerTryBlocks.isEmpty()) {
+		if (mergeInnerTryBlocks) {
+			List<TryCatchBlockAttr> 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<ExceptionHandler> handlers) {
-		handlers.addAll(tryBlock.getHandlers());
-		for (TryCatchBlockAttr innerTryBlock : tryBlock.getInnerTryBlocks()) {
-			collectAllHandlers(innerTryBlock, handlers);
-		}
-	}
-
 	private static boolean checkSlices(FinallyExtractInfo extractInfo) {
 		InsnsSlice finallySlice = extractInfo.getFinallyInsnsSlice();
 		List<InsnNode> finallyInsnsList = finallySlice.getInsnsList();

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\");");
+	}
+}