fix: improve blocks tree compare for finally extract (#1501)

jadx-core/src/main/java/jadx/core/Consts.java

@@ -7,6 +7,7 @@ public class Consts {
 	public static final boolean DEBUG_TYPE_INFERENCE = false;
 	public static final boolean DEBUG_OVERLOADED_CASTS = false;
 	public static final boolean DEBUG_EXC_HANDLERS = false;
+	public static final boolean DEBUG_FINALLY = false;
 
 	public static final String CLASS_OBJECT = "java.lang.Object";
 	public static final String CLASS_STRING = "java.lang.String";

jadx-core/src/main/java/jadx/core/dex/nodes/BlockNode.java

@@ -170,6 +170,10 @@ public final class BlockNode extends AttrNode implements IBlock, Comparable<Bloc
 		return contains(AFlag.RETURN);
 	}
 
+	public boolean isEmpty() {
+		return instructions.isEmpty();
+	}
+
 	@Override
 	public int hashCode() {
 		return startOffset;

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

@@ -6,9 +6,12 @@ import java.util.List;
 import java.util.Set;
 
 import jadx.core.dex.nodes.BlockNode;
+import jadx.core.dex.nodes.MethodNode;
 import jadx.core.dex.trycatch.ExceptionHandler;
+import jadx.core.utils.Utils;
 
 public class FinallyExtractInfo {
+	private final MethodNode mth;
 	private final ExceptionHandler finallyHandler;
 	private final List<BlockNode> allHandlerBlocks;
 	private final List<InsnsSlice> duplicateSlices = new ArrayList<>();
@@ -16,12 +19,17 @@ public class FinallyExtractInfo {
 	private final InsnsSlice finallyInsnsSlice = new InsnsSlice();
 	private final BlockNode startBlock;
 
-	public FinallyExtractInfo(ExceptionHandler finallyHandler, BlockNode startBlock, List<BlockNode> allHandlerBlocks) {
+	public FinallyExtractInfo(MethodNode mth, ExceptionHandler finallyHandler, BlockNode startBlock, List<BlockNode> allHandlerBlocks) {
+		this.mth = mth;
 		this.finallyHandler = finallyHandler;
 		this.startBlock = startBlock;
 		this.allHandlerBlocks = allHandlerBlocks;
 	}
 
+	public MethodNode getMth() {
+		return mth;
+	}
+
 	public ExceptionHandler getFinallyHandler() {
 		return finallyHandler;
 	}
@@ -45,4 +53,12 @@ public class FinallyExtractInfo {
 	public BlockNode getStartBlock() {
 		return startBlock;
 	}
+
+	@Override
+	public String toString() {
+		return "FinallyExtractInfo{"
+				+ "\n finally:\n  " + finallyInsnsSlice
+				+ "\n dups:\n  " + Utils.listToString(duplicateSlices, "\n  ")
+				+ "\n}";
+	}
 }

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

@@ -9,6 +9,7 @@ import org.jetbrains.annotations.Nullable;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+import jadx.core.Consts;
 import jadx.core.dex.attributes.AFlag;
 import jadx.core.dex.attributes.AType;
 import jadx.core.dex.instructions.InsnType;
@@ -38,6 +39,7 @@ import jadx.core.utils.Utils;
 )
 public class MarkFinallyVisitor extends AbstractVisitor {
 	private static final Logger LOG = LoggerFactory.getLogger(MarkFinallyVisitor.class);
+	// private static final Logger LOG = LoggerFactory.getLogger(MarkFinallyVisitor.class);
 
 	@Override
 	public void visit(MethodNode mth) {
@@ -60,7 +62,7 @@ public class MarkFinallyVisitor extends AbstractVisitor {
 				}
 			}
 		} catch (Exception e) {
-			LOG.warn("Undo finally extract visitor, mth: {}", mth, e);
+			mth.addWarnComment("Undo finally extract visitor", e);
 			undoFinallyVisitor(mth);
 		}
 	}
@@ -100,20 +102,23 @@ public class MarkFinallyVisitor extends AbstractVisitor {
 		List<BlockNode> handlerBlocks =
 				new ArrayList<>(BlockUtils.collectBlocksDominatedByWithExcHandlers(mth, handlerBlock, handlerBlock));
 		handlerBlocks.remove(handlerBlock); // exclude block with 'move-exception'
-		handlerBlocks.removeIf(b -> BlockUtils.checkLastInsnType(b, InsnType.THROW));
+		cutPathEnds(mth, handlerBlocks);
 		if (handlerBlocks.isEmpty() || BlockUtils.isAllBlocksEmpty(handlerBlocks)) {
 			// remove empty catch
 			allHandler.getTryBlock().removeHandler(allHandler);
 			return true;
 		}
 		BlockNode startBlock = Utils.getOne(handlerBlock.getCleanSuccessors());
-		FinallyExtractInfo extractInfo = new FinallyExtractInfo(allHandler, startBlock, handlerBlocks);
+		FinallyExtractInfo extractInfo = new FinallyExtractInfo(mth, allHandler, startBlock, handlerBlocks);
+		if (Consts.DEBUG_FINALLY) {
+			LOG.debug("Finally info: handler=({}), start={}, blocks={}", allHandler, startBlock, handlerBlocks);
+		}
 
 		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)
+			// 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());
@@ -137,10 +142,12 @@ public class MarkFinallyVisitor extends AbstractVisitor {
 				}
 			}
 		}
+		if (Consts.DEBUG_FINALLY) {
+			LOG.debug("Handlers slices:\n{}", extractInfo);
+		}
 		boolean mergeInnerTryBlocks;
 		int duplicatesCount = extractInfo.getDuplicateSlices().size();
-		boolean fullTryBlock = duplicatesCount == (handlers.size() - 1);
-		if (fullTryBlock) {
+		if (duplicatesCount == (handlers.size() - 1)) {
 			// all collected handlers have duplicate block
 			mergeInnerTryBlocks = hasInnerBlocks;
 		} else {
@@ -170,15 +177,24 @@ public class MarkFinallyVisitor extends AbstractVisitor {
 			if (upPath.size() < handlerBlocks.size()) {
 				continue;
 			}
+			if (Consts.DEBUG_FINALLY) {
+				LOG.debug("Checking dup path starts: {} from {}", upPath, pred);
+			}
 			for (BlockNode block : upPath) {
 				if (searchDuplicateInsns(block, extractInfo)) {
 					found = true;
+					if (Consts.DEBUG_FINALLY) {
+						LOG.debug("Found dup in: {} from {}", block, pred);
+					}
 					break;
 				} else {
 					extractInfo.getFinallyInsnsSlice().resetIncomplete();
 				}
 			}
 		}
+		if (Consts.DEBUG_FINALLY) {
+			LOG.debug("Result slices:\n{}", extractInfo);
+		}
 		if (!found) {
 			return false;
 		}
@@ -204,6 +220,28 @@ public class MarkFinallyVisitor extends AbstractVisitor {
 		return true;
 	}
 
+	private static void cutPathEnds(MethodNode mth, List<BlockNode> handlerBlocks) {
+		List<BlockNode> throwBlocks = ListUtils.filter(handlerBlocks,
+				b -> BlockUtils.checkLastInsnType(b, InsnType.THROW));
+		if (throwBlocks.size() != 1) {
+			mth.addDebugComment("Finally have unexpected throw blocks count: " + throwBlocks.size() + ", expect 1");
+			return;
+		}
+		BlockNode throwBlock = throwBlocks.get(0);
+		handlerBlocks.remove(throwBlock);
+		removeEmptyUpPath(handlerBlocks, throwBlock);
+	}
+
+	private static void removeEmptyUpPath(List<BlockNode> handlerBlocks, BlockNode startBlock) {
+		for (BlockNode pred : startBlock.getPredecessors()) {
+			if (pred.isEmpty()) {
+				if (handlerBlocks.remove(pred) && !BlockUtils.isBackEdge(pred, startBlock)) {
+					removeEmptyUpPath(handlerBlocks, pred);
+				}
+			}
+		}
+	}
+
 	private static List<BlockNode> getPathStarts(MethodNode mth, BlockNode bottom, BlockNode bottomFinallyBlock) {
 		Stream<BlockNode> preds = bottom.getPredecessors().stream().filter(b -> b != bottomFinallyBlock);
 		if (bottom == mth.getExitBlock()) {
@@ -219,9 +257,8 @@ public class MarkFinallyVisitor extends AbstractVisitor {
 		for (InsnsSlice dupSlice : extractInfo.getDuplicateSlices()) {
 			List<InsnNode> dupInsnsList = dupSlice.getInsnsList();
 			if (dupInsnsList.size() != finallyInsnsList.size()) {
-				if (LOG.isDebugEnabled()) {
-					LOG.debug("Incorrect finally slice size: {}, expected: {}", dupSlice, finallySlice);
-				}
+				extractInfo.getMth().addDebugComment(
+						"Incorrect finally slice size: " + dupSlice + ", expected: " + finallySlice);
 				return false;
 			}
 		}
@@ -231,9 +268,8 @@ public class MarkFinallyVisitor extends AbstractVisitor {
 				List<InsnNode> insnsList = dupSlice.getInsnsList();
 				InsnNode dupInsn = insnsList.get(i);
 				if (finallyInsn.getType() != dupInsn.getType()) {
-					if (LOG.isDebugEnabled()) {
-						LOG.debug("Incorrect finally slice insn: {}, expected: {}", dupInsn, finallyInsn);
-					}
+					extractInfo.getMth().addDebugComment(
+							"Incorrect finally slice insn: " + dupInsn + ", expected: " + finallyInsn);
 					return false;
 				}
 			}
@@ -342,24 +378,29 @@ public class MarkFinallyVisitor extends AbstractVisitor {
 	private static InsnsSlice isStartBlock(BlockNode dupBlock, BlockNode finallyBlock, FinallyExtractInfo extractInfo) {
 		List<InsnNode> dupInsns = dupBlock.getInstructions();
 		List<InsnNode> finallyInsns = finallyBlock.getInstructions();
-		if (dupInsns.size() < finallyInsns.size()) {
+		int dupSize = dupInsns.size();
+		int finSize = finallyInsns.size();
+		if (dupSize < finSize) {
 			return null;
 		}
-		int startPos = dupInsns.size() - finallyInsns.size();
+		int startPos;
 		int endPos = 0;
-		// fast check from end of block
-		if (!checkInsns(dupInsns, finallyInsns, startPos)) {
-			// check from block start
-			if (checkInsns(dupInsns, finallyInsns, 0)) {
-				startPos = 0;
-				endPos = finallyInsns.size();
-			} else {
+		if (dupSize == finSize) {
+			if (!checkInsns(dupInsns, finallyInsns, 0)) {
+				return null;
+			}
+			startPos = 0;
+		} else {
+			// dupSize > finSize
+			startPos = dupSize - finSize;
+			// fast check from end of block
+			if (!checkInsns(dupInsns, finallyInsns, startPos)) {
 				// search start insn
 				boolean found = false;
 				for (int i = 1; i < startPos; i++) {
 					if (checkInsns(dupInsns, finallyInsns, i)) {
 						startPos = i;
-						endPos = finallyInsns.size() + i;
+						endPos = finSize + i;
 						found = true;
 						break;
 					}
@@ -379,7 +420,7 @@ public class MarkFinallyVisitor extends AbstractVisitor {
 			// both slices completed
 			complete = true;
 		} else {
-			endIndex = dupInsns.size();
+			endIndex = dupSize;
 			complete = false;
 		}
 
@@ -393,9 +434,8 @@ public class MarkFinallyVisitor extends AbstractVisitor {
 		if (finallySlice.isComplete()) {
 			// compare slices
 			if (finallySlice.getInsnsList().size() != slice.getInsnsList().size()) {
-				if (LOG.isDebugEnabled()) {
-					LOG.debug("Another duplicated slice has different insns count: {}, finally: {}", slice, finallySlice);
-				}
+				extractInfo.getMth().addDebugComment(
+						"Another duplicated slice has different insns count: " + slice + ", finally: " + finallySlice);
 				return null;
 			}
 			// TODO: add additional slices checks
@@ -522,7 +562,7 @@ public class MarkFinallyVisitor extends AbstractVisitor {
 				DepthTraversal.visit(visitor, mth);
 			}
 		} catch (Exception e) {
-			LOG.error("Undo finally extract failed, mth: {}", mth, e);
+			mth.addError("Undo finally extract failed", e);
 		}
 	}
 }

jadx-core/src/test/java/jadx/tests/api/IntegrationTest.java

@@ -79,8 +79,6 @@ public abstract class IntegrationTest extends TestUtils {
 	private static final String TEST_DIRECTORY = "src/test/java";
 	private static final String TEST_DIRECTORY2 = "jadx-core/" + TEST_DIRECTORY;
 
-	private static final String OUT_DIR = "test-out-tmp";
-
 	private static final String DEFAULT_INPUT_PLUGIN = "dx";
 	/**
 	 * Set 'TEST_INPUT_PLUGIN' env variable to use 'java' or 'dx' input in tests
@@ -132,7 +130,7 @@ public abstract class IntegrationTest extends TestUtils {
 		this.useJavaInput = null;
 
 		args = new JadxArgs();
-		args.setOutDir(new File(OUT_DIR));
+		args.setOutDir(new File("test-out-tmp"));
 		args.setShowInconsistentCode(true);
 		args.setThreadsCount(1);
 		args.setSkipResources(true);
@@ -156,6 +154,10 @@ public abstract class IntegrationTest extends TestUtils {
 		}
 	}
 
+	public void setOutDirSuffix(String suffix) {
+		args.setOutDir(new File("test-out-" + suffix + "-tmp"));
+	}
+
 	public String getTestName() {
 		return this.getClass().getSimpleName();
 	}

jadx-core/src/test/java/jadx/tests/api/extensions/profiles/TestProfile.java

@@ -31,6 +31,10 @@ public enum TestProfile implements Consumer<IntegrationTest> {
 		test.useTargetJavaVersion(11);
 		test.useJavaInput();
 	}),
+	JAVA17("java-17", test -> {
+		test.useTargetJavaVersion(17);
+		test.useJavaInput();
+	}),
 	ECJ_DX_J8("ecj-dx-j8", test -> {
 		test.useEclipseCompiler();
 		test.useTargetJavaVersion(8);
@@ -52,8 +56,9 @@ public enum TestProfile implements Consumer<IntegrationTest> {
 	}
 
 	@Override
-	public void accept(IntegrationTest integrationTest) {
-		this.setup.accept(integrationTest);
+	public void accept(IntegrationTest test) {
+		this.setup.accept(test);
+		test.setOutDirSuffix(description);
 	}
 
 	public String getDescription() {

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

+package jadx.tests.integration.trycatch;
+
+import jadx.tests.api.IntegrationTest;
+import jadx.tests.api.extensions.profiles.TestProfile;
+import jadx.tests.api.extensions.profiles.TestWithProfiles;
+
+import static jadx.tests.api.utils.assertj.JadxAssertions.assertThat;
+
+public class TestTryCatchFinally14 extends IntegrationTest {
+
+	@SuppressWarnings("unused")
+	public static class TestCls {
+		private TCls t;
+
+		public void test() {
+			try {
+				if (t != null) {
+					t.doSomething();
+				}
+			} finally {
+				if (t != null) {
+					t.doFinally();
+				}
+			}
+		}
+
+		private static class TCls {
+			public void doSomething() {
+			}
+
+			public void doFinally() {
+			}
+		}
+	}
+
+	@TestWithProfiles({ TestProfile.DX_J8, TestProfile.D8_J11, TestProfile.JAVA8 })
+	public void test() {
+		assertThat(getClassNode(TestCls.class))
+				.code()
+				.containsOne(".doSomething();")
+				.containsOne("} finally {")
+				.containsOne(".doFinally();")
+				.countString(2, "!= null) {");
+	}
+}