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)

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();
+}

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;

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;

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 <code>"text " + 1 + " text"</code>.
+	 *
+	 * @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;

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();"));
+	}
+}