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 class JadxCLIArgs {
 		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 class JadxCLIArgs {
 		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 class JadxArgs {
 		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 class JadxArgs {
 		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.DotGraphVisitor;
 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 class Jadx {
 
 			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 @@ import jadx.core.Consts;
 
 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 class AccessInfo {
 		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 class MethodNode extends LineAttrNode implements ILoadable, IDexNode {
 		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

+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

+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

 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 @@ public abstract class SmaliTest extends IntegrationTest {
 		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 class TestLineNumbers2 extends IntegrationTest {
 		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 class TestEnums2 extends IntegrationTest {
 
 		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 class TestEnums2 extends IntegrationTest {
 		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 class TestInnerClassSyntheticRename extends SmaliTest {
 		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

+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() {"));
+	}
+}

jadx-core/src/test/smali/others/TestBadMethodAccessModifiers/TestCls$A.smali

+.class public abstract Lothers/TestCls$A;
+.super Ljava/lang/Object;
+.source "TestCls.java"
+
+
+# annotations
+.annotation system Ldalvik/annotation/EnclosingClass;
+    value = Lothers/TestCls;
+.end annotation
+
+.annotation system Ldalvik/annotation/InnerClass;
+    accessFlags = 0x401
+    name = "A"
+.end annotation
+
+
+# instance fields
+.field final synthetic this$0:Lothers/TestCls;
+
+
+# direct methods
+.method public constructor <init>(Lothers/TestCls;)V
+    .registers 2
+    .param p1, "this$0"    # Lothers/TestCls;
+
+    .prologue
+    .line 5
+    iput-object p1, p0, Lothers/TestCls$A;->this$0:Lothers/TestCls;
+
+    invoke-direct {p0}, Ljava/lang/Object;-><init>()V
+
+    return-void
+.end method
+
+
+# virtual methods
+.method public abstract test()V
+.end method

jadx-core/src/test/smali/others/TestBadMethodAccessModifiers/TestCls$B.smali

+.class public Lothers/TestCls$B;
+.super Lothers/TestCls$A;
+.source "TestCls.java"
+
+
+# annotations
+.annotation system Ldalvik/annotation/EnclosingClass;
+    value = Lothers/TestCls;
+.end annotation
+
+.annotation system Ldalvik/annotation/InnerClass;
+    accessFlags = 0x1
+    name = "B"
+.end annotation
+
+
+# instance fields
+.field final synthetic this$0:Lothers/TestCls;
+
+
+# direct methods
+.method public constructor <init>(Lothers/TestCls;)V
+    .registers 2
+    .param p1, "this$0"    # Lothers/TestCls;
+
+    .prologue
+    .line 9
+    iput-object p1, p0, Lothers/TestCls$B;->this$0:Lothers/TestCls;
+
+    invoke-direct {p0, p1}, Lothers/TestCls$A;-><init>(Lothers/TestCls;)V
+
+    return-void
+.end method
+
+
+# virtual methods
+.method protected test()V
+    .registers 1
+
+    .prologue
+    .line 11
+    return-void
+.end method

jadx-core/src/test/smali/others/TestBadMethodAccessModifiers/TestCls.smali

+.class public Lothers/TestCls;
+.super Ljava/lang/Object;
+.source "TestCls.java"
+
+
+# annotations
+.annotation system Ldalvik/annotation/MemberClasses;
+    value = {
+        Lothers/TestCls$B;,
+        Lothers/TestCls$A;
+    }
+.end annotation
+
+
+# direct methods
+.method public constructor <init>()V
+    .registers 1
+
+    .prologue
+    .line 3
+    invoke-direct {p0}, Ljava/lang/Object;-><init>()V
+
+    return-void
+.end method

jadx-gui/src/main/java/jadx/gui/settings/JadxSettings.java

@@ -26,7 +26,7 @@ public class JadxSettings extends JadxCLIArgs {
 
 	private static final String USER_HOME = System.getProperty("user.home");
 	private static final int RECENT_FILES_COUNT = 15;
-	private static final int CURRENT_SETTINGS_VERSION = 5;
+	private static final int CURRENT_SETTINGS_VERSION = 6;
 
 	private static final Font DEFAULT_FONT = FONT_HACK != null ? FONT_HACK : new RSyntaxTextArea().getFont();
 
@@ -235,6 +235,10 @@ public class JadxSettings extends JadxCLIArgs {
 		this.replaceConsts = replaceConsts;
 	}
 
+	public void setRespectBytecodeAccessModifiers(boolean respectBytecodeAccessModifiers) {
+		this.respectBytecodeAccessModifiers = respectBytecodeAccessModifiers;
+	}
+
 	public void setUseImports(boolean useImports) {
 		this.useImports = useImports;
 	}
@@ -311,6 +315,10 @@ public class JadxSettings extends JadxCLIArgs {
 		}
 		if (fromVersion == 4) {
 			setUseImports(true);
+			fromVersion++;
+		}
+		if (fromVersion == 5) {
+			setRespectBytecodeAccessModifiers(false);
 		}
 		settingsVersion = CURRENT_SETTINGS_VERSION;
 		sync();

jadx-gui/src/main/java/jadx/gui/settings/JadxSettingsWindow.java

@@ -259,6 +259,13 @@ public class JadxSettingsWindow extends JDialog {
 			needReload();
 		});
 
+		JCheckBox respectBytecodeAccessModifiers = new JCheckBox();
+		respectBytecodeAccessModifiers.setSelected(settings.isRespectBytecodeAccessModifiers());
+		respectBytecodeAccessModifiers.addItemListener(e -> {
+			settings.setRespectBytecodeAccessModifiers(e.getStateChange() == ItemEvent.SELECTED);
+			needReload();
+		});
+
 		JCheckBox useImports = new JCheckBox();
 		useImports.setSelected(settings.isUseImports());
 		useImports.addItemListener(e -> {
@@ -274,6 +281,7 @@ public class JadxSettingsWindow extends JDialog {
 		other.addRow(NLS.str("preferences.showInconsistentCode"), showInconsistentCode);
 		other.addRow(NLS.str("preferences.escapeUnicode"), escapeUnicode);
 		other.addRow(NLS.str("preferences.replaceConsts"), replaceConsts);
+		other.addRow(NLS.str("preferences.respectBytecodeAccessModifiers"), respectBytecodeAccessModifiers);
 		other.addRow(NLS.str("preferences.useImports"), useImports);
 		other.addRow(NLS.str("preferences.fallback"), fallback);
 		other.addRow(NLS.str("preferences.skipResourcesDecode"), resourceDecode);

jadx-gui/src/main/resources/i18n/Messages_en_US.properties

@@ -86,6 +86,7 @@ preferences.fallback=Fallback mode (simple dump)
 preferences.showInconsistentCode=Show inconsistent code
 preferences.escapeUnicode=Escape unicode
 preferences.replaceConsts=Replace constants
+preferences.respectBytecodeAccessModifiers=Respect bytecode access modifiers
 preferences.useImports=Use import statements
 preferences.skipResourcesDecode=Don't decode resources
 preferences.threads=Processing threads count