Skip to content

Commit

Permalink
fix: comment out instructions also before other constructor call (#685)
Browse files Browse the repository at this point in the history
  • Loading branch information
skylot committed Jul 5, 2019
1 parent c6c54f9 commit 533b686
Show file tree
Hide file tree
Showing 6 changed files with 189 additions and 29 deletions.
16 changes: 14 additions & 2 deletions jadx-core/src/main/java/jadx/core/codegen/RegionGen.java
Original file line number Diff line number Diff line change
Expand Up @@ -131,12 +131,20 @@ private void makeIf(IfRegion region, CodeWriter code, boolean newLine) throws Co
}
}
}
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)) {
Expand All @@ -146,7 +154,11 @@ private void makeIf(IfRegion region, CodeWriter code, boolean newLine) throws Co
}
code.add('{');
makeRegionIndent(code, els);
code.startLine('}');
if (comment) {
code.startLine("// }");
} else {
code.startLine('}');
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,20 +2,19 @@

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;
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;
Expand All @@ -38,8 +37,6 @@
)
public class PrepareForCodeGen extends AbstractVisitor {

public static final SuperCallRegionVisitor SUPER_CALL_REGION_VISITOR = new SuperCallRegionVisitor();

@Override
public void visit(MethodNode mth) throws JadxException {
List<BlockNode> blocks = mth.getBasicBlocks();
Expand All @@ -55,7 +52,7 @@ public void visit(MethodNode mth) throws JadxException {
removeParenthesis(block);
modifyArith(block);
}
commentOutInsnsBeforeSuper(mth);
commentOutInsnsInConstructor(mth);
}

private static void removeInstructions(BlockNode block) {
Expand Down Expand Up @@ -180,31 +177,94 @@ private static void modifyArith(BlockNode block) {
}
}

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 {
@Override
public void processBlock(MethodNode mth, IBlock container) {
for (InsnNode insn : container.getInstructions()) {
private ConstructorInsn searchConstructorCall(MethodNode mth) {
for (BlockNode block : mth.getBasicBlocks()) {
for (InsnNode insn : block.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 (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;
}

private static void commentOutInsns(IBlock container, InsnNode superInsn) {
@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()) {
if (insn == superInsn) {
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 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);
}
}
}
}
Expand Down
26 changes: 17 additions & 9 deletions jadx-core/src/test/java/jadx/tests/api/IntegrationTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,8 @@ public abstract class IntegrationTest extends TestUtils {
protected boolean useEclipseCompiler;
protected Map<Integer, String> resMap = Collections.emptyMap();

private boolean allowWarnInCode;

private DynamicCompiler dynamicCompiler;

@BeforeEach
Expand Down Expand Up @@ -170,7 +172,7 @@ protected void decompileAndCheck(JadxDecompiler d, List<ClassNode> clsList) {
}
System.out.println("-----------------------------------------------------------");

clsList.forEach(IntegrationTest::checkCode);
clsList.forEach(this::checkCode);
compile(clsList);
clsList.forEach(this::runAutoCheck);
}
Expand Down Expand Up @@ -212,25 +214,27 @@ protected void generateClsCode(ClassNode cls) {
}
}

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);
}
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<String> commentsAttr = node.get(AType.COMMENTS);
if (commentsAttr != null) {
for (String comment : commentsAttr.getList()) {
if (comment.contains("JADX WARN")) {
return true;
if (!allowWarnInCode) {
AttrList<String> commentsAttr = node.get(AType.COMMENTS);
if (commentsAttr != null) {
for (String comment : commentsAttr.getList()) {
if (comment.contains("JADX WARN")) {
return true;
}
}
}
}
Expand Down Expand Up @@ -444,6 +448,10 @@ protected void enableDeobfuscation() {
args.setDeobfuscationMaxLength(64);
}

protected void allowWarnInCode() {
allowWarnInCode = true;
}

// Use only for debug purpose
@Deprecated
protected void outputCFG() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ public void checkNull(Object o) {

@Test
public void test() {
allowWarnInCode();
ClassNode cls = getClassNodeFromSmaliFiles("B");
String code = cls.getCode().toString();

Expand Down
Original file line number Diff line number Diff line change
@@ -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);"));
}
}
39 changes: 39 additions & 0 deletions jadx-core/src/test/smali/others/TestInsnsBeforeThis.smali
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
.class public Lothers/TestInsnsBeforeThis;
.super Ljava/lang/Object;

.method public constructor <init>(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;-><init>(I)V

return-void
.end method

.method public constructor <init>(I)V
.registers 3

.prologue
invoke-direct {p0}, Ljava/lang/Object;-><init>()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;-><init>()V
throw v0

:cond_8
return-void
.end method

0 comments on commit 533b686

Please sign in to comment.