jessikitty/jadx
1
0
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases Wiki Activity

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

Browse Source
This commit is contained in:
Skylot
2022-05-31 20:57:34 +01:00
parent fcd58ae76f
commit e6b6b93cbb
7 changed files with 147 additions and 34 deletions
Download Patch File Download Diff File Expand all files Collapse all files
jadx-core/src/main/java/jadx/core/Consts.java
+1
View File
@@ -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
+4
View File
@@ -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
+17 -1
View File
@@ -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
+68 -28
View File
@@ -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
+5 -3
View File
@@ -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
+7 -2
View File
@@ -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
+45
View File
@@ -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) {");
}
}