fix: checks for field init reorder (#1599)

skylot · Aug 4, 2022 · 6e5899c · 6e5899c
1 parent c66ffaa
commit 6e5899c
Show file tree

Hide file tree

Showing 6 changed files with 83 additions and 29 deletions.
diff --git a/jadx-core/src/main/java/jadx/core/dex/nodes/FieldNode.java b/jadx-core/src/main/java/jadx/core/dex/nodes/FieldNode.java
@@ -57,6 +57,10 @@ public boolean isStatic() {
 		return accFlags.isStatic();
 	}
 
+	public boolean isInstance() {
+		return !accFlags.isStatic();
+	}
+
 	public String getName() {
 		return fieldInfo.getName();
 	}

diff --git a/jadx-core/src/main/java/jadx/core/dex/nodes/InsnNode.java b/jadx-core/src/main/java/jadx/core/dex/nodes/InsnNode.java
@@ -264,21 +264,6 @@ public boolean canReorder() {
 		}
 	}
 
-	public boolean canReorderRecursive() {
-		if (!canReorder()) {
-			return false;
-		}
-		for (InsnArg arg : this.getArguments()) {
-			if (arg.isInsnWrap()) {
-				InsnNode wrapInsn = ((InsnWrapArg) arg).getWrapInsn();
-				if (!wrapInsn.canReorderRecursive()) {
-					return false;
-				}
-			}
-		}
-		return true;
-	}
-
 	public boolean containsWrappedInsn() {
 		for (InsnArg arg : this.getArguments()) {
 			if (arg.isInsnWrap()) {

diff --git a/jadx-core/src/main/java/jadx/core/dex/visitors/ExtractFieldInit.java b/jadx-core/src/main/java/jadx/core/dex/visitors/ExtractFieldInit.java
@@ -29,6 +29,7 @@
 import jadx.core.dex.visitors.shrink.CodeShrinkVisitor;
 import jadx.core.utils.BlockUtils;
 import jadx.core.utils.InsnRemover;
+import jadx.core.utils.ListUtils;
 import jadx.core.utils.Utils;
 import jadx.core.utils.exceptions.JadxException;
 
@@ -45,20 +46,22 @@ public boolean visit(ClassNode cls) throws JadxException {
 		for (ClassNode inner : cls.getInnerClasses()) {
 			visit(inner);
 		}
-		moveStaticFieldsInit(cls);
-		moveCommonFieldsInit(cls);
+		if (!cls.getFields().isEmpty()) {
+			moveStaticFieldsInit(cls);
+			moveCommonFieldsInit(cls);
+		}
 		return false;
 	}
 
 	private static final class FieldInitInfo {
 		final FieldNode fieldNode;
 		final IndexInsnNode putInsn;
-		final boolean singlePath;
+		final boolean canMove;
 
-		public FieldInitInfo(FieldNode fieldNode, IndexInsnNode putInsn, boolean singlePath) {
+		public FieldInitInfo(FieldNode fieldNode, IndexInsnNode putInsn, boolean canMove) {
 			this.fieldNode = fieldNode;
 			this.putInsn = putInsn;
-			this.singlePath = singlePath;
+			this.canMove = canMove;
 		}
 	}
 
@@ -80,6 +83,9 @@ private static void moveStaticFieldsInit(ClassNode cls) {
 				|| classInitMth.getBasicBlocks() == null) {
 			return;
 		}
+		if (ListUtils.noneMatch(cls.getFields(), FieldNode::isStatic)) {
+			return;
+		}
 		while (processStaticFields(cls, classInitMth)) {
 			// sometimes instructions moved to field init prevent from vars inline -> inline and try again
 			CodeShrinkVisitor.shrinkMethod(classInitMth);
@@ -116,15 +122,15 @@ private static boolean processStaticFields(ClassNode cls, MethodNode classInitMt
 	}
 
 	private static void moveCommonFieldsInit(ClassNode cls) {
+		if (ListUtils.noneMatch(cls.getFields(), FieldNode::isInstance)) {
+			return;
+		}
 		List<MethodNode> constructors = getConstructorsList(cls);
 		if (constructors.isEmpty()) {
 			return;
 		}
 		List<ConstructorInitInfo> infoList = new ArrayList<>(constructors.size());
 		for (MethodNode constructorMth : constructors) {
-			if (constructorMth.isNoCode()) {
-				return;
-			}
 			List<FieldInitInfo> inits = collectFieldsInit(cls, constructorMth, InsnType.IPUT);
 			filterFieldsInit(inits);
 			if (inits.isEmpty()) {
@@ -168,19 +174,25 @@ private static List<FieldInitInfo> collectFieldsInit(ClassNode cls, MethodNode m
 		Set<BlockNode> singlePathBlocks = new HashSet<>();
 		BlockUtils.visitSinglePath(mth.getEnterBlock(), singlePathBlocks::add);
 
+		boolean canReorder = true;
 		for (BlockNode block : mth.getBasicBlocks()) {
 			for (InsnNode insn : block.getInstructions()) {
+				boolean fieldInsn = false;
 				if (insn.getType() == putType) {
 					IndexInsnNode putInsn = (IndexInsnNode) insn;
 					FieldInfo field = (FieldInfo) putInsn.getIndex();
 					if (field.getDeclClass().equals(cls.getClassInfo())) {
 						FieldNode fn = cls.searchField(field);
 						if (fn != null) {
-							boolean singlePath = singlePathBlocks.contains(block);
-							fieldsInit.add(new FieldInitInfo(fn, putInsn, singlePath));
+							boolean canMove = canReorder && singlePathBlocks.contains(block);
+							fieldsInit.add(new FieldInitInfo(fn, putInsn, canMove));
+							fieldInsn = true;
 						}
 					}
 				}
+				if (!fieldInsn && canReorder && !insn.canReorder()) {
+					canReorder = false;
+				}
 			}
 		}
 		return fieldsInit;
@@ -226,14 +238,14 @@ private static void filterFieldsInit(List<FieldInitInfo> inits) {
 	}
 
 	private static boolean checkInsn(FieldInitInfo initInfo) {
-		if (!initInfo.singlePath) {
+		if (!initInfo.canMove) {
 			return false;
 		}
 		IndexInsnNode insn = initInfo.putInsn;
 		InsnArg arg = insn.getArg(0);
 		if (arg.isInsnWrap()) {
 			InsnNode wrapInsn = ((InsnWrapArg) arg).getWrapInsn();
-			if (!wrapInsn.canReorderRecursive() && insn.contains(AType.EXC_CATCH)) {
+			if (!wrapInsn.canReorder() && insn.contains(AType.EXC_CATCH)) {
 				return false;
 			}
 		} else {
@@ -364,7 +376,7 @@ private static List<MethodNode> getConstructorsList(ClassNode cls) {
 			AccessInfo accFlags = mth.getAccessFlags();
 			if (!accFlags.isStatic() && accFlags.isConstructor()) {
 				list.add(mth);
-				if (BlockUtils.isAllBlocksEmpty(mth.getBasicBlocks())) {
+				if (mth.isNoCode() || BlockUtils.isAllBlocksEmpty(mth.getBasicBlocks())) {
 					return Collections.emptyList();
 				}
 			}

diff --git a/jadx-core/src/main/java/jadx/core/utils/ListUtils.java b/jadx-core/src/main/java/jadx/core/utils/ListUtils.java
@@ -160,6 +160,10 @@ public static <T> boolean allMatch(Collection<T> list, Predicate<T> test) {
 		return true;
 	}
 
+	public static <T> boolean noneMatch(Collection<T> list, Predicate<T> test) {
+		return !anyMatch(list, test);
+	}
+
 	public static <T> boolean anyMatch(Collection<T> list, Predicate<T> test) {
 		if (list == null || list.isEmpty()) {
 			return false;

diff --git a/jadx-core/src/test/java/jadx/tests/api/compiler/TestCompiler.java b/jadx-core/src/test/java/jadx/tests/api/compiler/TestCompiler.java
@@ -113,7 +113,7 @@ public Object invoke(String clsFullName, String methodName, Class<?>[] types, Ob
 			assertNotNull(mth, "Failed to get method " + methodName + '(' + Arrays.toString(types) + ')');
 			return mth.invoke(inst, args);
 		} catch (Throwable e) {
-			IntegrationTest.rethrow("Invoke error", e);
+			IntegrationTest.rethrow("Invoke error for method: " + methodName, e);
 			return null;
 		}
 	}

diff --git a/jadx-core/src/test/java/jadx/tests/integration/others/TestFieldInitNegative.java b/jadx-core/src/test/java/jadx/tests/integration/others/TestFieldInitNegative.java
@@ -0,0 +1,49 @@
+package jadx.tests.integration.others;
+
+import org.junit.jupiter.api.Test;
+
+import jadx.tests.api.IntegrationTest;
+
+import static jadx.tests.api.utils.assertj.JadxAssertions.assertThat;
+
+/**
+ * Negative case for field initialization move (#1599).
+ * Can't reorder with other instance methods.
+ */
+public class TestFieldInitNegative extends IntegrationTest {
+
+	public static class TestCls {
+		StringBuilder sb;
+		int field;
+
+		public TestCls() {
+			initBuilder(new StringBuilder("sb"));
+			this.field = initField();
+			this.sb.append(this.field);
+		}
+
+		private void initBuilder(StringBuilder sb) {
+			this.sb = sb;
+		}
+
+		private int initField() {
+			return sb.length();
+		}
+
+		public String getStr() {
+			return sb.toString();
+		}
+
+		public void check() {
+			assertThat(new TestCls().getStr()).isEqualTo("sb2"); // no NPE
+		}
+	}
+
+	@Test
+	public void test() {
+		assertThat(getClassNode(TestCls.class))
+				.code()
+				.doesNotContain("int field = initField();")
+				.containsOne("this.field = initField();");
+	}
+}