core: fix endless loop processing (#275)

jadx-core/src/main/java/jadx/core/codegen/TypeGen.java

@@ -4,6 +4,7 @@ import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import jadx.api.JadxArgs;
+import jadx.core.deobf.NameMapper;
 import jadx.core.dex.instructions.args.ArgType;
 import jadx.core.dex.instructions.args.PrimitiveType;
 import jadx.core.dex.nodes.IDexNode;
@@ -57,7 +58,11 @@ public class TypeGen {
 			case BOOLEAN:
 				return lit == 0 ? "false" : "true";
 			case CHAR:
-				return stringUtils.unescapeChar((char) lit);
+				char ch = (char) lit;
+				if (!NameMapper.isPrintableChar(ch)) {
+					return Integer.toString(ch);
+				}
+				return stringUtils.unescapeChar(ch);
 			case BYTE:
 				return formatByte((byte) lit);
 			case SHORT:
@@ -171,5 +176,4 @@ public class TypeGen {
 		}
 		return Float.toString(f) + "f";
 	}
-
 }

jadx-core/src/main/java/jadx/core/dex/visitors/regions/ProcessVariables.java

 package jadx.core.dex.visitors.regions;
 
 import java.util.ArrayList;
-import java.util.HashMap;
 import java.util.Iterator;
 import java.util.LinkedHashMap;
 import java.util.LinkedHashSet;
@@ -10,9 +9,6 @@ import java.util.Map;
 import java.util.Map.Entry;
 import java.util.Set;
 
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
-
 import jadx.core.dex.attributes.AFlag;
 import jadx.core.dex.attributes.AType;
 import jadx.core.dex.attributes.nodes.DeclareVariablesAttr;
@@ -21,7 +17,6 @@ import jadx.core.dex.instructions.args.ArgType;
 import jadx.core.dex.instructions.args.RegisterArg;
 import jadx.core.dex.instructions.args.VarName;
 import jadx.core.dex.nodes.IBlock;
-import jadx.core.dex.nodes.IBranchRegion;
 import jadx.core.dex.nodes.IContainer;
 import jadx.core.dex.nodes.IRegion;
 import jadx.core.dex.nodes.InsnNode;
@@ -34,7 +29,6 @@ import jadx.core.utils.RegionUtils;
 import jadx.core.utils.exceptions.JadxException;
 
 public class ProcessVariables extends AbstractVisitor {
-	private static final Logger LOG = LoggerFactory.getLogger(ProcessVariables.class);
 
 	private static class Variable {
 		private final int regNum;
@@ -213,16 +207,13 @@ public class ProcessVariables extends AbstractVisitor {
 			return;
 		}
 
-		Map<IContainer, Integer> regionsOrder = new HashMap<>();
-		calculateOrder(mth.getRegion(), regionsOrder, 0, true);
-
 		for (Iterator<Entry<Variable, Usage>> it = usageMap.entrySet().iterator(); it.hasNext(); ) {
 			Entry<Variable, Usage> entry = it.next();
 			Usage u = entry.getValue();
 			// check if variable can be declared at current assigns
 			for (IRegion assignRegion : u.getAssigns()) {
 				if (u.getArgRegion() == assignRegion
-						&& canDeclareInRegion(u, assignRegion, regionsOrder)
+						&& canDeclareInRegion(u, assignRegion)
 						&& declareAtAssign(u)) {
 					it.remove();
 					break;
@@ -258,7 +249,7 @@ public class ProcessVariables extends AbstractVisitor {
 			IRegion parent = region;
 			boolean declared = false;
 			while (parent != null) {
-				if (canDeclareInRegion(u, region, regionsOrder)) {
+				if (canDeclareInRegion(u, region)) {
 					declareVar(region, u.getArg());
 					declared = true;
 					break;
@@ -311,38 +302,7 @@ public class ProcessVariables extends AbstractVisitor {
 		dv.addVar(arg);
 	}
 
-	private static int calculateOrder(IContainer container, Map<IContainer, Integer> regionsOrder, int id, boolean inc) {
-		if (!(container instanceof IRegion)) {
-			return id;
-		}
-		IRegion region = (IRegion) container;
-		Integer previous = regionsOrder.put(region, id);
-		if (previous != null) {
-			return id;
-		}
-		for (IContainer c : region.getSubBlocks()) {
-			if (c instanceof IBranchRegion) {
-				// on branch set for all inner regions same order id
-				id = calculateOrder(c, regionsOrder, inc ? id + 1 : id, false);
-			} else {
-				List<IContainer> handlers = RegionUtils.getExcHandlersForRegion(c);
-				if (!handlers.isEmpty()) {
-					for (IContainer handler : handlers) {
-						id = calculateOrder(handler, regionsOrder, inc ? id + 1 : id, inc);
-					}
-				}
-				id = calculateOrder(c, regionsOrder, inc ? id + 1 : id, inc);
-			}
-		}
-		return id;
-	}
-
-	private static boolean canDeclareInRegion(Usage u, IRegion region, Map<IContainer, Integer> regionsOrder) {
-		Integer pos = regionsOrder.get(region);
-		if (pos == null) {
-			LOG.debug("TODO: Not found order for region {} for {}", region, u);
-			return false;
-		}
+	private static boolean canDeclareInRegion(Usage u, IRegion region) {
 		// workaround for declare variables used in several loops
 		if (region instanceof LoopRegion) {
 			for (IRegion r : u.getAssigns()) {
@@ -355,32 +315,12 @@ public class ProcessVariables extends AbstractVisitor {
 		if (region.contains(AFlag.ELSE_IF_CHAIN)) {
 			return false;
 		}
-		return isAllRegionsAfter(region, pos, u.getAssigns(), regionsOrder)
-				&& isAllRegionsAfter(region, pos, u.getUseRegions(), regionsOrder);
-	}
-
-	private static boolean isAllRegionsAfter(IRegion region, int pos,
-	                                         Set<IRegion> regions, Map<IContainer, Integer> regionsOrder) {
-		for (IRegion r : regions) {
-			if (r == region) {
-				continue;
-			}
-			Integer rPos = regionsOrder.get(r);
-			if (rPos == null) {
-				LOG.debug("TODO: Not found order for region {} in {}", r, regionsOrder);
-				return false;
-			}
-			if (pos > rPos) {
-				return false;
-			}
-			if (pos == rPos) {
-				return isAllRegionsAfterRecursive(region, regions);
-			}
-		}
-		return true;
+		// TODO: make index for faster search
+		return isAllRegionsAfter(region, u.getAssigns())
+				&& isAllRegionsAfter(region, u.getUseRegions());
 	}
 
-	private static boolean isAllRegionsAfterRecursive(IRegion region, Set<IRegion> others) {
+	private static boolean isAllRegionsAfter(IRegion region, Set<IRegion> others) {
 		for (IRegion r : others) {
 			if (!RegionUtils.isRegionContainsRegion(region, r)) {
 				return false;

jadx-core/src/main/java/jadx/core/dex/visitors/regions/RegionMaker.java

@@ -192,10 +192,9 @@ public class RegionMaker {
 			if (loopExit != null) {
 				// add 'break' instruction before path cross between main loop exit and sub-exit
 				for (Edge exitEdge : loop.getExitEdges()) {
-					if (!exitBlocks.contains(exitEdge.getSource())) {
-						continue;
+					if (exitBlocks.contains(exitEdge.getSource())) {
+						insertBreak(stack, loopExit, exitEdge);
 					}
-					insertBreak(stack, loopExit, exitEdge);
 				}
 			}
 		}
@@ -207,7 +206,8 @@ public class RegionMaker {
 			loopStart.remove(AType.LOOP);
 			loop.getEnd().add(AFlag.SKIP);
 			stack.addExit(loop.getEnd());
-			loopRegion.setBody(makeRegion(loopStart, stack));
+			Region body = makeRegion(loopStart, stack);
+			loopRegion.setBody(body);
 			loopStart.addAttr(AType.LOOP, loop);
 			loop.getEnd().remove(AFlag.SKIP);
 		} else {
@@ -303,16 +303,31 @@ public class RegionMaker {
 		loopStart.remove(AType.LOOP);
 		stack.push(loopRegion);
 
-		BlockNode loopExit = null;
+		BlockNode out = null;
 		// insert 'break' for exits
 		List<Edge> exitEdges = loop.getExitEdges();
-		for (Edge exitEdge : exitEdges) {
+		if (exitEdges.size() == 1) {
+			Edge exitEdge = exitEdges.get(0);
 			BlockNode exit = exitEdge.getTarget();
 			if (insertBreak(stack, exit, exitEdge)) {
 				BlockNode nextBlock = getNextBlock(exit);
 				if (nextBlock != null) {
 					stack.addExit(nextBlock);
-					loopExit = nextBlock;
+					out = nextBlock;
+				}
+			}
+		} else {
+			for (Edge exitEdge : exitEdges) {
+				BlockNode exit = exitEdge.getTarget();
+				List<BlockNode> blocks = BlockUtils.bitSetToBlocks(mth, exit.getDomFrontier());
+				for (BlockNode block : blocks) {
+					if (BlockUtils.isPathExists(exit, block)) {
+						stack.addExit(block);
+						insertBreak(stack, block, exitEdge);
+						out = block;
+					} else {
+						insertBreak(stack, exit, exitEdge);
+					}
 				}
 			}
 		}
@@ -326,13 +341,13 @@ public class RegionMaker {
 		}
 		loopRegion.setBody(body);
 
-		if (loopExit == null) {
+		if (out == null) {
 			BlockNode next = getNextBlock(loopEnd);
-			loopExit = RegionUtils.isRegionContainsBlock(body, next) ? null : next;
+			out = RegionUtils.isRegionContainsBlock(body, next) ? null : next;
 		}
 		stack.pop();
 		loopStart.addAttr(AType.LOOP, loop);
-		return loopExit;
+		return out;
 	}
 
 	private boolean inExceptionHandlerBlocks(BlockNode loopEnd) {
@@ -463,7 +478,7 @@ public class RegionMaker {
 	}
 
 	private static boolean canInsertContinue(BlockNode pred, List<BlockNode> predecessors, BlockNode loopEnd,
-			Set<BlockNode> loopExitNodes) {
+	                                         Set<BlockNode> loopExitNodes) {
 		if (!pred.contains(AFlag.SYNTHETIC)
 				|| BlockUtils.checkLastInsnType(pred, InsnType.CONTINUE)) {
 			return false;
@@ -553,8 +568,8 @@ public class RegionMaker {
 	/**
 	 * Traverse from monitor-enter thru successors and collect blocks contains monitor-exit
 	 */
-	private static void traverseMonitorExits(SynchronizedRegion region, InsnArg arg, BlockNode block,
-			Set<BlockNode> exits, Set<BlockNode> visited) {
+	private static void traverseMonitorExits(SynchronizedRegion region, InsnArg arg, BlockNode block, Set<BlockNode> exits,
+	                                         Set<BlockNode> visited) {
 		visited.add(block);
 		for (InsnNode insn : block.getInstructions()) {
 			if (insn.getType() == InsnType.MONITOR_EXIT
@@ -828,7 +843,7 @@ public class RegionMaker {
 	}
 
 	private boolean isBadCasesOrder(Map<BlockNode, List<Object>> blocksMap,
-			Map<BlockNode, BlockNode> fallThroughCases) {
+	                                Map<BlockNode, BlockNode> fallThroughCases) {
 		BlockNode nextCaseBlock = null;
 		for (BlockNode caseBlock : blocksMap.keySet()) {
 			if (nextCaseBlock != null && !caseBlock.equals(nextCaseBlock)) {

jadx-core/src/main/java/jadx/core/utils/DebugUtils.java

@@ -99,7 +99,7 @@ public class DebugUtils {
 				CodeWriter code = new CodeWriter();
 				ig.makeInsn(insn, code);
 				String insnStr = code.toString().substring(CodeWriter.NL.length());
-				LOG.debug("{}> {}", indent, insnStr);
+				LOG.debug("{}> {}\t{}", indent, insnStr, insn.getAttributesString());
 			} catch (CodegenException e) {
 				LOG.debug("{}>!! {}", indent, insn);
 			}

jadx-core/src/test/java/jadx/tests/integration/loops/TestBreakInLoop2.java

@@ -9,6 +9,7 @@ import jadx.tests.api.IntegrationTest;
 
 import static jadx.tests.api.utils.JadxMatchers.containsOne;
 import static org.junit.Assert.assertThat;
+import static org.hamcrest.Matchers.anyOf;
 
 public class TestBreakInLoop2 extends IntegrationTest {
 
@@ -43,7 +44,7 @@ public class TestBreakInLoop2 extends IntegrationTest {
 		String code = cls.getCode().toString();
 
 		assertThat(code, containsOne("while (true) {"));
-		assertThat(code, containsOne("break;"));
+		assertThat(code, anyOf(containsOne("break;"), containsOne("return;")));
 		assertThat(code, containsOne("throw ex;"));
 		assertThat(code, containsOne("data.clear();"));
 		assertThat(code, containsOne("Thread.sleep(100);"));

jadx-core/src/test/java/jadx/tests/integration/loops/TestLoopConditionInvoke.java

@@ -37,7 +37,7 @@ public class TestLoopConditionInvoke extends IntegrationTest {
 		String code = cls.getCode().toString();
 
 		assertThat(code, containsOne("do {"));
-		assertThat(code, containsOne("if (ch == '\\u0000') {"));
+		assertThat(code, containsOne("if (ch == 0) {"));
 		assertThat(code, containsOne("this.pos = startPos;"));
 		assertThat(code, containsOne("return false;"));
 		assertThat(code, containsOne("} while (ch != lastChar);"));

jadx-core/src/test/java/jadx/tests/integration/loops/TestSequentialLoops.java

@@ -14,7 +14,7 @@ import static org.junit.Assert.assertThat;
 public class TestSequentialLoops extends IntegrationTest {
 
 	public static class TestCls {
-		public int test7(int a, int b) {
+		public int test(int a, int b) {
 			int c = b;
 			int z;
 

jadx-core/src/test/java/jadx/tests/integration/loops/TestSequentialLoops2.java

+package jadx.tests.integration.loops;
+
+import org.junit.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.junit.Assert.assertThat;
+
+public class TestSequentialLoops2 extends IntegrationTest {
+
+	public static class TestCls {
+		private static char[] lowercases = new char[]{'a'};
+
+		public static String asciiToLowerCase(String s) {
+			char[] c = null;
+			int i = s.length();
+			while (i-- > 0) {
+				char c1 = s.charAt(i);
+				if (c1 <= 127) {
+					char c2 = lowercases[c1];
+					if (c1 != c2) {
+						c = s.toCharArray();
+						c[i] = c2;
+						break;
+					}
+				}
+			}
+			while (i-- > 0) {
+				if (c[i] <= 127) {
+					c[i] = lowercases[c[i]];
+				}
+			}
+			return c == null ? s : new String(c);
+		}
+	}
+
+	@Test
+	public void test() {
+		ClassNode cls = getClassNode(TestCls.class);
+		String code = cls.getCode().toString();
+
+		assertThat(code, countString(2, "while ("));
+		assertThat(code, containsString("break;"));
+		assertThat(code, containsOne("return c"));
+		assertThat(code, countString(2, "<= 127"));
+	}
+}