From a71b3a71d816bd188b0541afa8fcbb8c399518a6 Mon Sep 17 00:00:00 2001 From: Skylot Date: Tue, 26 Apr 2022 20:10:33 +0100 Subject: [PATCH] fix: better code styling for `if-else` blocks (#1455) --- .../java/jadx/core/dex/attributes/AFlag.java | 1 + .../dex/regions/conditions/IfCondition.java | 10 + .../debuginfo/DebugInfoAttachVisitor.java | 19 +- .../dex/visitors/regions/IfRegionVisitor.java | 180 +++++---- .../core/dex/visitors/regions/TernaryMod.java | 5 +- .../main/java/jadx/core/utils/BlockUtils.java | 27 +- .../java/jadx/core/utils/RegionUtils.java | 90 ++++- .../java/jadx/tests/api/IntegrationTest.java | 6 +- .../conditions/TestBitwiseAnd.java | 1 + .../conditions/TestConditions14.java | 1 + .../conditions/TestIfCodeStyle.java | 160 ++++++++ .../debuginfo/TestLineNumbers2.java | 11 +- .../loops/TestSequentialLoops.java | 20 +- .../trycatch/TestLoopInTryCatch.java | 6 +- .../smali/conditions/TestIfCodeStyle.smali | 354 ++++++++++++++++++ .../smali/trycatch/TestLoopInTryCatch.smali | 2 +- .../sections/debuginfo/DebugInfoParser.java | 33 +- 17 files changed, 782 insertions(+), 144 deletions(-) create mode 100644 jadx-core/src/test/java/jadx/tests/integration/conditions/TestIfCodeStyle.java create mode 100644 jadx-core/src/test/smali/conditions/TestIfCodeStyle.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 c8aedde8..3a77ad21 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 @@ -80,6 +80,7 @@ public enum AFlag { RERUN_SSA_TRANSFORM, METHOD_CANDIDATE_FOR_INLINE, + USE_LINES_HINTS, // source lines info in methods can be trusted DISABLE_BLOCKS_LOCK, diff --git a/jadx-core/src/main/java/jadx/core/dex/regions/conditions/IfCondition.java b/jadx-core/src/main/java/jadx/core/dex/regions/conditions/IfCondition.java index 2e0ed000..9f1d96be 100644 --- a/jadx-core/src/main/java/jadx/core/dex/regions/conditions/IfCondition.java +++ b/jadx-core/src/main/java/jadx/core/dex/regions/conditions/IfCondition.java @@ -278,6 +278,16 @@ public final class IfCondition extends AttrNode { return list; } + public int getSourceLine() { + for (InsnNode insn : collectInsns()) { + int line = insn.getSourceLine(); + if (line != 0) { + return line; + } + } + return 0; + } + @Nullable public InsnNode getFirstInsn() { if (mode == Mode.COMPARE) { diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/debuginfo/DebugInfoAttachVisitor.java b/jadx-core/src/main/java/jadx/core/dex/visitors/debuginfo/DebugInfoAttachVisitor.java index 2229c510..35b71c88 100644 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/debuginfo/DebugInfoAttachVisitor.java +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/debuginfo/DebugInfoAttachVisitor.java @@ -1,12 +1,15 @@ package jadx.core.dex.visitors.debuginfo; +import java.util.HashMap; import java.util.List; import java.util.Map; import jadx.api.plugins.input.data.IDebugInfo; import jadx.api.plugins.input.data.ILocalVar; +import jadx.core.dex.attributes.AFlag; import jadx.core.dex.attributes.nodes.LocalVarsDebugInfoAttr; import jadx.core.dex.attributes.nodes.RegDebugInfoAttr; +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.RegisterArg; @@ -17,6 +20,7 @@ import jadx.core.dex.visitors.AbstractVisitor; import jadx.core.dex.visitors.JadxVisitor; import jadx.core.dex.visitors.blocks.BlockSplitter; import jadx.core.dex.visitors.ssa.SSATransform; +import jadx.core.utils.ListUtils; import jadx.core.utils.exceptions.JadxException; @JadxVisitor( @@ -52,17 +56,30 @@ public class DebugInfoAttachVisitor extends AbstractVisitor { if (lineMapping.isEmpty()) { return; } + Map linesStat = new HashMap<>(); // count repeating lines for (Map.Entry entry : lineMapping.entrySet()) { try { Integer offset = entry.getKey(); InsnNode insn = insnArr[offset]; if (insn != null) { - insn.setSourceLine(entry.getValue()); + int line = entry.getValue(); + insn.setSourceLine(line); + if (insn.getType() != InsnType.NOP) { + linesStat.merge(line, 1, (v, one) -> v + 1); + } } } catch (Exception e) { mth.addWarnComment("Error attach source line", e); } } + // 3 here is allowed maximum for lines repeat, + // can occur in indexed 'for' loops (3 instructions with same line) + List> repeatingLines = ListUtils.filter(linesStat.entrySet(), p -> p.getValue() > 3); + if (repeatingLines.isEmpty()) { + mth.add(AFlag.USE_LINES_HINTS); + } else { + mth.addDebugComment("Don't trust debug lines info. Repeating lines: " + repeatingLines); + } } private void attachDebugInfo(MethodNode mth, List localVars, InsnNode[] insnArr) { diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/regions/IfRegionVisitor.java b/jadx-core/src/main/java/jadx/core/dex/visitors/regions/IfRegionVisitor.java index 46aa981f..ec0d60a9 100644 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/regions/IfRegionVisitor.java +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/regions/IfRegionVisitor.java @@ -38,15 +38,105 @@ public class IfRegionVisitor extends AbstractVisitor { public boolean enterRegion(MethodNode mth, IRegion region) { if (region instanceof IfRegion) { IfRegion ifRegion = (IfRegion) region; - simplifyIfCondition(ifRegion); - moveReturnToThenBlock(mth, ifRegion); - moveBreakToThenBlock(ifRegion); - markElseIfChains(ifRegion); + orderBranches(mth, ifRegion); + markElseIfChains(mth, ifRegion); } return true; } } + @SuppressWarnings("UnnecessaryReturnStatement") + private static void orderBranches(MethodNode mth, IfRegion ifRegion) { + if (RegionUtils.isEmpty(ifRegion.getElseRegion())) { + return; + } + if (RegionUtils.isEmpty(ifRegion.getThenRegion())) { + invertIfRegion(ifRegion); + return; + } + if (mth.contains(AFlag.USE_LINES_HINTS)) { + int thenLine = RegionUtils.getFirstSourceLine(ifRegion.getThenRegion()); + int elseLine = RegionUtils.getFirstSourceLine(ifRegion.getElseRegion()); + if (thenLine != 0 && elseLine != 0) { + if (thenLine > elseLine) { + invertIfRegion(ifRegion); + } + return; + } + } + if (ifRegion.simplifyCondition()) { + IfCondition condition = ifRegion.getCondition(); + if (condition != null && condition.getMode() == Mode.NOT) { + invertIfRegion(ifRegion); + } + } + int thenSize = insnsCount(ifRegion.getThenRegion()); + int elseSize = insnsCount(ifRegion.getElseRegion()); + if (isSimpleExitBlock(mth, ifRegion.getElseRegion())) { + if (isSimpleExitBlock(mth, ifRegion.getThenRegion())) { + if (elseSize < thenSize) { + invertIfRegion(ifRegion); + return; + } + } + boolean lastRegion = ifRegion == RegionUtils.getLastRegion(mth.getRegion()); + if (elseSize == 1 && lastRegion && mth.isVoidReturn()) { + // single return at method end will be removed later + return; + } + if (!lastRegion) { + invertIfRegion(ifRegion); + } + return; + } + boolean thenExit = RegionUtils.hasExitBlock(ifRegion.getThenRegion()); + boolean elseExit = RegionUtils.hasExitBlock(ifRegion.getElseRegion()); + if (elseExit && (!thenExit || elseSize < thenSize)) { + invertIfRegion(ifRegion); + return; + } + // move 'if' from 'then' branch to make 'else if' chain + if (isIfRegion(ifRegion.getThenRegion()) + && !isIfRegion(ifRegion.getElseRegion()) + && !thenExit) { + invertIfRegion(ifRegion); + return; + } + // move 'break' into 'then' branch + if (RegionUtils.hasBreakInsn(ifRegion.getElseRegion())) { + invertIfRegion(ifRegion); + return; + } + } + + private static boolean isIfRegion(IContainer container) { + if (container instanceof IfRegion) { + return true; + } + if (container instanceof IRegion) { + List subBlocks = ((IRegion) container).getSubBlocks(); + return subBlocks.size() == 1 && subBlocks.get(0) instanceof IfRegion; + } + return false; + } + + /** + * Mark if-else-if chains + */ + private static void markElseIfChains(MethodNode mth, IfRegion ifRegion) { + if (isSimpleExitBlock(mth, ifRegion.getThenRegion())) { + return; + } + IContainer elsRegion = ifRegion.getElseRegion(); + if (elsRegion instanceof Region) { + List subBlocks = ((Region) elsRegion).getSubBlocks(); + if (subBlocks.size() == 1 && subBlocks.get(0) instanceof IfRegion) { + subBlocks.get(0).add(AFlag.ELSE_IF_CHAIN); + elsRegion.add(AFlag.ELSE_IF_CHAIN); + } + } + } + private static class RemoveRedundantElseVisitor implements IRegionIterativeVisitor { @Override public boolean visitRegion(MethodNode mth, IRegion region) { @@ -57,76 +147,6 @@ public class IfRegionVisitor extends AbstractVisitor { } } - private static void simplifyIfCondition(IfRegion ifRegion) { - if (ifRegion.simplifyCondition()) { - IfCondition condition = ifRegion.getCondition(); - if (condition.getMode() == Mode.NOT) { - invertIfRegion(ifRegion); - } - } - IContainer elseRegion = ifRegion.getElseRegion(); - if (elseRegion == null || RegionUtils.isEmpty(elseRegion)) { - return; - } - boolean thenIsEmpty = RegionUtils.isEmpty(ifRegion.getThenRegion()); - if (thenIsEmpty || hasSimpleReturnBlock(ifRegion.getThenRegion())) { - invertIfRegion(ifRegion); - } - - if (!thenIsEmpty) { - // move 'if' from then to make 'else if' chain - if (isIfRegion(ifRegion.getThenRegion()) - && !isIfRegion(elseRegion)) { - invertIfRegion(ifRegion); - } - } - } - - private static boolean isIfRegion(IContainer container) { - if (container instanceof IfRegion) { - return true; - } - if (container instanceof IRegion) { - List subBlocks = ((IRegion) container).getSubBlocks(); - if (subBlocks.size() == 1 && subBlocks.get(0) instanceof IfRegion) { - return true; - } - } - return false; - } - - private static void moveReturnToThenBlock(MethodNode mth, IfRegion ifRegion) { - if (!mth.isVoidReturn() - && hasSimpleReturnBlock(ifRegion.getElseRegion()) - /* && insnsCount(ifRegion.getThenRegion()) < 2 */) { - invertIfRegion(ifRegion); - } - } - - private static void moveBreakToThenBlock(IfRegion ifRegion) { - if (ifRegion.getElseRegion() != null - && RegionUtils.hasBreakInsn(ifRegion.getElseRegion())) { - invertIfRegion(ifRegion); - } - } - - /** - * Mark if-else-if chains - */ - private static void markElseIfChains(IfRegion ifRegion) { - if (hasSimpleReturnBlock(ifRegion.getThenRegion())) { - return; - } - IContainer elsRegion = ifRegion.getElseRegion(); - if (elsRegion instanceof Region) { - List subBlocks = ((Region) elsRegion).getSubBlocks(); - if (subBlocks.size() == 1 && subBlocks.get(0) instanceof IfRegion) { - subBlocks.get(0).add(AFlag.ELSE_IF_CHAIN); - elsRegion.add(AFlag.ELSE_IF_CHAIN); - } - } - } - private static boolean removeRedundantElseBlock(MethodNode mth, IfRegion ifRegion) { if (ifRegion.getElseRegion() == null || ifRegion.contains(AFlag.ELSE_IF_CHAIN) @@ -162,16 +182,16 @@ public class IfRegionVisitor extends AbstractVisitor { } } - private static boolean hasSimpleReturnBlock(IContainer region) { - if (region == null) { + private static boolean isSimpleExitBlock(MethodNode mth, IContainer container) { + if (container == null) { return false; } - if (region.contains(AFlag.RETURN)) { + if (container.contains(AFlag.RETURN) || RegionUtils.isExitBlock(mth, container)) { return true; } - if (region instanceof IRegion) { - List subBlocks = ((IRegion) region).getSubBlocks(); - return subBlocks.size() == 1 && subBlocks.get(0).contains(AFlag.RETURN); + if (container instanceof IRegion) { + List subBlocks = ((IRegion) container).getSubBlocks(); + return subBlocks.size() == 1 && RegionUtils.isExitBlock(mth, subBlocks.get(0)); } return false; } diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/regions/TernaryMod.java b/jadx-core/src/main/java/jadx/core/dex/visitors/regions/TernaryMod.java index 60008bc4..fd0a8e5d 100644 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/regions/TernaryMod.java +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/regions/TernaryMod.java @@ -93,7 +93,8 @@ public class TernaryMod extends AbstractRegionVisitor implements IRegionIterativ InsnNode thenInsn = tb.getInstructions().get(0); InsnNode elseInsn = eb.getInstructions().get(0); - if (thenInsn.getSourceLine() != elseInsn.getSourceLine()) { + if (mth.contains(AFlag.USE_LINES_HINTS) + && thenInsn.getSourceLine() != elseInsn.getSourceLine()) { if (thenInsn.getSourceLine() != 0 && elseInsn.getSourceLine() != 0) { // sometimes source lines incorrect if (!checkLineStats(thenInsn, elseInsn)) { @@ -134,6 +135,8 @@ public class TernaryMod extends AbstractRegionVisitor implements IRegionIterativ InsnArg thenArg = InsnArg.wrapInsnIntoArg(thenInsn); InsnArg elseArg = InsnArg.wrapInsnIntoArg(elseInsn); TernaryInsn ternInsn = new TernaryInsn(ifRegion.getCondition(), resArg, thenArg, elseArg); + int branchLine = Math.max(thenInsn.getSourceLine(), elseInsn.getSourceLine()); + ternInsn.setSourceLine(Math.max(ifRegion.getSourceLine(), branchLine)); InsnRemover.unbindResult(mth, elseInsn); 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 3a296ee8..daee89e1 100644 --- a/jadx-core/src/main/java/jadx/core/utils/BlockUtils.java +++ b/jadx-core/src/main/java/jadx/core/utils/BlockUtils.java @@ -182,6 +182,16 @@ public class BlockUtils { return null; } + public static int getFirstSourceLine(IBlock block) { + for (InsnNode insn : block.getInstructions()) { + int line = insn.getSourceLine(); + if (line != 0) { + return line; + } + } + return 0; + } + @Nullable public static InsnNode getFirstInsn(@Nullable IBlock block) { if (block == null) { @@ -207,11 +217,22 @@ public class BlockUtils { } public static boolean isExitBlock(MethodNode mth, BlockNode block) { - BlockNode exitBlock = mth.getExitBlock(); - if (block == exitBlock) { + if (block == mth.getExitBlock()) { return true; } - return exitBlock.getPredecessors().contains(block); + return isExitBlock(block); + } + + public static boolean isExitBlock(BlockNode block) { + List successors = block.getSuccessors(); + if (successors.isEmpty()) { + return true; + } + if (successors.size() == 1) { + BlockNode next = successors.get(0); + return next.getSuccessors().isEmpty(); + } + return false; } public static boolean containsExitInsn(IBlock block) { diff --git a/jadx-core/src/main/java/jadx/core/utils/RegionUtils.java b/jadx-core/src/main/java/jadx/core/utils/RegionUtils.java index 6f7e6372..ef6f9007 100644 --- a/jadx-core/src/main/java/jadx/core/utils/RegionUtils.java +++ b/jadx-core/src/main/java/jadx/core/utils/RegionUtils.java @@ -16,6 +16,7 @@ import jadx.core.dex.instructions.InsnType; import jadx.core.dex.nodes.BlockNode; import jadx.core.dex.nodes.IBlock; import jadx.core.dex.nodes.IBranchRegion; +import jadx.core.dex.nodes.IConditionRegion; import jadx.core.dex.nodes.IContainer; import jadx.core.dex.nodes.IRegion; import jadx.core.dex.nodes.InsnNode; @@ -52,6 +53,7 @@ public class RegionUtils { throw new JadxRuntimeException(unknownContainerType(container)); } + @Nullable public static InsnNode getFirstInsn(IContainer container) { if (container instanceof IBlock) { IBlock block = (IBlock) container; @@ -74,6 +76,37 @@ public class RegionUtils { } } + public static int getFirstSourceLine(IContainer container) { + if (container instanceof IBlock) { + return BlockUtils.getFirstSourceLine((IBlock) container); + } + if (container instanceof IConditionRegion) { + return ((IConditionRegion) container).getConditionSourceLine(); + } + if (container instanceof IBranchRegion) { + IBranchRegion branchRegion = (IBranchRegion) container; + return getFirstSourceLine(branchRegion.getBranches()); + } + if (container instanceof IRegion) { + IRegion region = (IRegion) container; + return getFirstSourceLine(region.getSubBlocks()); + } + return 0; + } + + private static int getFirstSourceLine(List containers) { + if (containers.isEmpty()) { + return 0; + } + for (IContainer container : containers) { + int line = getFirstSourceLine(container); + if (line != 0) { + return line; + } + } + return 0; + } + public static InsnNode getLastInsn(IContainer container) { if (container instanceof IBlock) { IBlock block = (IBlock) container; @@ -112,31 +145,58 @@ public class RegionUtils { } } + @Nullable + public static IContainer getLastRegion(@Nullable IContainer container) { + if (container == null) { + return null; + } + if (container instanceof IBlock || container instanceof IBranchRegion) { + return container; + } + if (container instanceof IRegion) { + return getLastRegion(Utils.last(((IRegion) container).getSubBlocks())); + } + throw new JadxRuntimeException(unknownContainerType(container)); + } + + public static boolean isExitBlock(MethodNode mth, IContainer container) { + if (container instanceof BlockNode) { + return BlockUtils.isExitBlock(mth, (BlockNode) container); + } + return false; + } + /** * Return true if last block in region has no successors or jump out insn (return or break) */ public static boolean hasExitBlock(IContainer container) { + if (container == null) { + return false; + } return hasExitBlock(container, container); } private static boolean hasExitBlock(IContainer rootContainer, IContainer container) { if (container instanceof BlockNode) { BlockNode blockNode = (BlockNode) container; - if (blockNode.getSuccessors().isEmpty()) { + if (BlockUtils.isExitBlock(blockNode)) { return true; } return isInsnExitContainer(rootContainer, (IBlock) container); - } else if (container instanceof IBranchRegion) { - return false; - } else if (container instanceof IBlock) { + } + if (container instanceof IBranchRegion) { + IBranchRegion branchRegion = (IBranchRegion) container; + return ListUtils.allMatch(branchRegion.getBranches(), RegionUtils::hasExitBlock); + } + if (container instanceof IBlock) { return isInsnExitContainer(rootContainer, (IBlock) container); - } else if (container instanceof IRegion) { + } + if (container instanceof IRegion) { List blocks = ((IRegion) container).getSubBlocks(); return !blocks.isEmpty() && hasExitBlock(rootContainer, blocks.get(blocks.size() - 1)); - } else { - throw new JadxRuntimeException(unknownContainerType(container)); } + throw new JadxRuntimeException(unknownContainerType(container)); } private static boolean isInsnExitContainer(IContainer rootContainer, IBlock block) { @@ -192,17 +252,25 @@ public class RegionUtils { public static int insnsCount(IContainer container) { if (container instanceof IBlock) { - return ((IBlock) container).getInstructions().size(); - } else if (container instanceof IRegion) { + List insnList = ((IBlock) container).getInstructions(); + int count = 0; + for (InsnNode insn : insnList) { + if (insn.contains(AFlag.DONT_GENERATE)) { + continue; + } + count++; + } + return count; + } + if (container instanceof IRegion) { IRegion region = (IRegion) container; int count = 0; for (IContainer block : region.getSubBlocks()) { count += insnsCount(block); } return count; - } else { - throw new JadxRuntimeException(unknownContainerType(container)); } + throw new JadxRuntimeException(unknownContainerType(container)); } public static boolean isEmpty(IContainer container) { 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 582b3b09..16d70bf3 100644 --- a/jadx-core/src/test/java/jadx/tests/api/IntegrationTest.java +++ b/jadx-core/src/test/java/jadx/tests/api/IntegrationTest.java @@ -150,9 +150,9 @@ public abstract class IntegrationTest extends TestUtils { close(decompiledCompiler); } - private void close(Closeable cloaseble) throws IOException { - if (cloaseble != null) { - cloaseble.close(); + private void close(Closeable closeable) throws IOException { + if (closeable != null) { + closeable.close(); } } diff --git a/jadx-core/src/test/java/jadx/tests/integration/conditions/TestBitwiseAnd.java b/jadx-core/src/test/java/jadx/tests/integration/conditions/TestBitwiseAnd.java index 0946eaca..f99da069 100644 --- a/jadx-core/src/test/java/jadx/tests/integration/conditions/TestBitwiseAnd.java +++ b/jadx-core/src/test/java/jadx/tests/integration/conditions/TestBitwiseAnd.java @@ -8,6 +8,7 @@ import jadx.tests.api.IntegrationTest; import static jadx.tests.api.utils.JadxMatchers.containsOne; import static org.hamcrest.MatcherAssert.assertThat; +@SuppressWarnings({ "PointlessBooleanExpression", "unused" }) public class TestBitwiseAnd extends IntegrationTest { public static class TestCls { diff --git a/jadx-core/src/test/java/jadx/tests/integration/conditions/TestConditions14.java b/jadx-core/src/test/java/jadx/tests/integration/conditions/TestConditions14.java index bbe07f11..f7131a43 100644 --- a/jadx-core/src/test/java/jadx/tests/integration/conditions/TestConditions14.java +++ b/jadx-core/src/test/java/jadx/tests/integration/conditions/TestConditions14.java @@ -10,6 +10,7 @@ import static org.hamcrest.MatcherAssert.assertThat; public class TestConditions14 extends IntegrationTest { + @SuppressWarnings({ "EqualsReplaceableByObjectsCall", "ConstantConditions" }) public static class TestCls { public static boolean test(Object a, Object b) { diff --git a/jadx-core/src/test/java/jadx/tests/integration/conditions/TestIfCodeStyle.java b/jadx-core/src/test/java/jadx/tests/integration/conditions/TestIfCodeStyle.java new file mode 100644 index 00000000..9fb5f113 --- /dev/null +++ b/jadx-core/src/test/java/jadx/tests/integration/conditions/TestIfCodeStyle.java @@ -0,0 +1,160 @@ +package jadx.tests.integration.conditions; + +import org.junit.jupiter.api.Test; + +import jadx.tests.api.SmaliTest; + +import static jadx.tests.api.utils.assertj.JadxAssertions.assertThat; + +/** + * Issue #1455 + */ +public class TestIfCodeStyle extends SmaliTest { + + @SuppressWarnings({ "ConstantConditions", "FieldCanBeLocal", "unused" }) + public static class TestCls { + + private String moduleName; + private String modulePath; + private String preinstalledModulePath; + private long versionCode; + private String versionName; + private boolean isFactory; + private boolean isActive; + + public void test(Parcel parcel) { + int startPos = parcel.dataPosition(); + int size = parcel.readInt(); + if (size < 0) { + if (startPos > Integer.MAX_VALUE - size) { + throw new RuntimeException("Overflow in the size of parcelable"); + } + parcel.setDataPosition(startPos + size); + return; + } + try { + if (parcel.dataPosition() - startPos >= size) { + if (startPos > Integer.MAX_VALUE - size) { + throw new RuntimeException("Overflow in the size of parcelable"); + } + parcel.setDataPosition(startPos + size); + return; + } + this.moduleName = parcel.readString(); + if (parcel.dataPosition() - startPos >= size) { + if (startPos > Integer.MAX_VALUE - size) { + throw new RuntimeException("Overflow in the size of parcelable"); + } + parcel.setDataPosition(startPos + size); + return; + } + this.modulePath = parcel.readString(); + if (parcel.dataPosition() - startPos >= size) { + if (startPos > Integer.MAX_VALUE - size) { + throw new RuntimeException("Overflow in the size of parcelable"); + } + parcel.setDataPosition(startPos + size); + return; + } + this.preinstalledModulePath = parcel.readString(); + if (parcel.dataPosition() - startPos >= size) { + if (startPos > Integer.MAX_VALUE - size) { + throw new RuntimeException("Overflow in the size of parcelable"); + } + parcel.setDataPosition(startPos + size); + return; + } + this.versionCode = parcel.readLong(); + if (parcel.dataPosition() - startPos >= size) { + if (startPos > Integer.MAX_VALUE - size) { + throw new RuntimeException("Overflow in the size of parcelable"); + } + parcel.setDataPosition(startPos + size); + return; + } + this.versionName = parcel.readString(); + if (parcel.dataPosition() - startPos >= size) { + if (startPos > Integer.MAX_VALUE - size) { + throw new RuntimeException("Overflow in the size of parcelable"); + } + parcel.setDataPosition(startPos + size); + return; + } + this.isFactory = parcel.readInt() != 0; + if (parcel.dataPosition() - startPos >= size) { + if (startPos > Integer.MAX_VALUE - size) { + throw new RuntimeException("Overflow in the size of parcelable"); + } + parcel.setDataPosition(startPos + size); + return; + } + this.isActive = parcel.readInt() != 0; + if (startPos > Integer.MAX_VALUE - size) { + throw new RuntimeException("Overflow in the size of parcelable"); + } + parcel.setDataPosition(startPos + size); + } catch (Throwable e) { + if (startPos <= Integer.MAX_VALUE - size) { + parcel.setDataPosition(startPos + size); + throw e; + } + throw new RuntimeException("Overflow in the size of parcelable"); + } + } + + private static class Parcel { + public void setDataPosition(int i) { + } + + public int dataPosition() { + return 0; + } + + public int readInt() { + return 0; + } + + public String readString() { + return null; + } + + public long readLong() { + return 0; + } + } + } + + @Test + public void test() { + noDebugInfo(); + assertThat(getClassNode(TestCls.class)) + .code() + .doesNotContain("else") + .countString(8, "return;") + .containsLines(2, + "if (readInt < 0) {", + indent() + "if (dataPosition > Integer.MAX_VALUE - readInt) {", + indent(2) + "throw new RuntimeException(\"Overflow in the size of parcelable\");", + indent() + "}", + indent() + "parcel.setDataPosition(dataPosition + readInt);", + indent() + "return;", + "}"); + } + + @Test + public void testSmali() { + disableCompilation(); + assertThat(getClassNodeFromSmali()) + .code() + .doesNotContain("else") + .countString(8, "return;") + .containsLines(2, + "if (_aidl_parcelable_size < 0) {", + indent() + "if (_aidl_start_pos > Integer.MAX_VALUE - _aidl_parcelable_size) {", + indent(2) + "throw new RuntimeException(\"Overflow in the size of parcelable\");", + indent() + "}", + indent() + "_aidl_parcel.setDataPosition(_aidl_start_pos + _aidl_parcelable_size);", + indent() + "return;", + "}"); + } +} diff --git a/jadx-core/src/test/java/jadx/tests/integration/debuginfo/TestLineNumbers2.java b/jadx-core/src/test/java/jadx/tests/integration/debuginfo/TestLineNumbers2.java index 8cf551f5..895b740c 100644 --- a/jadx-core/src/test/java/jadx/tests/integration/debuginfo/TestLineNumbers2.java +++ b/jadx-core/src/test/java/jadx/tests/integration/debuginfo/TestLineNumbers2.java @@ -2,10 +2,10 @@ package jadx.tests.integration.debuginfo; import java.lang.ref.WeakReference; -import org.junit.jupiter.api.Test; - import jadx.core.dex.nodes.ClassNode; import jadx.tests.api.IntegrationTest; +import jadx.tests.api.extensions.profiles.TestProfile; +import jadx.tests.api.extensions.profiles.TestWithProfiles; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -32,17 +32,16 @@ public class TestLineNumbers2 extends IntegrationTest { } } - @Test + @TestWithProfiles({ TestProfile.DX_J8, TestProfile.JAVA8 }) public void test() { printLineNumbers(); ClassNode cls = getClassNode(TestCls.class); String linesMapStr = cls.getCode().getLineMapping().toString(); if (isJavaInput()) { - assertEquals("{6=16, 9=17, 12=21, 13=22, 14=23, 16=25, 18=27, 21=30}", linesMapStr); + assertEquals("{6=16, 9=17, 12=21, 13=22, 14=23, 15=24, 16=25, 18=27, 21=30, 22=31}", linesMapStr); } else { - // TODO: invert condition to match source lines - assertEquals("{6=16, 9=17, 12=21, 13=22, 14=23, 15=27, 17=24, 18=25, 19=27, 22=30, 23=31}", linesMapStr); + assertEquals("{6=16, 9=17, 12=21, 13=22, 14=23, 15=24, 16=25, 17=27, 19=27, 22=30, 23=31}", linesMapStr); } } } diff --git a/jadx-core/src/test/java/jadx/tests/integration/loops/TestSequentialLoops.java b/jadx-core/src/test/java/jadx/tests/integration/loops/TestSequentialLoops.java index f2b722a8..99303c4a 100644 --- a/jadx-core/src/test/java/jadx/tests/integration/loops/TestSequentialLoops.java +++ b/jadx-core/src/test/java/jadx/tests/integration/loops/TestSequentialLoops.java @@ -2,14 +2,9 @@ package jadx.tests.integration.loops; import org.junit.jupiter.api.Test; -import jadx.core.dex.nodes.ClassNode; import jadx.tests.api.IntegrationTest; -import static jadx.tests.api.utils.JadxMatchers.containsOne; -import static jadx.tests.api.utils.JadxMatchers.countString; -import static org.hamcrest.CoreMatchers.containsString; -import static org.hamcrest.CoreMatchers.not; -import static org.hamcrest.MatcherAssert.assertThat; +import static jadx.tests.api.utils.assertj.JadxAssertions.assertThat; public class TestSequentialLoops extends IntegrationTest { @@ -35,13 +30,10 @@ public class TestSequentialLoops extends IntegrationTest { @Test public void test() { - disableCompilation(); - ClassNode cls = getClassNode(TestCls.class); - String code = cls.getCode().toString(); - - assertThat(code, countString(2, "while (")); - assertThat(code, containsOne("break;")); - assertThat(code, containsOne("return c;")); - assertThat(code, not(containsString("else"))); + assertThat(getClassNode(TestCls.class)) + .code() + .countString(2, "while (") + .containsOne("break;") + .containsOne("return c;"); } } diff --git a/jadx-core/src/test/java/jadx/tests/integration/trycatch/TestLoopInTryCatch.java b/jadx-core/src/test/java/jadx/tests/integration/trycatch/TestLoopInTryCatch.java index 8c5a74ad..1568b62f 100644 --- a/jadx-core/src/test/java/jadx/tests/integration/trycatch/TestLoopInTryCatch.java +++ b/jadx-core/src/test/java/jadx/tests/integration/trycatch/TestLoopInTryCatch.java @@ -23,7 +23,8 @@ public class TestLoopInTryCatch extends SmaliTest { " break;", " }", "}", - "if (i == 1) {", + "if (i != 1) {", + " getI();", "}"), c -> c.containsLines(2, "int i;", @@ -37,7 +38,8 @@ public class TestLoopInTryCatch extends SmaliTest { " return;", " }", "}", - "if (i == 1) {", + "if (i != 1) {", + " getI();", "}")); } } diff --git a/jadx-core/src/test/smali/conditions/TestIfCodeStyle.smali b/jadx-core/src/test/smali/conditions/TestIfCodeStyle.smali new file mode 100644 index 00000000..0100c23b --- /dev/null +++ b/jadx-core/src/test/smali/conditions/TestIfCodeStyle.smali @@ -0,0 +1,354 @@ +.class public Lconditions/TestIfCodeStyle; +.super Ljava/lang/Object; +.implements Landroid/os/Parcelable; + + +.field public isActive:Z +.field public isFactory:Z +.field public moduleName:Ljava/lang/String; +.field public modulePath:Ljava/lang/String; +.field public preinstalledModulePath:Ljava/lang/String; +.field public versionCode:J +.field public versionName:Ljava/lang/String; + + +.method public final readFromParcel(Landroid/os/Parcel;)V + .registers 9 + .param p1, "_aidl_parcel" # Landroid/os/Parcel; + + .line 44 + invoke-virtual {p1}, Landroid/os/Parcel;->dataPosition()I + move-result v0 + + .line 45 + .local v0, "_aidl_start_pos":I + invoke-virtual {p1}, Landroid/os/Parcel;->readInt()I + move-result v1 + + .line 47 + .local v1, "_aidl_parcelable_size":I + const-string v2, "Overflow in the size of parcelable" + const v3, 0x7fffffff + if-gez v1, :cond_1e + + .line 63 + sub-int/2addr v3, v1 + + if-gt v0, v3, :cond_18 + + .line 66 + add-int v2, v0, v1 + invoke-virtual {p1, v2}, Landroid/os/Parcel;->setDataPosition(I)V + + .line 47 + return-void + + .line 64 + :cond_18 + new-instance v3, Ljava/lang/RuntimeException; + invoke-direct {v3, v2}, Ljava/lang/RuntimeException;->(Ljava/lang/String;)V + throw v3 + + .line 48 + :cond_1e + :try_start_1e + invoke-virtual {p1}, Landroid/os/Parcel;->dataPosition()I + + move-result v4 + :try_end_22 + .catchall {:try_start_1e .. :try_end_22} :catchall_fd + + sub-int/2addr v4, v0 + + if-lt v4, v1, :cond_34 + + .line 63 + sub-int/2addr v3, v1 + if-gt v0, v3, :cond_2e + + .line 66 + add-int v2, v0, v1 + invoke-virtual {p1, v2}, Landroid/os/Parcel;->setDataPosition(I)V + + .line 48 + return-void + + .line 64 + :cond_2e + new-instance v3, Ljava/lang/RuntimeException; + invoke-direct {v3, v2}, Ljava/lang/RuntimeException;->(Ljava/lang/String;)V + throw v3 + + .line 49 + :cond_34 + :try_start_34 + invoke-virtual {p1}, Landroid/os/Parcel;->readString()Ljava/lang/String; + move-result-object v4 + + iput-object v4, p0, Lconditions/TestIfCodeStyle;->moduleName:Ljava/lang/String; + + .line 50 + invoke-virtual {p1}, Landroid/os/Parcel;->dataPosition()I + move-result v4 + :try_end_3e + .catchall {:try_start_34 .. :try_end_3e} :catchall_fd + + sub-int/2addr v4, v0 + + if-lt v4, v1, :cond_50 + + .line 63 + sub-int/2addr v3, v1 + + if-gt v0, v3, :cond_4a + + .line 66 + add-int v2, v0, v1 + invoke-virtual {p1, v2}, Landroid/os/Parcel;->setDataPosition(I)V + + .line 50 + return-void + + .line 64 + :cond_4a + new-instance v3, Ljava/lang/RuntimeException; + invoke-direct {v3, v2}, Ljava/lang/RuntimeException;->(Ljava/lang/String;)V + throw v3 + + .line 51 + :cond_50 + :try_start_50 + invoke-virtual {p1}, Landroid/os/Parcel;->readString()Ljava/lang/String; + move-result-object v4 + iput-object v4, p0, Lconditions/TestIfCodeStyle;->modulePath:Ljava/lang/String; + + .line 52 + invoke-virtual {p1}, Landroid/os/Parcel;->dataPosition()I + move-result v4 + + :try_end_5a + .catchall {:try_start_50 .. :try_end_5a} :catchall_fd + + sub-int/2addr v4, v0 + if-lt v4, v1, :cond_6c + + .line 63 + sub-int/2addr v3, v1 + if-gt v0, v3, :cond_66 + + .line 66 + add-int v2, v0, v1 + invoke-virtual {p1, v2}, Landroid/os/Parcel;->setDataPosition(I)V + + .line 52 + return-void + + .line 64 + :cond_66 + new-instance v3, Ljava/lang/RuntimeException; + invoke-direct {v3, v2}, Ljava/lang/RuntimeException;->(Ljava/lang/String;)V + throw v3 + + .line 53 + :cond_6c + :try_start_6c + invoke-virtual {p1}, Landroid/os/Parcel;->readString()Ljava/lang/String; + move-result-object v4 + iput-object v4, p0, Lconditions/TestIfCodeStyle;->preinstalledModulePath:Ljava/lang/String; + + .line 54 + invoke-virtual {p1}, Landroid/os/Parcel;->dataPosition()I + move-result v4 + + :try_end_76 + .catchall {:try_start_6c .. :try_end_76} :catchall_fd + + sub-int/2addr v4, v0 + if-lt v4, v1, :cond_88 + + .line 63 + sub-int/2addr v3, v1 + if-gt v0, v3, :cond_82 + + .line 66 + add-int v2, v0, v1 + invoke-virtual {p1, v2}, Landroid/os/Parcel;->setDataPosition(I)V + + .line 54 + return-void + + .line 64 + :cond_82 + new-instance v3, Ljava/lang/RuntimeException; + invoke-direct {v3, v2}, Ljava/lang/RuntimeException;->(Ljava/lang/String;)V + throw v3 + + .line 55 + :cond_88 + :try_start_88 + invoke-virtual {p1}, Landroid/os/Parcel;->readLong()J + move-result-wide v4 + iput-wide v4, p0, Lconditions/TestIfCodeStyle;->versionCode:J + + .line 56 + invoke-virtual {p1}, Landroid/os/Parcel;->dataPosition()I + move-result v4 + + :try_end_92 + .catchall {:try_start_88 .. :try_end_92} :catchall_fd + + sub-int/2addr v4, v0 + if-lt v4, v1, :cond_a4 + + .line 63 + sub-int/2addr v3, v1 + if-gt v0, v3, :cond_9e + + .line 66 + add-int v2, v0, v1 + invoke-virtual {p1, v2}, Landroid/os/Parcel;->setDataPosition(I)V + + .line 56 + return-void + + .line 64 + :cond_9e + new-instance v3, Ljava/lang/RuntimeException; + invoke-direct {v3, v2}, Ljava/lang/RuntimeException;->(Ljava/lang/String;)V + throw v3 + + .line 57 + :cond_a4 + :try_start_a4 + invoke-virtual {p1}, Landroid/os/Parcel;->readString()Ljava/lang/String; + move-result-object v4 + iput-object v4, p0, Lconditions/TestIfCodeStyle;->versionName:Ljava/lang/String; + + .line 58 + invoke-virtual {p1}, Landroid/os/Parcel;->dataPosition()I + move-result v4 + :try_end_ae + .catchall {:try_start_a4 .. :try_end_ae} :catchall_fd + + sub-int/2addr v4, v0 + if-lt v4, v1, :cond_c0 + + .line 63 + sub-int/2addr v3, v1 + if-gt v0, v3, :cond_ba + + .line 66 + add-int v2, v0, v1 + invoke-virtual {p1, v2}, Landroid/os/Parcel;->setDataPosition(I)V + + .line 58 + return-void + + .line 64 + :cond_ba + new-instance v3, Ljava/lang/RuntimeException; + invoke-direct {v3, v2}, Ljava/lang/RuntimeException;->(Ljava/lang/String;)V + throw v3 + + .line 59 + :cond_c0 + :try_start_c0 + invoke-virtual {p1}, Landroid/os/Parcel;->readInt()I + move-result v4 + const/4 v5, 0x1 + const/4 v6, 0x0 + if-eqz v4, :cond_ca + move v4, v5 + goto :goto_cb + + :cond_ca + move v4, v6 + + :goto_cb + iput-boolean v4, p0, Lconditions/TestIfCodeStyle;->isFactory:Z + + .line 60 + invoke-virtual {p1}, Landroid/os/Parcel;->dataPosition()I + + move-result v4 + :try_end_d1 + .catchall {:try_start_c0 .. :try_end_d1} :catchall_fd + + sub-int/2addr v4, v0 + if-lt v4, v1, :cond_e3 + + .line 63 + sub-int/2addr v3, v1 + + if-gt v0, v3, :cond_dd + + .line 66 + add-int v2, v0, v1 + invoke-virtual {p1, v2}, Landroid/os/Parcel;->setDataPosition(I)V + + .line 60 + return-void + + .line 64 + :cond_dd + new-instance v3, Ljava/lang/RuntimeException; + invoke-direct {v3, v2}, Ljava/lang/RuntimeException;->(Ljava/lang/String;)V + throw v3 + + .line 61 + :cond_e3 + :try_start_e3 + invoke-virtual {p1}, Landroid/os/Parcel;->readInt()I + move-result v4 + + if-eqz v4, :cond_ea + goto :goto_eb + + :cond_ea + move v5, v6 + + :goto_eb + iput-boolean v5, p0, Lconditions/TestIfCodeStyle;->isActive:Z + :try_end_ed + .catchall {:try_start_e3 .. :try_end_ed} :catchall_fd + + .line 63 + sub-int/2addr v3, v1 + + if-gt v0, v3, :cond_f7 + + .line 66 + add-int v2, v0, v1 + invoke-virtual {p1, v2}, Landroid/os/Parcel;->setDataPosition(I)V + + .line 67 + nop + + .line 68 + return-void + + .line 64 + :cond_f7 + new-instance v3, Ljava/lang/RuntimeException; + invoke-direct {v3, v2}, Ljava/lang/RuntimeException;->(Ljava/lang/String;)V + throw v3 + + .line 63 + :catchall_fd + move-exception v4 + sub-int/2addr v3, v1 + if-le v0, v3, :cond_107 + + .line 64 + new-instance v3, Ljava/lang/RuntimeException; + invoke-direct {v3, v2}, Ljava/lang/RuntimeException;->(Ljava/lang/String;)V + throw v3 + + .line 66 + :cond_107 + add-int v2, v0, v1 + invoke-virtual {p1, v2}, Landroid/os/Parcel;->setDataPosition(I)V + + .line 67 + throw v4 +.end method diff --git a/jadx-core/src/test/smali/trycatch/TestLoopInTryCatch.smali b/jadx-core/src/test/smali/trycatch/TestLoopInTryCatch.smali index 132155ba..a4a4b2dd 100644 --- a/jadx-core/src/test/smali/trycatch/TestLoopInTryCatch.smali +++ b/jadx-core/src/test/smali/trycatch/TestLoopInTryCatch.smali @@ -24,7 +24,7 @@ :cond if-eq v1, v2, :end - + invoke-static {}, Ltrycatch/TestLoopInTryCatch;->getI()I return-void :try_end diff --git a/jadx-plugins/jadx-dex-input/src/main/java/jadx/plugins/input/dex/sections/debuginfo/DebugInfoParser.java b/jadx-plugins/jadx-dex-input/src/main/java/jadx/plugins/input/dex/sections/debuginfo/DebugInfoParser.java index 3973919b..fada61ab 100644 --- a/jadx-plugins/jadx-dex-input/src/main/java/jadx/plugins/input/dex/sections/debuginfo/DebugInfoParser.java +++ b/jadx-plugins/jadx-dex-input/src/main/java/jadx/plugins/input/dex/sections/debuginfo/DebugInfoParser.java @@ -101,25 +101,21 @@ public class DebugInfoParser { varsInfoFound = true; } } - - // process '0' instruction - addrChange(-1, 1, line); - setLine(addr, line); - - int c = in.readUByte(); - while (c != DBG_END_SEQUENCE) { + while (true) { + int c = in.readUByte(); + if (c == DBG_END_SEQUENCE) { + break; + } switch (c) { case DBG_ADVANCE_PC: { int addrInc = in.readUleb128(); - addr = addrChange(addr, addrInc, line); - setLine(addr, line); + addr = addrChange(addr, addrInc); break; } case DBG_ADVANCE_LINE: { line += in.readSleb128(); break; } - case DBG_START_LOCAL: { int regNum = in.readUleb128(); int nameId = in.readUleb128() - 1; @@ -154,30 +150,26 @@ public class DebugInfoParser { varsInfoFound = true; break; } - case DBG_SET_PROLOGUE_END: - case DBG_SET_EPILOGUE_BEGIN: + case DBG_SET_EPILOGUE_BEGIN: { // do nothing break; - + } case DBG_SET_FILE: { int idx = in.readUleb128() - 1; this.sourceFile = ext.getString(idx); break; } - default: { int adjustedOpCode = c - DBG_FIRST_SPECIAL; int addrInc = adjustedOpCode / DBG_LINE_RANGE; - addr = addrChange(addr, addrInc, line); + addr = addrChange(addr, addrInc); line += DBG_LINE_BASE + adjustedOpCode % DBG_LINE_RANGE; setLine(addr, line); break; } } - c = in.readUByte(); } - if (varsInfoFound) { for (DexLocalVar var : locals) { if (var != null && !var.isEnd()) { @@ -185,14 +177,11 @@ public class DebugInfoParser { } } } - return new DebugInfo(linesMap, resultList); } - private int addrChange(int addr, int addrInc, int line) { - int newAddr = Math.min(addr + addrInc, codeSize - 1); - setLine(newAddr, line); - return newAddr; + private int addrChange(int addr, int addrInc) { + return Math.min(addr + addrInc, codeSize - 1); } private void setLine(int offset, int line) {