From 009939f8662354ad5fae88015f6d30f55bafa66f Mon Sep 17 00:00:00 2001
From: Skylot <skylot@gmail.com>
Date: Thu, 5 Nov 2020 16:56:23 +0000
Subject: [PATCH] fix: prevent endless loop in method signature parsing (#1007)

Signed-off-by: Skylot <skylot@gmail.com>
---
 .../dex/nodes/parser/SignatureParser.java     | 25 ++++++++++++++-----
 .../core/dex/visitors/SignatureProcessor.java |  2 +-
 .../tests/functional/SignatureParserTest.java | 16 +++++++++---
 3 files changed, 32 insertions(+), 11 deletions(-)

diff --git a/jadx-core/src/main/java/jadx/core/dex/nodes/parser/SignatureParser.java b/jadx-core/src/main/java/jadx/core/dex/nodes/parser/SignatureParser.java
index 60ff9e67..c95f07cc 100644
--- a/jadx-core/src/main/java/jadx/core/dex/nodes/parser/SignatureParser.java
+++ b/jadx-core/src/main/java/jadx/core/dex/nodes/parser/SignatureParser.java
@@ -151,6 +151,9 @@ public class SignatureParser {
 				String typeVarName = consumeUntil(';');
 				if (typeVarName != null) {
 					consume(';');
+					if (typeVarName.contains(")")) {
+						throw new JadxRuntimeException("Bad name for type variable: " + typeVarName);
+					}
 					return ArgType.genericType(typeVarName);
 				}
 				break;
@@ -268,8 +271,7 @@ public class SignatureParser {
 			}
 			String id = consumeUntil(':');
 			if (id == null) {
-				LOG.error("Failed to parse generic types map: {}", sign);
-				return Collections.emptyList();
+				throw new JadxRuntimeException("Failed to parse generic types map");
 			}
 			consume(':');
 			tryConsume(':');
@@ -290,9 +292,12 @@ public class SignatureParser {
 		boolean next;
 		do {
 			ArgType argType = consumeType();
+			if (argType == null) {
+				throw new JadxRuntimeException("Unexpected end of signature");
+			}
 			if (!argType.equals(ArgType.OBJECT)) {
 				if (types.isEmpty()) {
-					types = new LinkedList<>();
+					types = new ArrayList<>();
 				}
 				types.add(argType);
 			}
@@ -304,15 +309,23 @@ public class SignatureParser {
 		return types;
 	}
 
-	public List<ArgType> consumeMethodArgs() {
+	public List<ArgType> consumeMethodArgs(int argsCount) {
 		consume('(');
 		if (lookAhead(')')) {
 			consume(')');
 			return Collections.emptyList();
 		}
-		List<ArgType> args = new LinkedList<>();
+		List<ArgType> args = new ArrayList<>(argsCount);
+		int limit = argsCount + 10; // just prevent endless loop, args count can be different for synthetic methods
 		do {
-			args.add(consumeType());
+			ArgType type = consumeType();
+			if (type == null) {
+				throw new JadxRuntimeException("Unexpected end of signature");
+			}
+			args.add(type);
+			if (args.size() > limit) {
+				throw new JadxRuntimeException("Arguments count limit reached: " + args.size());
+			}
 		} while (!lookAhead(')'));
 		consume(')');
 		return args;
diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/SignatureProcessor.java b/jadx-core/src/main/java/jadx/core/dex/visitors/SignatureProcessor.java
index ca4ab5c2..5aae3044 100644
--- a/jadx-core/src/main/java/jadx/core/dex/visitors/SignatureProcessor.java
+++ b/jadx-core/src/main/java/jadx/core/dex/visitors/SignatureProcessor.java
@@ -103,7 +103,7 @@ public class SignatureProcessor extends AbstractVisitor {
 		}
 		try {
 			List<ArgType> typeParameters = sp.consumeGenericTypeParameters();
-			List<ArgType> parsedArgTypes = sp.consumeMethodArgs();
+			List<ArgType> parsedArgTypes = sp.consumeMethodArgs(mth.getMethodInfo().getArgsCount());
 			ArgType parsedRetType = sp.consumeType();
 
 			if (!validateParsedType(parsedRetType, mth.getMethodInfo().getReturnType())) {
diff --git a/jadx-core/src/test/java/jadx/tests/functional/SignatureParserTest.java b/jadx-core/src/test/java/jadx/tests/functional/SignatureParserTest.java
index 36b281fd..d8317dea 100644
--- a/jadx-core/src/test/java/jadx/tests/functional/SignatureParserTest.java
+++ b/jadx-core/src/test/java/jadx/tests/functional/SignatureParserTest.java
@@ -8,6 +8,7 @@ import org.junit.jupiter.api.Test;
 import jadx.core.dex.instructions.args.ArgType;
 import jadx.core.dex.instructions.args.ArgType.WildcardBound;
 import jadx.core.dex.nodes.parser.SignatureParser;
+import jadx.core.utils.exceptions.JadxRuntimeException;
 
 import static jadx.core.dex.instructions.args.ArgType.INT;
 import static jadx.core.dex.instructions.args.ArgType.OBJECT;
@@ -19,6 +20,7 @@ import static jadx.core.dex.instructions.args.ArgType.outerGeneric;
 import static jadx.core.dex.instructions.args.ArgType.wildcard;
 import static java.util.Collections.emptyList;
 import static java.util.Collections.singletonList;
+import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.hasSize;
@@ -114,7 +116,7 @@ class SignatureParserTest {
 
 	@Test
 	public void testMethodArgs() {
-		List<ArgType> argTypes = new SignatureParser("(Ljava/util/List<*>;)V").consumeMethodArgs();
+		List<ArgType> argTypes = new SignatureParser("(Ljava/util/List<*>;)V").consumeMethodArgs(1);
 
 		assertThat(argTypes, hasSize(1));
 		assertThat(argTypes.get(0), is(generic("Ljava/util/List;", wildcard())));
@@ -122,7 +124,7 @@ class SignatureParserTest {
 
 	@Test
 	public void testMethodArgs2() {
-		List<ArgType> argTypes = new SignatureParser("(La/b/C<TT;>.d/E;)V").consumeMethodArgs();
+		List<ArgType> argTypes = new SignatureParser("(La/b/C<TT;>.d/E;)V").consumeMethodArgs(1);
 
 		assertThat(argTypes, hasSize(1));
 		ArgType argType = argTypes.get(0);
@@ -132,7 +134,13 @@ class SignatureParserTest {
 
 	@Test
 	public void testBadGenericMap() {
-		List<ArgType> list = new SignatureParser("<A:Ljava/lang/Object;B").consumeGenericTypeParameters();
-		assertThat(list, hasSize(0));
+		assertThatExceptionOfType(JadxRuntimeException.class)
+				.isThrownBy(() -> new SignatureParser("<A:Ljava/lang/Object;B").consumeGenericTypeParameters());
+	}
+
+	@Test
+	public void testBadArgs() {
+		assertThatExceptionOfType(JadxRuntimeException.class)
+				.isThrownBy(() -> new SignatureParser("(TCONTENT)Lpkg/Cls;").consumeMethodArgs(1));
 	}
 }
-- 
GitLab