diff --git a/jadx-core/src/main/java/jadx/core/codegen/RegionGen.java b/jadx-core/src/main/java/jadx/core/codegen/RegionGen.java index e4cd8a293..54b3c1fa8 100644 --- a/jadx-core/src/main/java/jadx/core/codegen/RegionGen.java +++ b/jadx-core/src/main/java/jadx/core/codegen/RegionGen.java @@ -131,12 +131,20 @@ public class RegionGen extends InsnGen { } } } + boolean comment = region.contains(AFlag.COMMENT_OUT); + if (comment) { + code.add("// "); + } code.add("if ("); new ConditionGen(this).add(code, region.getCondition()); code.add(") {"); makeRegionIndent(code, region.getThenRegion()); - code.startLine('}'); + if (comment) { + code.startLine("// }"); + } else { + code.startLine('}'); + } IContainer els = region.getElseRegion(); if (RegionUtils.notEmpty(els)) { @@ -146,7 +154,11 @@ public class RegionGen extends InsnGen { } code.add('{'); makeRegionIndent(code, els); - code.startLine('}'); + if (comment) { + code.startLine("// }"); + } else { + code.startLine('}'); + } } } diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/PrepareForCodeGen.java b/jadx-core/src/main/java/jadx/core/dex/visitors/PrepareForCodeGen.java index 003a213a7..2fb10f056 100644 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/PrepareForCodeGen.java +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/PrepareForCodeGen.java @@ -2,13 +2,11 @@ package jadx.core.dex.visitors; import java.util.Iterator; import java.util.List; -import java.util.Objects; import jadx.core.dex.attributes.AFlag; import jadx.core.dex.instructions.ArithNode; import jadx.core.dex.instructions.ArithOp; import jadx.core.dex.instructions.InsnType; -import jadx.core.dex.instructions.args.ArgType; import jadx.core.dex.instructions.args.InsnArg; import jadx.core.dex.instructions.args.InsnWrapArg; import jadx.core.dex.instructions.args.RegisterArg; @@ -16,6 +14,7 @@ import jadx.core.dex.instructions.mods.ConstructorInsn; import jadx.core.dex.instructions.mods.TernaryInsn; import jadx.core.dex.nodes.BlockNode; import jadx.core.dex.nodes.IBlock; +import jadx.core.dex.nodes.IRegion; import jadx.core.dex.nodes.InsnNode; import jadx.core.dex.nodes.MethodNode; import jadx.core.dex.regions.conditions.IfCondition; @@ -38,8 +37,6 @@ import jadx.core.utils.exceptions.JadxException; ) public class PrepareForCodeGen extends AbstractVisitor { - public static final SuperCallRegionVisitor SUPER_CALL_REGION_VISITOR = new SuperCallRegionVisitor(); - @Override public void visit(MethodNode mth) throws JadxException { List blocks = mth.getBasicBlocks(); @@ -55,7 +52,7 @@ public class PrepareForCodeGen extends AbstractVisitor { removeParenthesis(block); modifyArith(block); } - commentOutInsnsBeforeSuper(mth); + commentOutInsnsInConstructor(mth); } private static void removeInstructions(BlockNode block) { @@ -180,31 +177,94 @@ public class PrepareForCodeGen extends AbstractVisitor { } } - private void commentOutInsnsBeforeSuper(MethodNode mth) { - if (mth.isConstructor() && !Objects.equals(mth.getParentClass().getSuperClass(), ArgType.OBJECT)) { - DepthRegionTraversal.traverse(mth, SUPER_CALL_REGION_VISITOR); + private void commentOutInsnsInConstructor(MethodNode mth) { + if (mth.isConstructor()) { + ConstructorInsn constrInsn = searchConstructorCall(mth); + if (constrInsn != null && !constrInsn.contains(AFlag.DONT_GENERATE)) { + DepthRegionTraversal.traverse(mth, new ConstructorRegionVisitor(constrInsn)); + } } } - private static final class SuperCallRegionVisitor extends AbstractRegionVisitor { + private ConstructorInsn searchConstructorCall(MethodNode mth) { + for (BlockNode block : mth.getBasicBlocks()) { + for (InsnNode insn : block.getInstructions()) { + InsnType insnType = insn.getType(); + if (insnType == InsnType.CONSTRUCTOR) { + ConstructorInsn constrInsn = (ConstructorInsn) insn; + if (constrInsn.isSuper() || constrInsn.isThis()) { + return constrInsn; + } + } + } + } + return null; + } + + private static final class ConstructorRegionVisitor extends AbstractRegionVisitor { + private final ConstructorInsn constrInsn; + private int regionDepth; + private boolean found; + private boolean brokenCode; + private int commentedCount; + + public ConstructorRegionVisitor(ConstructorInsn constrInsn) { + this.constrInsn = constrInsn; + } + + @Override + public boolean enterRegion(MethodNode mth, IRegion region) { + if (found) { + return false; + } + regionDepth++; + return true; + } + + @Override + public void leaveRegion(MethodNode mth, IRegion region) { + if (!found) { + regionDepth--; + region.add(AFlag.COMMENT_OUT); + commentedCount++; + } + } + @Override public void processBlock(MethodNode mth, IBlock container) { + if (found) { + return; + } for (InsnNode insn : container.getInstructions()) { - InsnType insnType = insn.getType(); - if ((insnType == InsnType.CONSTRUCTOR) && ((ConstructorInsn) insn).isSuper()) { - // found super call - commentOutInsns(container, insn); - // TODO: process all previous regions (in case of branching before super call) + if (insn == constrInsn) { + found = true; + addMethodMsg(mth); + break; + } + insn.add(AFlag.COMMENT_OUT); + commentedCount++; + if (!brokenCode) { + RegisterArg resArg = insn.getResult(); + if (resArg != null) { + for (RegisterArg arg : resArg.getSVar().getUseList()) { + if (arg.getParentInsn() == constrInsn) { + brokenCode = true; + break; + } + } + } } } } - private static void commentOutInsns(IBlock container, InsnNode superInsn) { - for (InsnNode insn : container.getInstructions()) { - if (insn == superInsn) { - break; + private void addMethodMsg(MethodNode mth) { + if (commentedCount > 0) { + String msg = "JADX WARN: Illegal instructions before constructor call commented (this can break semantics)"; + if (brokenCode || regionDepth > 1) { + mth.addWarn(msg); + } else { + mth.addComment(msg); } - insn.add(AFlag.COMMENT_OUT); } } } 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 77a4c69ce..4462e54bf 100644 --- a/jadx-core/src/test/java/jadx/tests/api/IntegrationTest.java +++ b/jadx-core/src/test/java/jadx/tests/api/IntegrationTest.java @@ -81,6 +81,8 @@ public abstract class IntegrationTest extends TestUtils { protected boolean useEclipseCompiler; protected Map resMap = Collections.emptyMap(); + private boolean allowWarnInCode; + private DynamicCompiler dynamicCompiler; @BeforeEach @@ -170,7 +172,7 @@ public abstract class IntegrationTest extends TestUtils { } System.out.println("-----------------------------------------------------------"); - clsList.forEach(IntegrationTest::checkCode); + clsList.forEach(this::checkCode); compile(clsList); clsList.forEach(this::runAutoCheck); } @@ -212,7 +214,7 @@ public abstract class IntegrationTest extends TestUtils { } } - protected static void checkCode(ClassNode cls) { + protected void checkCode(ClassNode cls) { assertFalse(hasErrors(cls), "Inconsistent cls: " + cls); for (MethodNode mthNode : cls.getMethods()) { assertFalse(hasErrors(mthNode), "Method with problems: " + mthNode); @@ -220,17 +222,19 @@ public abstract class IntegrationTest extends TestUtils { assertThat(cls.getCode().toString(), not(containsString("inconsistent"))); } - private static boolean hasErrors(IAttributeNode node) { + private boolean hasErrors(IAttributeNode node) { if (node.contains(AFlag.INCONSISTENT_CODE) || node.contains(AType.JADX_ERROR) - || node.contains(AType.JADX_WARN)) { + || (node.contains(AType.JADX_WARN) && !allowWarnInCode)) { return true; } - AttrList commentsAttr = node.get(AType.COMMENTS); - if (commentsAttr != null) { - for (String comment : commentsAttr.getList()) { - if (comment.contains("JADX WARN")) { - return true; + if (!allowWarnInCode) { + AttrList commentsAttr = node.get(AType.COMMENTS); + if (commentsAttr != null) { + for (String comment : commentsAttr.getList()) { + if (comment.contains("JADX WARN")) { + return true; + } } } } @@ -444,6 +448,10 @@ public abstract class IntegrationTest extends TestUtils { args.setDeobfuscationMaxLength(64); } + protected void allowWarnInCode() { + allowWarnInCode = true; + } + // Use only for debug purpose @Deprecated protected void outputCFG() { diff --git a/jadx-core/src/test/java/jadx/tests/integration/others/TestInsnsBeforeSuper.java b/jadx-core/src/test/java/jadx/tests/integration/others/TestInsnsBeforeSuper.java index f31c71109..c0185d659 100644 --- a/jadx-core/src/test/java/jadx/tests/integration/others/TestInsnsBeforeSuper.java +++ b/jadx-core/src/test/java/jadx/tests/integration/others/TestInsnsBeforeSuper.java @@ -33,6 +33,7 @@ public class TestInsnsBeforeSuper extends SmaliTest { @Test public void test() { + allowWarnInCode(); ClassNode cls = getClassNodeFromSmaliFiles("B"); String code = cls.getCode().toString(); diff --git a/jadx-core/src/test/java/jadx/tests/integration/others/TestInsnsBeforeThis.java b/jadx-core/src/test/java/jadx/tests/integration/others/TestInsnsBeforeThis.java new file mode 100644 index 000000000..0fe08b2e2 --- /dev/null +++ b/jadx-core/src/test/java/jadx/tests/integration/others/TestInsnsBeforeThis.java @@ -0,0 +1,40 @@ +package jadx.tests.integration.others; + +import org.junit.jupiter.api.Test; + +import jadx.core.dex.nodes.ClassNode; +import jadx.tests.api.SmaliTest; + +import static jadx.tests.api.utils.JadxMatchers.containsOne; +import static org.hamcrest.MatcherAssert.assertThat; + +public class TestInsnsBeforeThis extends SmaliTest { + // @formatter:off + /* + public class A { + public A(String str) { + checkNull(str); + this(str.length()); + } + + public A(int i) { + } + + public void checkNull(Object o) { + if (o == null) { + throw new NullPointerException(); + } + } + } + */ + // @formatter:on + + @Test + public void test() { + allowWarnInCode(); + ClassNode cls = getClassNodeFromSmali(); + String code = cls.getCode().toString(); + + assertThat(code, containsOne("// checkNull(str);")); + } +} diff --git a/jadx-core/src/test/smali/others/TestInsnsBeforeThis.smali b/jadx-core/src/test/smali/others/TestInsnsBeforeThis.smali new file mode 100644 index 000000000..deb226b0b --- /dev/null +++ b/jadx-core/src/test/smali/others/TestInsnsBeforeThis.smali @@ -0,0 +1,39 @@ +.class public Lothers/TestInsnsBeforeThis; +.super Ljava/lang/Object; + +.method public constructor (Ljava/lang/String;)V + .registers 3 + + .prologue + invoke-static {p1}, Lothers/TestInsnsBeforeThis;->checkNull(Ljava/lang/Object;)V + + invoke-direct {p1}, Ljava/lang/String;->length()I + move-result v0 + + invoke-direct {p0, v0}, Lothers/TestInsnsBeforeThis;->(I)V + + return-void +.end method + +.method public constructor (I)V + .registers 3 + + .prologue + invoke-direct {p0}, Ljava/lang/Object;->()V + + return-void +.end method + +.method public static checkNull(Ljava/lang/Object;)V + .registers 3 + + .prologue + if-nez p0, :cond_8 + + new-instance v0, Ljava/lang/NullPointerException; + invoke-direct {v0}, Ljava/lang/NullPointerException;->()V + throw v0 + + :cond_8 + return-void +.end method