diff --git a/jadx-core/src/main/java/jadx/core/Consts.java b/jadx-core/src/main/java/jadx/core/Consts.java index f129de01b6e2ca01a0b053f3051b5a3733502744..e9e4bfdc18a803ac24616f991a7de6797db63cae 100644 --- a/jadx-core/src/main/java/jadx/core/Consts.java +++ b/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"; diff --git a/jadx-core/src/main/java/jadx/core/dex/nodes/BlockNode.java b/jadx-core/src/main/java/jadx/core/dex/nodes/BlockNode.java index a92b9fdc35ecf708ba188987d2eb64d71654a2b9..a31e41408c1143da75dcfd64d040342bdded2b69 100644 --- a/jadx-core/src/main/java/jadx/core/dex/nodes/BlockNode.java +++ b/jadx-core/src/main/java/jadx/core/dex/nodes/BlockNode.java @@ -170,6 +170,10 @@ public final class BlockNode extends AttrNode implements IBlock, Comparable allHandlerBlocks; private final List 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 allHandlerBlocks) { + public FinallyExtractInfo(MethodNode mth, ExceptionHandler finallyHandler, BlockNode startBlock, List 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}"; + } } 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 36d2ffc20090d85de06869528e3f655ac2781de4..354c82adf2d7654963863350737a3459748fe8b2 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 @@ -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 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 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 handlerBlocks) { + List 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 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 getPathStarts(MethodNode mth, BlockNode bottom, BlockNode bottomFinallyBlock) { Stream 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 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 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 dupInsns = dupBlock.getInstructions(); List 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); } } } diff --git a/jadx-core/src/test/java/jadx/tests/api/IntegrationTest.java b/jadx-core/src/test/java/jadx/tests/api/IntegrationTest.java index 620fd36d7fb0bd514e56afa4ac76912ce772c0e0..aef7dcd6750a8002f913de354a770e191c475271 100644 --- a/jadx-core/src/test/java/jadx/tests/api/IntegrationTest.java +++ b/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(); } diff --git a/jadx-core/src/test/java/jadx/tests/api/extensions/profiles/TestProfile.java b/jadx-core/src/test/java/jadx/tests/api/extensions/profiles/TestProfile.java index 4dbd5e394c6c56b290b8586c90166432a1259f40..bdb965bb29ed8d39113501059dd71e07cead8018 100644 --- a/jadx-core/src/test/java/jadx/tests/api/extensions/profiles/TestProfile.java +++ b/jadx-core/src/test/java/jadx/tests/api/extensions/profiles/TestProfile.java @@ -31,6 +31,10 @@ public enum TestProfile implements Consumer { 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 { } @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() { diff --git a/jadx-core/src/test/java/jadx/tests/integration/trycatch/TestTryCatchFinally14.java b/jadx-core/src/test/java/jadx/tests/integration/trycatch/TestTryCatchFinally14.java new file mode 100644 index 0000000000000000000000000000000000000000..631303cea433cc69610503b18d328241089f4ecb --- /dev/null +++ b/jadx-core/src/test/java/jadx/tests/integration/trycatch/TestTryCatchFinally14.java @@ -0,0 +1,45 @@ +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) {"); + } +}