From 1bd4526e4c3331bc4e7d242ad617bee913431108 Mon Sep 17 00:00:00 2001 From: Skylot Date: Sun, 15 Oct 2023 16:03:22 +0100 Subject: [PATCH] fix: improve restructure of nested try/catch/finally blocks (#2033) --- .../java/jadx/core/dex/attributes/AFlag.java | 1 + .../blocks/BlockExceptionHandler.java | 13 +- .../dex/visitors/blocks/BlockProcessor.java | 79 +++++++++- .../visitors/finaly/MarkFinallyVisitor.java | 19 ++- .../main/java/jadx/core/utils/BlockUtils.java | 17 ++- .../trycatch/TestNestedTryCatch5.java | 19 +++ .../smali/trycatch/TestNestedTryCatch5.smali | 137 ++++++++++++++++++ 7 files changed, 258 insertions(+), 27 deletions(-) create mode 100644 jadx-core/src/test/java/jadx/tests/integration/trycatch/TestNestedTryCatch5.java create mode 100644 jadx-core/src/test/smali/trycatch/TestNestedTryCatch5.smali diff --git a/jadx-core/src/main/java/jadx/core/dex/attributes/AFlag.java b/jadx-core/src/main/java/jadx/core/dex/attributes/AFlag.java index 76ba2074372..25781e858c9 100644 --- a/jadx-core/src/main/java/jadx/core/dex/attributes/AFlag.java +++ b/jadx-core/src/main/java/jadx/core/dex/attributes/AFlag.java @@ -33,6 +33,7 @@ public enum AFlag { EXC_TOP_SPLITTER, EXC_BOTTOM_SPLITTER, FINALLY_INSNS, + IGNORE_THROW_SPLIT, SKIP_FIRST_ARG, SKIP_ARG, // skip argument in invoke call diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/blocks/BlockExceptionHandler.java b/jadx-core/src/main/java/jadx/core/dex/visitors/blocks/BlockExceptionHandler.java index 03842bb28eb..571a27f2624 100644 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/blocks/BlockExceptionHandler.java +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/blocks/BlockExceptionHandler.java @@ -6,6 +6,7 @@ import java.util.Comparator; import java.util.Deque; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Objects; @@ -283,14 +284,7 @@ private static boolean checkTryCatchRelation(List tryBlocks, boolean catchInTry = innerTryBlock.getBlocks().stream().anyMatch(isHandlersIntersects(outerTryBlock)); boolean blocksOutsideHandler = outerTryBlock.getBlocks().stream().anyMatch(b -> !handlerBlocks.contains(b)); - boolean makeInner = catchInHandler && (catchInTry || blocksOutsideHandler); - if (makeInner && innerTryBlock.isAllHandler()) { - // inner try block can't have catch-all handler - outerTryBlock.setBlocks(Utils.concatDistinct(outerTryBlock.getBlocks(), innerTryBlock.getBlocks())); - innerTryBlock.clear(); - return false; - } - if (makeInner) { + if (catchInHandler && (catchInTry || blocksOutsideHandler)) { // convert to inner List mergedBlocks = Utils.concatDistinct(outerTryBlock.getBlocks(), innerTryBlock.getBlocks()); innerTryBlock.getHandlers().removeAll(outerTryBlock.getHandlers()); @@ -299,7 +293,8 @@ private static boolean checkTryCatchRelation(List tryBlocks, outerTryBlock.setBlocks(mergedBlocks); return false; } - if (innerTryBlock.getHandlers().containsAll(outerTryBlock.getHandlers())) { + Set innerHandlerSet = new HashSet<>(innerTryBlock.getHandlers()); + if (innerHandlerSet.containsAll(outerTryBlock.getHandlers())) { // merge List mergedBlocks = Utils.concatDistinct(outerTryBlock.getBlocks(), innerTryBlock.getBlocks()); List handlers = Utils.concatDistinct(outerTryBlock.getHandlers(), innerTryBlock.getHandlers()); diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/blocks/BlockProcessor.java b/jadx-core/src/main/java/jadx/core/dex/visitors/blocks/BlockProcessor.java index 75f1d27c55b..7c3d4d67aef 100644 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/blocks/BlockProcessor.java +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/blocks/BlockProcessor.java @@ -1,8 +1,12 @@ package jadx.core.dex.visitors.blocks; import java.util.ArrayList; +import java.util.Collections; +import java.util.HashMap; +import java.util.HashSet; import java.util.LinkedHashSet; import java.util.List; +import java.util.Map; import java.util.Set; import org.jetbrains.annotations.Nullable; @@ -20,6 +24,7 @@ import jadx.core.dex.nodes.Edge; import jadx.core.dex.nodes.InsnNode; import jadx.core.dex.nodes.MethodNode; +import jadx.core.dex.trycatch.ExcHandlerAttr; import jadx.core.dex.trycatch.TryCatchBlockAttr; import jadx.core.dex.visitors.AbstractVisitor; import jadx.core.utils.BlockUtils; @@ -264,7 +269,7 @@ private static boolean modifyBlocksTree(MethodNode mth) { if (mergeConstReturn(mth)) { return true; } - return splitReturnBlocks(mth); + return splitExitBlocks(mth); } private static boolean mergeConstReturn(MethodNode mth) { @@ -478,11 +483,13 @@ private static boolean splitLoops(MethodNode mth, BlockNode block, List preds, InsnNode returnInsn) { - InsnArg retArg = returnInsn.getArg(0); - if (retArg.isRegister()) { - RegisterArg arg = (RegisterArg) retArg; - int regNum = arg.getRegNum(); + private static boolean splitThrow(MethodNode mth, BlockNode exitBlock) { + if (exitBlock.contains(AFlag.IGNORE_THROW_SPLIT)) { + return false; + } + List preds = exitBlock.getPredecessors(); + if (preds.size() < 2) { + return false; + } + InsnNode throwInsn = BlockUtils.getLastInsn(exitBlock); + if (throwInsn == null || throwInsn.getType() != InsnType.THROW) { + return false; + } + // split only for several exception handlers + // traverse predecessors to exception handler + Map handlersMap = new HashMap<>(preds.size()); + Set handlers = new HashSet<>(preds.size()); + for (BlockNode pred : preds) { + BlockUtils.visitPredecessorsUntil(mth, pred, block -> { + ExcHandlerAttr excHandlerAttr = block.get(AType.EXC_HANDLER); + if (excHandlerAttr == null) { + return false; + } + boolean correctHandler = excHandlerAttr.getHandler().getBlocks().contains(block); + if (correctHandler && isArgAssignInPred(Collections.singletonList(block), throwInsn.getArg(0))) { + handlersMap.put(pred, excHandlerAttr); + handlers.add(block); + } + return correctHandler; + }); + } + if (handlers.size() == 1) { + exitBlock.add(AFlag.IGNORE_THROW_SPLIT); + return false; + } + + boolean first = true; + for (BlockNode pred : new ArrayList<>(preds)) { + if (first) { + first = false; + } else { + BlockNode newThrowBlock = BlockSplitter.startNewBlock(mth, -1); + newThrowBlock.add(AFlag.SYNTHETIC); + for (InsnNode oldInsn : exitBlock.getInstructions()) { + InsnNode copyInsn = oldInsn.copyWithoutSsa(); + copyInsn.add(AFlag.SYNTHETIC); + newThrowBlock.getInstructions().add(copyInsn); + } + newThrowBlock.copyAttributesFrom(exitBlock); + ExcHandlerAttr excHandlerAttr = handlersMap.get(pred); + if (excHandlerAttr != null) { + excHandlerAttr.getHandler().addBlock(newThrowBlock); + } + BlockSplitter.replaceConnection(pred, exitBlock, newThrowBlock); + } + } + return true; + } + + private static boolean isArgAssignInPred(List preds, InsnArg arg) { + if (arg.isRegister()) { + int regNum = ((RegisterArg) arg).getRegNum(); for (BlockNode pred : preds) { for (InsnNode insnNode : pred.getInstructions()) { RegisterArg result = insnNode.getResult(); 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 bf00c8ebe9b..e4908a20617 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 @@ -44,7 +44,6 @@ ) 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) { @@ -164,15 +163,22 @@ private static boolean extractFinally(MethodNode mth, TryCatchBlockAttr tryBlock mergeInnerTryBlocks = false; } - // remove 'finally' from 'try' blocks, check all up paths on each exit (connected with finally exit) + // remove 'finally' from 'try' blocks, + // check all up paths on each exit (connected with 'finally' exit) List tryBlocks = allHandler.getTryBlock().getBlocks(); BlockNode bottomBlock = BlockUtils.getBottomBlock(allHandler.getBlocks()); if (bottomBlock == null) { + if (Consts.DEBUG_FINALLY) { + LOG.warn("No bottom block for handler: {} and blocks: {}", allHandler, allHandler.getBlocks()); + } return false; } BlockNode bottomFinallyBlock = BlockUtils.followEmptyPath(bottomBlock); BlockNode bottom = BlockUtils.getNextBlock(bottomFinallyBlock); if (bottom == null) { + if (Consts.DEBUG_FINALLY) { + LOG.warn("Finally bottom block not found for: {} and: {}", bottomBlock, bottomFinallyBlock); + } return false; } boolean found = false; @@ -197,12 +203,15 @@ private static boolean extractFinally(MethodNode mth, TryCatchBlockAttr tryBlock } } } - if (Consts.DEBUG_FINALLY) { - LOG.debug("Result slices:\n{}", extractInfo); - } if (!found) { + if (Consts.DEBUG_FINALLY) { + LOG.info("Dup not found for all handler: {}", allHandler); + } return false; } + if (Consts.DEBUG_FINALLY) { + LOG.debug("Result slices:\n{}", extractInfo); + } if (!checkSlices(extractInfo)) { mth.addWarnComment("Finally extract failed"); return false; diff --git a/jadx-core/src/main/java/jadx/core/utils/BlockUtils.java b/jadx-core/src/main/java/jadx/core/utils/BlockUtils.java index 835ddb6118b..e0e48b7f71e 100644 --- a/jadx-core/src/main/java/jadx/core/utils/BlockUtils.java +++ b/jadx-core/src/main/java/jadx/core/utils/BlockUtils.java @@ -512,22 +512,29 @@ public static List collectPredecessors(MethodNode mth, BlockNode star bs.or(blocksToBitSet(mth, stopBlocks)); } List list = new ArrayList<>(); - traversePredecessors(start, bs, list::add); + traversePredecessors(start, bs, block -> { + list.add(block); + return false; + }); return list; } + public static void visitPredecessorsUntil(MethodNode mth, BlockNode start, Predicate visitor) { + traversePredecessors(start, newBlocksBitSet(mth), visitor); + } + /** - * Up BFS + * Up BFS. + * To stop return true from predicate */ - private static void traversePredecessors(BlockNode start, BitSet visited, Consumer visitor) { + private static void traversePredecessors(BlockNode start, BitSet visited, Predicate visitor) { Queue queue = new ArrayDeque<>(); queue.add(start); while (true) { BlockNode current = queue.poll(); - if (current == null) { + if (current == null || visitor.test(current)) { return; } - visitor.accept(current); for (BlockNode next : current.getPredecessors()) { int id = next.getId(); if (!visited.get(id)) { diff --git a/jadx-core/src/test/java/jadx/tests/integration/trycatch/TestNestedTryCatch5.java b/jadx-core/src/test/java/jadx/tests/integration/trycatch/TestNestedTryCatch5.java new file mode 100644 index 00000000000..83acd3f3df0 --- /dev/null +++ b/jadx-core/src/test/java/jadx/tests/integration/trycatch/TestNestedTryCatch5.java @@ -0,0 +1,19 @@ +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 TestNestedTryCatch5 extends SmaliTest { + + @Test + public void test() { + disableCompilation(); + assertThat(getClassNodeFromSmali()) + .code() + .doesNotContain("?? ") + .countString(3, "throw "); + } +} diff --git a/jadx-core/src/test/smali/trycatch/TestNestedTryCatch5.smali b/jadx-core/src/test/smali/trycatch/TestNestedTryCatch5.smali new file mode 100644 index 00000000000..04d0f97b8c0 --- /dev/null +++ b/jadx-core/src/test/smali/trycatch/TestNestedTryCatch5.smali @@ -0,0 +1,137 @@ +.class public Ltrycatch/TestNestedTryCatch5; +.super Ljava/lang/Object; + +.method public test(Landroid/database/sqlite/SQLiteDatabase;)V + .registers 13 + + const/4 v0, 0x0 + invoke-static {p1, v0}, LX/7Yz;->A0I(Ljava/lang/Object;I)V + iget-boolean v0, p0, LX/00y;->A00:Z + if-nez v0, :cond_9e + + :try_start_8 + iget-object v8, p0, LX/00y;->A03:LX/0Ux; + iget-object v0, p0, LX/00y;->A04:LX/0Km; + invoke-static {p1, v0}, LX/00y;->A01(Landroid/database/sqlite/SQLiteDatabase;LX/0Km;)LX/0g9; + move-result-object v10 + + check-cast v8, LX/0AB; + invoke-virtual {v8, v10}, LX/0AB;->A05(LX/0wU;)V + iget-object v0, v8, LX/0AB;->A01:LX/0Z4; + iget-object v9, v0, LX/0Z4;->A00:Landroidx/work/impl/WorkDatabase_Impl; + iput-object v10, v9, LX/0Rt;->A0B:LX/0wU; + const-string v0, "PRAGMA foreign_keys = ON" + invoke-interface {v10, v0}, LX/0wU;->Auv(Ljava/lang/String;)V + iget-object v1, v9, LX/0Rt;->A06:LX/0Uj; + iget-object v2, v1, LX/0Uj;->A05:Ljava/lang/Object; + + monitor-enter v2 + :try_end_25 + .catchall {:try_start_8 .. :try_end_25} :catchall_95 + + :try_start_25 + iget-boolean v0, v1, LX/0Uj;->A0D:Z + + if-eqz v0, :cond_31 + const-string v1, "ROOM" + const-string v0, "Invalidation tracker is initialized twice :/." + invoke-static {v1, v0}, Landroid/util/Log;->e(Ljava/lang/String;Ljava/lang/String;)I + goto :goto_4e + + :cond_31 + const-string v0, "PRAGMA temp_store = MEMORY;" + invoke-interface {v10, v0}, LX/0wU;->Auv(Ljava/lang/String;)V + const-string v0, "PRAGMA recursive_triggers=\'ON\';" + invoke-interface {v10, v0}, LX/0wU;->Auv(Ljava/lang/String;)V + const-string v0, "CREATE TEMP TABLE room_table_modification_log (table_id INTEGER PRIMARY KEY, invalidated INTEGER NOT NULL DEFAULT 0)" + invoke-interface {v10, v0}, LX/0wU;->Auv(Ljava/lang/String;)V + invoke-virtual {v1, v10}, LX/0Uj;->A00(LX/0wU;)V + const-string v0, "UPDATE room_table_modification_log SET invalidated = 0 WHERE invalidated = 1" + invoke-interface {v10, v0}, LX/0wU;->ArR(Ljava/lang/String;)LX/0wJ; + move-result-object v0 + + iput-object v0, v1, LX/0Uj;->A0C:LX/0wJ; + const/4 v0, 0x1 + iput-boolean v0, v1, LX/0Uj;->A0D:Z + :try_end_4e + .catchall {:try_start_25 .. :try_end_4e} :catchall_92 + + :goto_4e + :try_start_4e + monitor-exit v2 + + iget-object v0, v9, LX/0Rt;->A01:Ljava/util/List; + if-eqz v0, :cond_8e + invoke-interface {v0}, Ljava/util/List;->size()I + move-result v7 + + const/4 v6, 0x0 + + :goto_58 + if-ge v6, v7, :cond_8e + + iget-object v0, v9, LX/0Rt;->A01:Ljava/util/List; + invoke-interface {v0, v6}, Ljava/util/List;->get(I)Ljava/lang/Object; + iget-object v5, v10, LX/0g9;->A00:Landroid/database/sqlite/SQLiteDatabase; + invoke-virtual {v5}, Landroid/database/sqlite/SQLiteDatabase;->beginTransaction()V + :try_end_64 + .catchall {:try_start_4e .. :try_end_64} :catchall_95 + + :try_start_64 + invoke-static {}, LX/001;->A0r()Ljava/lang/StringBuilder; + move-result-object v4 + + const-string v0, "DELETE FROM workspec WHERE state IN (2, 3, 5) AND (last_enqueue_time + minimum_retention_duration) < " + invoke-virtual {v4, v0}, Ljava/lang/StringBuilder;->append(Ljava/lang/String;)Ljava/lang/StringBuilder; + invoke-static {}, Ljava/lang/System;->currentTimeMillis()J + move-result-wide v2 + + sget-wide v0, LX/0Jm;->A00:J + sub-long/2addr v2, v0 + invoke-virtual {v4, v2, v3}, Ljava/lang/StringBuilder;->append(J)Ljava/lang/StringBuilder; + const-string v0, " AND (SELECT COUNT(*)=0 FROM dependency WHERE prerequisite_id=id AND work_spec_id NOT IN (SELECT id FROM workspec WHERE state IN (2, 3, 5)))" + invoke-static {v0, v4}, LX/000;->A0X(Ljava/lang/String;Ljava/lang/StringBuilder;)Ljava/lang/String; + move-result-object v0 + + invoke-interface {v10, v0}, LX/0wU;->Auv(Ljava/lang/String;)V + invoke-virtual {v5}, Landroid/database/sqlite/SQLiteDatabase;->setTransactionSuccessful()V + :try_end_83 + .catchall {:try_start_64 .. :try_end_83} :catchall_89 + + :try_start_83 + invoke-virtual {v5}, Landroid/database/sqlite/SQLiteDatabase;->endTransaction()V + add-int/lit8 v6, v6, 0x1 + goto :goto_58 + + :catchall_89 + move-exception v0 + invoke-virtual {v5}, Landroid/database/sqlite/SQLiteDatabase;->endTransaction()V + goto :goto_94 + + :cond_8e + const/4 v0, 0x0 + iput-object v0, v8, LX/0AB;->A00:LX/0N8; + goto :goto_9e + + :catchall_92 + move-exception v0 + monitor-exit v2 + + :goto_94 + throw v0 + :try_end_95 + .catchall {:try_start_83 .. :try_end_95} :catchall_95 + + :catchall_95 + move-exception v2 + sget-object v1, LX/0Fu;->A04:LX/0Fu; + new-instance v0, LX/0oe; + invoke-direct {v0, v1, v2}, LX/0oe;->(LX/0Fu;Ljava/lang/Throwable;)V + throw v0 + + :cond_9e + :goto_9e + const/4 v0, 0x1 + iput-boolean v0, p0, LX/00y;->A01:Z + return-void +.end method