From 72b2663949555b03aeb864775d84f163b91bcea7 Mon Sep 17 00:00:00 2001 From: Jan S Date: Sat, 12 Jan 2019 19:12:28 +0100 Subject: [PATCH] fix: ArrayIndexOutOfBoundsException in string concatenation visitor (#427) * fix: ArrayIndexOutOfBoundsException in string concatenation visitor * fix: typo in comment * fix: StringBuilder chain processing created wrong code * test: simple JUnit test cases added for testing StringBuilder chain processing (chains that can be and that can't be simplified) --- .../dex/instructions/CallMthInterface.java | 8 ++ .../core/dex/instructions/InvokeNode.java | 2 +- .../instructions/mods/ConstructorInsn.java | 3 +- .../core/dex/visitors/SimplifyVisitor.java | 27 ++++-- .../SimplifyVisitorStringBuilderTest.java | 94 +++++++++++++++++++ 5 files changed, 124 insertions(+), 10 deletions(-) create mode 100644 jadx-core/src/main/java/jadx/core/dex/instructions/CallMthInterface.java create mode 100644 jadx-core/src/test/java/jadx/tests/integration/SimplifyVisitorStringBuilderTest.java diff --git a/jadx-core/src/main/java/jadx/core/dex/instructions/CallMthInterface.java b/jadx-core/src/main/java/jadx/core/dex/instructions/CallMthInterface.java new file mode 100644 index 000000000..f5b8a84ee --- /dev/null +++ b/jadx-core/src/main/java/jadx/core/dex/instructions/CallMthInterface.java @@ -0,0 +1,8 @@ +package jadx.core.dex.instructions; + +import jadx.core.dex.info.MethodInfo; + +public interface CallMthInterface { + + public MethodInfo getCallMth(); +} diff --git a/jadx-core/src/main/java/jadx/core/dex/instructions/InvokeNode.java b/jadx-core/src/main/java/jadx/core/dex/instructions/InvokeNode.java index 5baa632cd..217fa08e5 100644 --- a/jadx-core/src/main/java/jadx/core/dex/instructions/InvokeNode.java +++ b/jadx-core/src/main/java/jadx/core/dex/instructions/InvokeNode.java @@ -9,7 +9,7 @@ import jadx.core.dex.nodes.InsnNode; import jadx.core.utils.InsnUtils; import jadx.core.utils.Utils; -public class InvokeNode extends InsnNode { +public class InvokeNode extends InsnNode implements CallMthInterface { private final InvokeType type; private final MethodInfo mth; diff --git a/jadx-core/src/main/java/jadx/core/dex/instructions/mods/ConstructorInsn.java b/jadx-core/src/main/java/jadx/core/dex/instructions/mods/ConstructorInsn.java index b4451df7c..43d4a7625 100644 --- a/jadx-core/src/main/java/jadx/core/dex/instructions/mods/ConstructorInsn.java +++ b/jadx-core/src/main/java/jadx/core/dex/instructions/mods/ConstructorInsn.java @@ -2,13 +2,14 @@ package jadx.core.dex.instructions.mods; import jadx.core.dex.info.ClassInfo; import jadx.core.dex.info.MethodInfo; +import jadx.core.dex.instructions.CallMthInterface; import jadx.core.dex.instructions.InsnType; import jadx.core.dex.instructions.InvokeNode; import jadx.core.dex.instructions.args.RegisterArg; import jadx.core.dex.nodes.InsnNode; import jadx.core.dex.nodes.MethodNode; -public class ConstructorInsn extends InsnNode { +public class ConstructorInsn extends InsnNode implements CallMthInterface { private final MethodInfo callMth; private final CallType callType; diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/SimplifyVisitor.java b/jadx-core/src/main/java/jadx/core/dex/visitors/SimplifyVisitor.java index 49ab39228..a7109faff 100644 --- a/jadx-core/src/main/java/jadx/core/dex/visitors/SimplifyVisitor.java +++ b/jadx-core/src/main/java/jadx/core/dex/visitors/SimplifyVisitor.java @@ -4,19 +4,13 @@ import java.util.ArrayList; import java.util.Collections; import java.util.List; +import jadx.core.dex.instructions.*; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import jadx.core.Consts; import jadx.core.dex.info.FieldInfo; import jadx.core.dex.info.MethodInfo; -import jadx.core.dex.instructions.ArithNode; -import jadx.core.dex.instructions.ArithOp; -import jadx.core.dex.instructions.ConstStringNode; -import jadx.core.dex.instructions.IfNode; -import jadx.core.dex.instructions.IndexInsnNode; -import jadx.core.dex.instructions.InsnType; -import jadx.core.dex.instructions.InvokeNode; import jadx.core.dex.instructions.args.ArgType; import jadx.core.dex.instructions.args.FieldArg; import jadx.core.dex.instructions.args.InsnArg; @@ -148,6 +142,15 @@ public class SimplifyVisitor extends AbstractVisitor { } } + /** + * Simplify chains of calls to StringBuilder#append() plus constructor of StringBuilder. + * Those chains are usually automatically generated by the Java compiler when you create String + * concatenations like "text " + 1 + " text". + * + * @param mth + * @param insn + * @return + */ private static InsnNode convertInvoke(MethodNode mth, InsnNode insn) { MethodInfo callMth = ((InvokeNode) insn).getCallMth(); @@ -201,7 +204,15 @@ public class SimplifyVisitor extends AbstractVisitor { } for (; argInd < len; argInd++) { // Add the .append(xxx) arg string to concat - concatInsn.addArg(chain.get(argInd).getArg(1)); + InsnNode node = chain.get(argInd); + MethodInfo method = ((CallMthInterface) node).getCallMth(); + if (!(node.getArgsCount() < 2 && method.isConstructor() || method.getName().equals("append"))) { + // The chain contains other calls to StringBuilder methods than the constructor or append. + // We can't simplify such chains, therefore we leave them as they are. + return null; + } + // process only constructor and append() calls + concatInsn.addArg(node.getArg(1)); } concatInsn.setResult(insn.getResult()); return concatInsn; diff --git a/jadx-core/src/test/java/jadx/tests/integration/SimplifyVisitorStringBuilderTest.java b/jadx-core/src/test/java/jadx/tests/integration/SimplifyVisitorStringBuilderTest.java new file mode 100644 index 000000000..5baf911d4 --- /dev/null +++ b/jadx-core/src/test/java/jadx/tests/integration/SimplifyVisitorStringBuilderTest.java @@ -0,0 +1,94 @@ +package jadx.tests.integration; + +import jadx.core.dex.nodes.ClassNode; +import jadx.core.dex.visitors.SimplifyVisitor; +import jadx.core.utils.exceptions.JadxException; +import jadx.tests.api.IntegrationTest; +import org.junit.Test; + +import static org.hamcrest.CoreMatchers.containsString; +import static org.junit.Assert.assertThat; + +/** + * Test the StringBuilder simplification part of {@link SimplifyVisitor} + * + * @author Jan Peter Stotz + */ +public class SimplifyVisitorStringBuilderTest extends IntegrationTest { + + public static class TestCls1 { + public String test() { + return new StringBuilder("[init]").append("a1").append('c').append(2).append(0l).append(1.0f). + append(2.0d).append(true).toString(); + } + } + + @Test + public void test1() throws JadxException { + ClassNode cls = getClassNode(SimplifyVisitorStringBuilderTest.TestCls1.class); + SimplifyVisitor visitor = new SimplifyVisitor(); + visitor.visit(cls); + String code = cls.getCode().toString(); + assertThat(code, containsString("return \"[init]\" + \"a1\" + 'c' + 2 + 0 + 1.0f + 2.0d + true;")); + } + + public static class TestCls2 { + public String test() { + // A chain with non-final variables + String sInit = "[init]"; + String s = "a1"; + char c = 'c'; + int i = 1; + long l = 2; + float f = 1.0f; + double d = 2.0d; + boolean b = true; + return new StringBuilder(sInit).append(s).append(c).append(i).append(l).append(f). + append(d).append(b).toString(); + } + } + + @Test + public void test2() throws JadxException { + ClassNode cls = getClassNode(SimplifyVisitorStringBuilderTest.TestCls2.class); + SimplifyVisitor visitor = new SimplifyVisitor(); + visitor.visit(cls); + String code = cls.getCode().toString(); + assertThat(code, containsString("return \"[init]\" + \"a1\" + 'c' + 1 + 2 + 1.0f + 2.0d + true;")); + } + + public static class TestClsStringUtilsReverse { + + /** + * Simplified version of org.apache.commons.lang3.StringUtils.reverse() + */ + public static String reverse(final String str) { + return new StringBuilder(str).reverse().toString(); + } + } + + @Test + public void test3() throws JadxException { + ClassNode cls = getClassNode(SimplifyVisitorStringBuilderTest.TestClsStringUtilsReverse.class); + SimplifyVisitor visitor = new SimplifyVisitor(); + visitor.visit(cls); + String code = cls.getCode().toString(); + assertThat(code, containsString("return new StringBuilder(str).reverse().toString();")); + } + + public static class TestClsChainWithDelete { + public String test() { + // a chain we can't simplify + return new StringBuilder("[init]").append("a1").delete(1, 2).toString(); + } + } + + @Test + public void testChainWithDelete() throws JadxException { + ClassNode cls = getClassNode(TestClsChainWithDelete.class); + SimplifyVisitor visitor = new SimplifyVisitor(); + visitor.visit(cls); + String code = cls.getCode().toString(); + assertThat(code, containsString("return new StringBuilder(\"[init]\").append(\"a1\").delete(1, 2).toString();")); + } +}