core: fix try/catch wrap logic (fix #47)

jadx-core/src/main/java/jadx/core/dex/regions/TryCatchRegion.java

@@ -84,8 +84,14 @@ public final class TryCatchRegion extends AbstractRegion implements IBranchRegio
 
 	@Override
 	public String toString() {
-		return "Try: " + tryRegion
-				+ " catches: " + Utils.listToString(catchRegions.values())
-				+ (finallyRegion == null ? "" : " finally: " + finallyRegion);
+		StringBuilder sb = new StringBuilder();
+		sb.append("Try: ").append(tryRegion);
+		if (!catchRegions.isEmpty()) {
+			sb.append(" catches: ").append(Utils.listToString(catchRegions.values()));
+		}
+		if (finallyRegion != null) {
+			sb.append(" finally: ").append(finallyRegion);
+		}
+		return sb.toString();
 	}
 }

jadx-core/src/main/java/jadx/core/dex/visitors/blocksmaker/BlockExceptionHandler.java

@@ -65,7 +65,9 @@ public class BlockExceptionHandler extends AbstractVisitor {
 
 	private static void processExceptionHandlers(MethodNode mth, BlockNode block) {
 		ExcHandlerAttr handlerAttr = block.get(AType.EXC_HANDLER);
-		if (handlerAttr != null) {
+		if (handlerAttr == null) {
+			return;
+		}
 		ExceptionHandler excHandler = handlerAttr.getHandler();
 		excHandler.addBlock(block);
 		for (BlockNode node : BlockUtils.collectBlocksDominatedBy(block, block)) {
@@ -99,7 +101,6 @@ public class BlockExceptionHandler extends AbstractVisitor {
 			}
 		}
 	}
-	}
 
 	private static boolean onlyAllHandler(TryCatchBlock tryBlock) {
 		if (tryBlock.getHandlersCount() == 1) {

jadx-core/src/main/java/jadx/core/dex/visitors/blocksmaker/BlockFinish.java

 package jadx.core.dex.visitors.blocksmaker;
 
+import jadx.core.dex.attributes.AType;
 import jadx.core.dex.instructions.IfNode;
 import jadx.core.dex.instructions.InsnType;
 import jadx.core.dex.nodes.BlockNode;
 import jadx.core.dex.nodes.InsnNode;
 import jadx.core.dex.nodes.MethodNode;
+import jadx.core.dex.trycatch.ExcHandlerAttr;
+import jadx.core.dex.trycatch.SplitterBlockAttr;
 import jadx.core.dex.visitors.AbstractVisitor;
+import jadx.core.utils.BlockUtils;
 
+import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 public class BlockFinish extends AbstractVisitor {
 
+	private static final Logger LOG = LoggerFactory.getLogger(BlockFinish.class);
+
 	@Override
 	public void visit(MethodNode mth) {
 		if (mth.isNoCode()) {
@@ -20,6 +31,7 @@ public class BlockFinish extends AbstractVisitor {
 		for (BlockNode block : mth.getBasicBlocks()) {
 			block.updateCleanSuccessors();
 			initBlocksInIfNodes(block);
+			fixSplitterBlock(block);
 		}
 
 		mth.finishBasicBlocks();
@@ -37,4 +49,47 @@ public class BlockFinish extends AbstractVisitor {
 			}
 		}
 	}
+
+	/**
+	 * For evey exception handler must be only one splitter block,
+	 * select correct one and remove others if necessary.
+	 */
+	private static void fixSplitterBlock(BlockNode block) {
+		ExcHandlerAttr excHandlerAttr = block.get(AType.EXC_HANDLER);
+		if (excHandlerAttr == null) {
+			return;
+		}
+		BlockNode handlerBlock = excHandlerAttr.getHandler().getHandlerBlock();
+		if (handlerBlock.getPredecessors().size() < 2) {
+			return;
+		}
+		Map<BlockNode, SplitterBlockAttr> splitters = new HashMap<BlockNode, SplitterBlockAttr>();
+		for (BlockNode pred : handlerBlock.getPredecessors()) {
+			pred = BlockUtils.skipSyntheticPredecessor(pred);
+			SplitterBlockAttr splitterAttr = pred.get(AType.SPLITTER_BLOCK);
+			if (splitterAttr != null && pred == splitterAttr.getBlock()) {
+				splitters.put(pred, splitterAttr);
+			}
+		}
+		if (splitters.size() < 2) {
+			return;
+		}
+		BlockNode topSplitter = BlockUtils.getTopBlock(splitters.keySet());
+		if (topSplitter == null) {
+			LOG.warn("Unknown top splitter block from list: {}", splitters);
+			return;
+		}
+		for (Map.Entry<BlockNode, SplitterBlockAttr> entry : splitters.entrySet()) {
+			BlockNode pred = entry.getKey();
+			SplitterBlockAttr splitterAttr = entry.getValue();
+			if (pred == topSplitter) {
+				block.addAttr(splitterAttr);
+			} else {
+				pred.remove(AType.SPLITTER_BLOCK);
+				for (BlockNode s : pred.getCleanSuccessors()) {
+					s.remove(AType.SPLITTER_BLOCK);
+				}
+			}
+		}
+	}
 }

jadx-core/src/main/java/jadx/core/dex/visitors/blocksmaker/BlockSplitter.java

@@ -151,19 +151,26 @@ public class BlockSplitter extends AbstractVisitor {
 					BlockNode thisBlock = getBlock(jump.getDest(), blocksMap);
 					connect(srcBlock, thisBlock);
 				}
+				connectExceptionHandlers(blocksMap, block, insn);
+			}
+		}
+	}
 
-				// connect exception handlers
+	private static void connectExceptionHandlers(Map<Integer, BlockNode> blocksMap, BlockNode block, InsnNode insn) {
 		CatchAttr catches = insn.get(AType.CATCH_BLOCK);
-				// get synthetic block for handlers
 		SplitterBlockAttr spl = block.get(AType.SPLITTER_BLOCK);
-				if (catches != null && spl != null) {
+		if (catches == null || spl == null) {
+			return;
+		}
 		BlockNode splitterBlock = spl.getBlock();
 		boolean tryEnd = insn.contains(AFlag.TRY_LEAVE);
 		for (ExceptionHandler h : catches.getTryBlock().getHandlers()) {
 			BlockNode handlerBlock = getBlock(h.getHandleOffset(), blocksMap);
 			// skip self loop in handler
 			if (splitterBlock != handlerBlock) {
+				if (!handlerBlock.contains(AType.SPLITTER_BLOCK)) {
 					handlerBlock.addAttr(spl);
+				}
 				connect(splitterBlock, handlerBlock);
 			}
 			if (tryEnd) {
@@ -171,9 +178,6 @@ public class BlockSplitter extends AbstractVisitor {
 			}
 		}
 	}
-			}
-		}
-	}
 
 	private static boolean isSplitByJump(InsnNode prevInsn, InsnNode currentInsn) {
 		List<JumpInfo> pJumps = prevInsn.getAll(AType.JUMP);

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

@@ -12,6 +12,7 @@ import jadx.core.dex.regions.TryCatchRegion;
 import jadx.core.dex.regions.loops.LoopRegion;
 import jadx.core.dex.trycatch.CatchAttr;
 import jadx.core.dex.trycatch.ExceptionHandler;
+import jadx.core.dex.trycatch.SplitterBlockAttr;
 import jadx.core.dex.trycatch.TryCatchBlock;
 import jadx.core.utils.BlockUtils;
 import jadx.core.utils.ErrorsCounter;
@@ -65,39 +66,28 @@ public class ProcessTryCatchRegions extends AbstractRegionVisitor {
 
 		// for each try block search nearest dominator block
 		for (TryCatchBlock tb : tryBlocks) {
-			BitSet bs = null;
-			// build bitset with dominators of blocks covered with this try/catch block
-			for (BlockNode block : mth.getBasicBlocks()) {
-				CatchAttr c = block.get(AType.CATCH_BLOCK);
-				if (c != null && c.getTryBlock() == tb) {
-					if (bs == null) {
-						bs = (BitSet) block.getDoms().clone();
-					} else {
-						bs.and(block.getDoms());
+			BitSet bs = new BitSet(mth.getBasicBlocks().size());
+			for (ExceptionHandler excHandler : tb.getHandlers()) {
+				SplitterBlockAttr splitter = excHandler.getHandlerBlock().get(AType.SPLITTER_BLOCK);
+				if (splitter != null) {
+					BlockNode block = splitter.getBlock();
+					bs.set(block.getId());
 				}
 			}
-			}
-			if (bs == null) {
-				LOG.debug(" Can't build try/catch dominators bitset, tb: {}, mth: {} ", tb, mth);
-				continue;
-			}
-
-			// intersect to get dominator of dominators
 			List<BlockNode> domBlocks = BlockUtils.bitSetToBlocks(mth, bs);
-			for (BlockNode block : domBlocks) {
-				bs.andNot(block.getDoms());
-			}
-			domBlocks = BlockUtils.bitSetToBlocks(mth, bs);
+			BlockNode domBlock;
 			if (domBlocks.size() != 1) {
+				domBlock = BlockUtils.getTopBlock(domBlocks);
+				if (domBlock == null) {
 					throw new JadxRuntimeException(
-						"Exception block dominator not found, method:" + mth + ". bs: " + bs);
+							"Exception block dominator not found, method:" + mth + ". bs: " + domBlocks);
+				}
+			} else {
+				domBlock = domBlocks.get(0);
 			}
-
-			BlockNode domBlock = domBlocks.get(0);
-
 			TryCatchBlock prevTB = tryBlocksMap.put(domBlock, tb);
 			if (prevTB != null) {
-				LOG.info("!!! TODO: merge try blocks in {}", mth);
+				ErrorsCounter.methodError(mth, "Failed to process nested try/catch");
 			}
 		}
 	}
@@ -136,7 +126,7 @@ public class ProcessTryCatchRegions extends AbstractRegionVisitor {
 		Region tryRegion = new Region(replaceRegion);
 		List<IContainer> subBlocks = replaceRegion.getSubBlocks();
 		for (IContainer cont : subBlocks) {
-			if (RegionUtils.isDominatedBy(dominator, cont)) {
+			if (RegionUtils.hasPathThroughBlock(dominator, cont)) {
 				if (isHandlerPath(tb, cont)) {
 					break;
 				}
@@ -170,7 +160,7 @@ public class ProcessTryCatchRegions extends AbstractRegionVisitor {
 
 	private static boolean isHandlerPath(TryCatchBlock tb, IContainer cont) {
 		for (ExceptionHandler h : tb.getHandlers()) {
-			if (RegionUtils.hasPathThruBlock(h.getHandlerBlock(), cont)) {
+			if (RegionUtils.hasPathThroughBlock(h.getHandlerBlock(), cont)) {
 				return true;
 			}
 		}

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

@@ -19,6 +19,7 @@ import jadx.core.utils.exceptions.JadxRuntimeException;
 
 import java.util.ArrayList;
 import java.util.BitSet;
+import java.util.Collection;
 import java.util.Collections;
 import java.util.HashSet;
 import java.util.LinkedList;
@@ -342,6 +343,25 @@ public class BlockUtils {
 		return traverseSuccessorsUntil(start, end, new BitSet());
 	}
 
+	public static BlockNode getTopBlock(Collection<BlockNode> blocks) {
+		if (blocks.size() == 1) {
+			return blocks.iterator().next();
+		}
+		for (BlockNode from : blocks) {
+			boolean top = true;
+			for (BlockNode to : blocks) {
+				if (from != to && !isPathExists(from, to)) {
+					top = false;
+					break;
+				}
+			}
+			if (top) {
+				return from;
+			}
+		}
+		return null;
+	}
+
 	public static boolean isOnlyOnePathExists(BlockNode start, BlockNode end) {
 		if (start == end) {
 			return true;

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

@@ -59,6 +59,10 @@ public class DebugUtils {
 		printRegions(mth, false);
 	}
 
+	public static void printRegion(MethodNode mth, IRegion region, boolean printInsn) {
+		printRegion(mth, region, "", printInsn);
+	}
+
 	public static void printRegions(MethodNode mth, boolean printInsns) {
 		LOG.debug("|{}", mth.toString());
 		printRegion(mth, mth.getRegion(), "|  ", printInsns);

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

@@ -271,7 +271,7 @@ public class RegionUtils {
 		}
 	}
 
-	public static boolean hasPathThruBlock(BlockNode block, IContainer cont) {
+	public static boolean hasPathThroughBlock(BlockNode block, IContainer cont) {
 		if (block == cont) {
 			return true;
 		}
@@ -282,7 +282,7 @@ public class RegionUtils {
 		} else if (cont instanceof IRegion) {
 			IRegion region = (IRegion) cont;
 			for (IContainer c : region.getSubBlocks()) {
-				if (!hasPathThruBlock(block, c)) {
+				if (!hasPathThroughBlock(block, c)) {
 					return false;
 				}
 			}

jadx-core/src/test/java/jadx/tests/integration/trycatch/TestTryCatchFinally4.java

+package jadx.tests.integration.trycatch;
+
+import jadx.core.dex.nodes.ClassNode;
+import jadx.tests.api.IntegrationTest;
+
+import java.io.File;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.io.OutputStream;
+
+import org.junit.Test;
+
+import static jadx.tests.api.utils.JadxMatchers.containsOne;
+import static org.junit.Assert.assertThat;
+
+public class TestTryCatchFinally4 extends IntegrationTest {
+
+	public static class TestCls {
+		public void test() throws IOException {
+			File file = File.createTempFile("test", "txt");
+			OutputStream outputStream = new FileOutputStream(file);
+			try {
+				outputStream.write(1);
+			} finally {
+				try {
+					outputStream.close();
+					file.delete();
+				} catch (IOException e) {
+				}
+			}
+		}
+	}
+
+	@Test
+	public void test() {
+		ClassNode cls = getClassNode(TestCls.class);
+		String code = cls.getCode().toString();
+
+		assertThat(code, containsOne("File file = File.createTempFile(\"test\", \"txt\");"));
+		assertThat(code, containsOne("OutputStream outputStream = new FileOutputStream(file);"));
+		assertThat(code, containsOne("outputStream.write(1);"));
+		assertThat(code, containsOne("} finally {"));
+		assertThat(code, containsOne("outputStream.close();"));
+		assertThat(code, containsOne("} catch (IOException e) {"));
+	}
+}