fix: change not allowed access modifiers for methods (#387) (PR #439)

Fix visibility access modifies for methods (see discussions in #370 and #387):
    * all virtual methods become public
    * direct methods become private (instead constructors and static methods for now)
    * such modifications perform by default and can be disabled by the option in preferences (`--respect-bytecode-access-modifiers` in jadx-cli)
    * if changed to method added comment (`Access modifiers changed, original: private`)

jadx-cli/src/main/java/jadx/cli/JadxCLIArgs.java

@@ -53,6 +53,9 @@ public class JadxCLIArgs {
 	@Parameter(names = {"--escape-unicode"}, description = "escape non latin characters in strings (with \\u)")
 	protected boolean escapeUnicode = false;
 
+	@Parameter(names = {"--respect-bytecode-access-modifiers"}, description = "don't change original access modifiers")
+	protected boolean respectBytecodeAccessModifiers = false;
+
 	@Parameter(names = {"--deobf"}, description = "activate deobfuscation")
 	protected boolean deobfuscationOn = false;
 
@@ -154,6 +157,7 @@ public JadxArgs toJadxArgs() {
 		args.setDeobfuscationMaxLength(deobfuscationMaxLength);
 		args.setUseSourceNameAsClassAlias(deobfuscationUseSourceNameAsAlias);
 		args.setEscapeUnicode(escapeUnicode);
+		args.setRespectBytecodeAccModifiers(respectBytecodeAccessModifiers);
 		args.setExportAsGradleProject(exportAsGradleProject);
 		args.setUseImports(useImports);
 		return args;
@@ -239,6 +243,10 @@ public boolean isReplaceConsts() {
 		return replaceConsts;
 	}
 
+	public boolean isRespectBytecodeAccessModifiers() {
+		return respectBytecodeAccessModifiers;
+	}
+
 	public boolean isExportAsGradleProject() {
 		return exportAsGradleProject;
 	}

jadx-core/src/main/java/jadx/api/JadxArgs.java

@@ -40,6 +40,7 @@ public class JadxArgs {
 
 	private boolean escapeUnicode = false;
 	private boolean replaceConsts = true;
+	private boolean respectBytecodeAccModifiers = false;
 	private boolean exportAsGradleProject = false;
 
 	public JadxArgs() {
@@ -204,6 +205,14 @@ public void setReplaceConsts(boolean replaceConsts) {
 		this.replaceConsts = replaceConsts;
 	}
 
+	public boolean isRespectBytecodeAccModifiers() {
+		return respectBytecodeAccModifiers;
+	}
+
+	public void setRespectBytecodeAccModifiers(boolean respectBytecodeAccModifiers) {
+		this.respectBytecodeAccModifiers = respectBytecodeAccModifiers;
+	}
+
 	public boolean isExportAsGradleProject() {
 		return exportAsGradleProject;
 	}
@@ -234,8 +243,10 @@ public String toString() {
 		sb.append(", deobfuscationMaxLength=").append(deobfuscationMaxLength);
 		sb.append(", escapeUnicode=").append(escapeUnicode);
 		sb.append(", replaceConsts=").append(replaceConsts);
+		sb.append(", respectBytecodeAccModifiers=").append(respectBytecodeAccModifiers);
 		sb.append(", exportAsGradleProject=").append(exportAsGradleProject);
 		sb.append('}');
 		return sb.toString();
 	}
+
 }

jadx-core/src/main/java/jadx/core/Jadx.java

@@ -19,6 +19,7 @@
 import jadx.core.dex.visitors.EnumVisitor;
 import jadx.core.dex.visitors.ExtractFieldInit;
 import jadx.core.dex.visitors.FallbackModeVisitor;
+import jadx.core.dex.visitors.FixAccessModifiers;
 import jadx.core.dex.visitors.IDexTreeVisitor;
 import jadx.core.dex.visitors.MethodInlineVisitor;
 import jadx.core.dex.visitors.ModVisitor;
@@ -96,6 +97,7 @@ public static List<IDexTreeVisitor> getPassesList(JadxArgs args) {
 
 			passes.add(new MethodInlineVisitor());
 			passes.add(new ExtractFieldInit());
+			passes.add(new FixAccessModifiers());
 			passes.add(new ClassModifier());
 			passes.add(new EnumVisitor());
 			passes.add(new PrepareForCodeGen());

jadx-core/src/main/java/jadx/core/dex/info/AccessInfo.java

@@ -6,6 +6,7 @@
 
 public class AccessInfo {
 
+	public static final int VISIBILITY_FLAGS = AccessFlags.ACC_PUBLIC | AccessFlags.ACC_PROTECTED | AccessFlags.ACC_PRIVATE;
 	private final int accFlags;
 
 	public enum AFType {
@@ -30,11 +31,24 @@ public AccessInfo remove(int flag) {
 		return this;
 	}
 
+	public AccessInfo add(int flag) {
+		if (!containsFlag(flag)) {
+			return new AccessInfo(accFlags | flag, type);
+		}
+		return this;
+	}
+
+	public AccessInfo changeVisibility(int flag) {
+		int currentVisFlags = accFlags & VISIBILITY_FLAGS;
+		if (currentVisFlags == flag) {
+			return this;
+		}
+		int unsetAllVisFlags = accFlags & ~VISIBILITY_FLAGS;
+		return new AccessInfo(unsetAllVisFlags | flag, type);
+	}
+
 	public AccessInfo getVisibility() {
-		int f = accFlags & AccessFlags.ACC_PUBLIC
-				| accFlags & AccessFlags.ACC_PROTECTED
-				| accFlags & AccessFlags.ACC_PRIVATE;
-		return new AccessInfo(f, type);
+		return new AccessInfo(accFlags & VISIBILITY_FLAGS, type);
 	}
 
 	public boolean isPublic() {

jadx-core/src/main/java/jadx/core/dex/nodes/MethodNode.java

@@ -51,7 +51,7 @@ public class MethodNode extends LineAttrNode implements ILoadable, IDexNode {
 
 	private final MethodInfo mthInfo;
 	private final ClassNode parentClass;
-	private final AccessInfo accFlags;
+	private AccessInfo accFlags;
 
 	private final Method methodData;
 	private int regsCount;
@@ -599,6 +599,10 @@ public AccessInfo getAccessFlags() {
 		return accFlags;
 	}
 
+	public void setAccFlags(AccessInfo accFlags) {
+		this.accFlags = accFlags;
+	}
+
 	public Region getRegion() {
 		return region;
 	}

jadx-core/src/main/java/jadx/core/dex/visitors/FixAccessModifiers.java

@@ -0,0 +1,57 @@
+package jadx.core.dex.visitors;
+
+import com.android.dx.rop.code.AccessFlags;
+
+import jadx.core.dex.attributes.AType;
+import jadx.core.dex.info.AccessInfo;
+import jadx.core.dex.nodes.MethodNode;
+import jadx.core.dex.nodes.RootNode;
+
+@JadxVisitor(
+		name = "FixAccessModifiers",
+		desc = "Change class and method access modifiers if needed",
+		runAfter = ModVisitor.class
+)
+public class FixAccessModifiers extends AbstractVisitor {
+
+	private boolean respectAccessModifiers;
+
+	@Override
+	public void init(RootNode root) {
+		this.respectAccessModifiers = root.getArgs().isRespectBytecodeAccModifiers();
+	}
+
+	@Override
+	public void visit(MethodNode mth) {
+		if (respectAccessModifiers) {
+			return;
+		}
+		AccessInfo accessFlags = mth.getAccessFlags();
+		int newVisFlag = fixVisibility(mth, accessFlags);
+		if (newVisFlag != 0) {
+			AccessInfo newAccFlags = accessFlags.changeVisibility(newVisFlag);
+			if (newAccFlags != accessFlags) {
+				mth.setAccFlags(newAccFlags);
+				mth.addAttr(AType.COMMENTS, "Access modifiers changed, original: " + accessFlags.rawString());
+			}
+		}
+	}
+
+	private int fixVisibility(MethodNode mth, AccessInfo accessFlags) {
+		if (mth.isVirtual()) {
+			// make virtual methods public
+			return AccessFlags.ACC_PUBLIC;
+		} else {
+			if (accessFlags.isAbstract()) {
+				// make abstract methods public
+				return AccessFlags.ACC_PUBLIC;
+			}
+			if (accessFlags.isConstructor() || accessFlags.isStatic()) {
+				// TODO: make public if used outside
+				return 0;
+			}
+			// make other direct methods private
+			return AccessFlags.ACC_PRIVATE;
+		}
+	}
+}

jadx-core/src/test/java/jadx/core/dex/info/AccessInfoTest.java

@@ -0,0 +1,32 @@
+package jadx.core.dex.info;
+
+import com.android.dx.rop.code.AccessFlags;
+import org.junit.Test;
+
+import jadx.core.dex.info.AccessInfo.AFType;
+
+import static org.hamcrest.Matchers.is;
+import static org.junit.Assert.assertSame;
+import static org.junit.Assert.assertThat;
+
+public class AccessInfoTest {
+
+	@Test
+	public void changeVisibility() {
+		AccessInfo accessInfo = new AccessInfo(AccessFlags.ACC_PROTECTED | AccessFlags.ACC_STATIC, AFType.METHOD);
+		AccessInfo result = accessInfo.changeVisibility(AccessFlags.ACC_PUBLIC);
+
+		assertThat(result.isPublic(), is(true));
+		assertThat(result.isPrivate(), is(false));
+		assertThat(result.isProtected(), is(false));
+
+		assertThat(result.isStatic(), is(true));
+	}
+
+	@Test
+	public void changeVisibilityNoOp() {
+		AccessInfo accessInfo = new AccessInfo(AccessFlags.ACC_PUBLIC, AFType.METHOD);
+		AccessInfo result = accessInfo.changeVisibility(AccessFlags.ACC_PUBLIC);
+		assertSame(accessInfo, result);
+	}
+}

jadx-core/src/test/java/jadx/tests/api/SmaliTest.java

@@ -1,6 +1,10 @@
 package jadx.tests.api;
 
 import java.io.File;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.stream.Collectors;
 
 import org.jf.smali.Smali;
 import org.jf.smali.SmaliOptions;
@@ -16,7 +20,7 @@ public abstract class SmaliTest extends IntegrationTest {
 	protected ClassNode getClassNodeFromSmali(String file, String clsName) {
 		File smaliFile = getSmaliFile(file);
 		File outDex = createTempFile(".dex");
-		compileSmali(smaliFile, outDex);
+		compileSmali(outDex, Collections.singletonList(smaliFile));
 		return getClassNodeFromFile(outDex, clsName);
 	}
 
@@ -28,27 +32,37 @@ protected ClassNode getClassNodeFromSmaliWithPkg(String pkg, String clsName) {
 		return getClassNodeFromSmali(pkg + File.separatorChar + clsName, pkg + '.' + clsName);
 	}
 
+	protected ClassNode getClassNodeFromSmaliFiles(String pkg, String testName, String clsName, String... smaliFileNames) {
+		File outDex = createTempFile(".dex");
+		List<File> smaliFiles = Arrays.stream(smaliFileNames)
+				.map(file -> getSmaliFile(pkg + File.separatorChar + testName + File.separatorChar + file))
+				.collect(Collectors.toList());
+		compileSmali(outDex, smaliFiles);
+		return getClassNodeFromFile(outDex, pkg + "." + clsName);
+	}
+
 	protected ClassNode getClassNodeFromSmali(String clsName) {
 		return getClassNodeFromSmali(clsName, clsName);
 	}
 
-	private static File getSmaliFile(String clsName) {
-		File smaliFile = new File(SMALI_TESTS_DIR, clsName + SMALI_TESTS_EXT);
+	private static File getSmaliFile(String baseName) {
+		File smaliFile = new File(SMALI_TESTS_DIR, baseName + SMALI_TESTS_EXT);
 		if (smaliFile.exists()) {
 			return smaliFile;
 		}
-		smaliFile = new File(SMALI_TESTS_PROJECT, smaliFile.getPath());
-		if (smaliFile.exists()) {
-			return smaliFile;
+		File pathFromRoot = new File(SMALI_TESTS_PROJECT, smaliFile.getPath());
+		if (pathFromRoot.exists()) {
+			return pathFromRoot;
 		}
-		throw new AssertionError("Smali file not found: " + smaliFile.getAbsolutePath());
+		throw new AssertionError("Smali file not found: " + smaliFile.getPath());
 	}
 
-	private static boolean compileSmali(File input, File output) {
+	private static boolean compileSmali(File output, List<File> inputFiles) {
 		try {
 			SmaliOptions params = new SmaliOptions();
 			params.outputDexFile = output.getAbsolutePath();
-			Smali.assemble(params, input.getAbsolutePath());
+			List<String> inputFileNames = inputFiles.stream().map(File::getAbsolutePath).collect(Collectors.toList());
+			Smali.assemble(params, inputFileNames);
 		} catch (Exception e) {
 			throw new AssertionError("Smali assemble error", e);
 		}

jadx-core/src/test/java/jadx/tests/integration/debuginfo/TestLineNumbers2.java

@@ -19,7 +19,7 @@ public static class TestCls {
 		public TestCls(TestCls s) {
 		}
 
-		TestCls test(TestCls s) {
+		public TestCls test(TestCls s) {
 			TestCls store = f != null ? f.get() : null;
 			if (store == null) {
 				store = new TestCls(s);

jadx-core/src/test/java/jadx/tests/integration/enums/TestEnums2.java

@@ -14,17 +14,17 @@ public static class TestCls {
 
 		public enum Operation {
 			PLUS {
-				int apply(int x, int y) {
+				public int apply(int x, int y) {
 					return x + y;
 				}
 			},
 			MINUS {
-				int apply(int x, int y) {
+				public int apply(int x, int y) {
 					return x - y;
 				}
 			};
 
-			abstract int apply(int x, int y);
+			public abstract int apply(int x, int y);
 		}
 	}
 
@@ -36,17 +36,17 @@ public void test() {
 		assertThat(code, JadxMatchers.containsLines(1,
 				"public enum Operation {",
 				indent(1) + "PLUS {",
-				indent(2) + "int apply(int x, int y) {",
+				indent(2) + "public int apply(int x, int y) {",
 				indent(3) + "return x + y;",
 				indent(2) + "}",
 				indent(1) + "},",
 				indent(1) + "MINUS {",
-				indent(2) + "int apply(int x, int y) {",
+				indent(2) + "public int apply(int x, int y) {",
 				indent(3) + "return x - y;",
 				indent(2) + "}",
 				indent(1) + "};",
 				"",
-				indent(1) + "abstract int apply(int i, int i2);",
+				indent(1) + "public abstract int apply(int i, int i2);",
 				"}"
 		));
 	}

jadx-core/src/test/java/jadx/tests/integration/inner/TestInnerClassSyntheticRename.java

@@ -34,8 +34,8 @@ public void test() {
 		ClassNode cls = getClassNodeFromSmali("inner/TestInnerClassSyntheticRename", "com.github.skylot.testasync.MyAsync");
 		String code = cls.getCode().toString();
 
-		assertThat(code, containsOne("protected List<Uri> doInBackground(Uri... uriArr) {"));
-		assertThat(code, containsOne("protected void onPostExecute(List<Uri> list) {"));
+		assertThat(code, containsOne("List<Uri> doInBackground(Uri... uriArr) {"));
+		assertThat(code, containsOne("void onPostExecute(List<Uri> list) {"));
 		assertThat(code, not(containsString("synthetic")));
 	}
 }

jadx-core/src/test/java/jadx/tests/integration/others/TestBadMethodAccessModifiers.java

@@ -0,0 +1,36 @@
+package jadx.tests.integration.others;
+
+import org.junit.Test;
+
+import jadx.core.dex.nodes.ClassNode;
+import jadx.tests.api.SmaliTest;
+
+import static jadx.tests.api.utils.JadxMatchers.containsOne;
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.not;
+import static org.junit.Assert.assertThat;
+
+public class TestBadMethodAccessModifiers extends SmaliTest {
+	/*
+		public static class TestCls {
+
+			public abstract class A {
+				public abstract void test();
+			}
+
+			public class B extends A {
+				protected void test() {
+				}
+			}
+		}
+	*/
+	@Test
+	public void test() {
+		ClassNode cls = getClassNodeFromSmaliFiles("others", "TestBadMethodAccessModifiers", "TestCls",
+				"TestCls$A", "TestCls$B", "TestCls");
+		String code = cls.getCode().toString();
+
+		assertThat(code, not(containsString("protected void test() {")));
+		assertThat(code, containsOne("public void test() {"));
+	}
+}