fix: various minor improvements (PR #1418)

* chore: better variable naming for getInstance calls
* chore: rebalance preferences window and fix empty plugins section directly after jadx-gui start
* chore: do not ask for project save if nothing had been changed
* use parallel mode for gradle
* minor improvements for app debugging
* apply CodeQL suggestion to prevent log injection
* handle IntelliJ Idea warnings
* replace not-ASCII chars in LogUtils.escape

Co-authored-by: Skylot <skylot@gmail.com>

gradle.properties

@@ -1 +1,2 @@
 org.gradle.warning.mode=all
+org.gradle.parallel=true

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

@@ -268,13 +268,17 @@ public class NameGen {
 
 	private String makeNameFromInvoke(MethodInfo callMth) {
 		String name = callMth.getName();
+		ArgType declType = callMth.getDeclClass().getType();
+		if ("getInstance".equals(name)) {
+			// e.g. Cipher.getInstance
+			return makeNameForType(declType);
+		}
 		if (name.startsWith("get") || name.startsWith("set")) {
 			return fromName(name.substring(3));
 		}
 		if ("iterator".equals(name)) {
 			return "it";
 		}
-		ArgType declType = callMth.getDeclClass().getType();
 		if ("toString".equals(name)) {
 			return makeNameForType(declType);
 		}

jadx-core/src/main/java/jadx/core/utils/log/LogUtils.java

@@ -0,0 +1,30 @@
+package jadx.core.utils.log;
+
+import java.nio.charset.StandardCharsets;
+import java.util.regex.Pattern;
+
+/**
+ * Escape input from untrusted source before pass to logger.
+ * Suggested by CodeQL: https://codeql.github.com/codeql-query-help/java/java-log-injection/
+ */
+public class LogUtils {
+
+	private static final Pattern ALFA_NUMERIC = Pattern.compile("\\w*");
+
+	public static String escape(String input) {
+		if (input == null) {
+			return "null";
+		}
+		if (ALFA_NUMERIC.matcher(input).matches()) {
+			return input;
+		}
+		return input.replaceAll("\\W", ".");
+	}
+
+	public static String escape(byte[] input) {
+		if (input == null) {
+			return "null";
+		}
+		return escape(new String(input, StandardCharsets.UTF_8));
+	}
+}

jadx-core/src/test/java/jadx/core/utils/log/LogUtilsTest.java

@@ -0,0 +1,13 @@
+package jadx.core.utils.log;
+
+import org.junit.jupiter.api.Test;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+class LogUtilsTest {
+
+	@Test
+	void escape() {
+		assertThat(LogUtils.escape("Guest'%0AUser:'Admin")).isEqualTo("Guest..0AUser..Admin");
+	}
+}

jadx-gui/src/main/java/jadx/gui/device/debugger/DebugController.java

@@ -11,6 +11,7 @@ import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 
+import javax.swing.JOptionPane;
 import javax.swing.tree.DefaultMutableTreeNode;
 
 import org.slf4j.Logger;
@@ -40,6 +41,7 @@ import jadx.gui.ui.panel.IDebugController;
 import jadx.gui.ui.panel.JDebuggerPanel;
 import jadx.gui.ui.panel.JDebuggerPanel.IListElement;
 import jadx.gui.ui.panel.JDebuggerPanel.ValueTreeNode;
+import jadx.gui.utils.NLS;
 
 public final class DebugController implements SmaliDebugger.SuspendListener, IDebugController {
 	private static final Logger LOG = LoggerFactory.getLogger(DebugController.class);
@@ -69,23 +71,22 @@ public final class DebugController implements SmaliDebugger.SuspendListener, IDe
 	private final ExecutorService updateQueue = Executors.newSingleThreadExecutor();
 	private final ExecutorService lazyQueue = Executors.newSingleThreadExecutor();
 
-	/**
-	 * @param args at least 3 elements, host, port and android release version respectively.
-	 */
 	@Override
-	public boolean startDebugger(JDebuggerPanel debuggerPanel, String[] args) {
+	public boolean startDebugger(JDebuggerPanel debuggerPanel, String adbHost, int adbPort, int androidVer) {
 		if (TYPE_MAP.isEmpty()) {
 			initTypeMap();
 		}
 		this.debuggerPanel = debuggerPanel;
 		debuggerPanel.resetUI();
 		try {
-			debugger = SmaliDebugger.attach(args[0], Integer.parseInt(args[1]), this);
+			debugger = SmaliDebugger.attach(adbHost, adbPort, this);
 		} catch (SmaliDebuggerException e) {
+			JOptionPane.showMessageDialog(debuggerPanel.getMainWindow(), e.getMessage(),
+					NLS.str("error_dialog.title"), JOptionPane.ERROR_MESSAGE);
 			logErr(e);
 			return false;
 		}
-		art = ArtAdapter.getAdapter(Integer.parseInt(args[2]));
+		art = ArtAdapter.getAdapter(androidVer);
 		resetAllInfo();
 		hasResumed = false;
 		run = debugger::resume;

jadx-gui/src/main/java/jadx/gui/device/protocol/ADB.java

@@ -5,6 +5,7 @@ import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
 import java.net.Socket;
+import java.net.SocketException;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
@@ -14,10 +15,12 @@ import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 import java.util.concurrent.TimeUnit;
 import java.util.stream.Collectors;
+import java.util.stream.Stream;
 
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+import jadx.core.utils.log.LogUtils;
 import jadx.gui.utils.IOUtils;
 
 public class ADB {
@@ -29,10 +32,26 @@ public class ADB {
 	private static final String CMD_FEATURES = "000dhost:features";
 	private static final String CMD_TRACK_DEVICES = "0014host:track-devices-l";
 	private static final byte[] OKAY = "OKAY".getBytes();
+	private static final byte[] FAIL = "FAIL".getBytes();
 
 	static boolean isOkay(InputStream stream) throws IOException {
 		byte[] buf = IOUtils.readNBytes(stream, 4);
-		return Arrays.equals(buf, OKAY);
+		if (Arrays.equals(buf, OKAY)) {
+			return true;
+		}
+		if (Arrays.equals(buf, FAIL)) {
+			// Observed that after FAIL the length in hex follows and afterwards an error message,
+			// but it is unclear if this is true for all cases where isOkay is used.
+			// int msgLen = Integer.parseInt(new String(IOUtils.readNBytes(stream, 4)), 16);
+			// byte[] errorMsg = IOUtils.readNBytes(stream, msgLen);
+			// LOG.error("isOkay failed: received error message: {}", new String(errorMsg));
+			LOG.error("isOkay failed");
+			return false;
+		}
+		if (buf == null) {
+			throw new IOException("isOkay failed - steam ended");
+		}
+		throw new IOException("isOkay failed - unexpected response " + new String(buf));
 	}
 
 	public static byte[] exec(String cmd, OutputStream outputStream, InputStream inputStream) throws IOException {
@@ -40,11 +59,9 @@ public class ADB {
 	}
 
 	public static byte[] exec(String cmd) throws IOException {
-		byte[] res;
 		try (Socket socket = connect()) {
-			res = exec(cmd, socket.getOutputStream(), socket.getInputStream());
+			return exec(cmd, socket.getOutputStream(), socket.getInputStream());
 		}
-		return res;
 	}
 
 	public static Socket connect() throws IOException {
@@ -55,14 +72,12 @@ public class ADB {
 		return new Socket(host, port);
 	}
 
-	static boolean execCommandAsync(OutputStream outputStream,
-			InputStream inputStream, String cmd) throws IOException {
+	static boolean execCommandAsync(OutputStream outputStream, InputStream inputStream, String cmd) throws IOException {
 		outputStream.write(cmd.getBytes());
 		return isOkay(inputStream);
 	}
 
-	private static byte[] execCommandSync(OutputStream outputStream,
-			InputStream inputStream, String cmd) throws IOException {
+	private static byte[] execCommandSync(OutputStream outputStream, InputStream inputStream, String cmd) throws IOException {
 		outputStream.write(cmd.getBytes());
 		if (isOkay(inputStream)) {
 			return readServiceProtocol(inputStream);
@@ -73,15 +88,26 @@ public class ADB {
 	static byte[] readServiceProtocol(InputStream stream) {
 		try {
 			byte[] buf = IOUtils.readNBytes(stream, 4);
-			int len = unhex(buf);
-			if (len == 0) {
-				return new byte[0];
+			if (buf == null) {
+				return null;
 			}
-			return IOUtils.readNBytes(stream, len);
+			int len = unhex(buf);
+			byte[] result;
+			if (len == 0) {
+				result = new byte[0];
+			} else {
+				result = IOUtils.readNBytes(stream, len);
+			}
+			if (LOG.isTraceEnabled()) {
+				LOG.trace("readServiceProtocol result: {}", LogUtils.escape(result));
+			}
+			return result;
+		} catch (SocketException e) {
+			LOG.error("Aborting readServiceProtocol: socket closed");
 		} catch (IOException e) {
-			LOG.error("Failed to read readServiceProtocol: {}", e.toString());
-			return null;
+			LOG.error("Failed to read readServiceProtocol", e);
 		}
+		return null;
 	}
 
 	static boolean setSerial(String serial, OutputStream outputStream, InputStream inputStream) throws IOException {
@@ -91,7 +117,9 @@ public class ADB {
 		boolean ok = isOkay(inputStream);
 		if (ok) {
 			// skip the shell-state-id returned by ADB server, it's not important for the following actions.
-			ok = inputStream.skip(8) == 8;
+			IOUtils.readNBytes(inputStream, 8);
+		} else {
+			LOG.error("setSerial command {} failed", LogUtils.escape(setSerialCmd));
 		}
 		return ok;
 	}
@@ -106,8 +134,7 @@ public class ADB {
 		return null;
 	}
 
-	static byte[] execShellCommandRaw(String serial, String cmd,
-			OutputStream outputStream, InputStream inputStream) throws IOException {
+	static byte[] execShellCommandRaw(String serial, String cmd, OutputStream outputStream, InputStream inputStream) throws IOException {
 		if (setSerial(serial, outputStream, inputStream)) {
 			return execShellCommandRaw(cmd, outputStream, inputStream);
 		}
@@ -124,14 +151,16 @@ public class ADB {
 
 	public static boolean startServer(String adbPath, int port) throws IOException {
 		String tcpPort = String.format("tcp:%d", port);
-		java.lang.Process proc = new ProcessBuilder(adbPath, "-L", tcpPort, "start-server")
+		List<String> command = Arrays.asList(adbPath, "-L", tcpPort, "start-server");
+		java.lang.Process proc = new ProcessBuilder(command)
 				.redirectErrorStream(true)
 				.start();
 		try {
-			proc.waitFor(3, TimeUnit.SECONDS); // for listening to a port, 3 sec should be more than enough.
+			// Wait for the adb server to start. On Windows even on a fast system 6 seconds are not unusual.
+			proc.waitFor(10, TimeUnit.SECONDS);
 			proc.exitValue();
 		} catch (Exception e) {
-			LOG.error("Start server error", e);
+			LOG.error("ADB start server failed with command: {}", String.join(" ", command), e);
 			proc.destroyForcibly();
 			return false;
 		}
@@ -143,7 +172,7 @@ public class ADB {
 				out.write(buf, 0, read);
 			}
 		}
-		return new String(out.toByteArray()).contains(tcpPort);
+		return out.toString().contains(tcpPort);
 	}
 
 	public static boolean isServerRunning(String host, int port) {
@@ -167,22 +196,21 @@ public class ADB {
 		}
 		ExecutorService listenThread = Executors.newFixedThreadPool(1);
 		listenThread.execute(() -> {
-			for (;;) {
+			while (true) {
 				byte[] res = readServiceProtocol(inputStream);
-				if (res != null) {
-					if (listener != null) {
-						String payload = new String(res);
-						String[] deviceLines = payload.split("\n");
-						List<ADBDeviceInfo> deviceInfoList = new ArrayList<>(deviceLines.length);
-						for (String deviceLine : deviceLines) {
-							if (!deviceLine.trim().isEmpty()) {
-								deviceInfoList.add(ADBDeviceInfo.make(deviceLine, host, port));
-							}
+				if (res == null) {
+					break; // socket disconnected
+				}
+				if (listener != null) {
+					String payload = new String(res);
+					String[] deviceLines = payload.split("\n");
+					List<ADBDeviceInfo> deviceInfoList = new ArrayList<>(deviceLines.length);
+					for (String deviceLine : deviceLines) {
+						if (!deviceLine.trim().isEmpty()) {
+							deviceInfoList.add(ADBDeviceInfo.make(deviceLine, host, port));
 						}
-						listener.onDeviceStatusChange(deviceInfoList);
 					}
-				} else { // socket disconnected
-					break;
+					listener.onDeviceStatusChange(deviceInfoList);
 				}
 			}
 			if (listener != null) {
@@ -193,43 +221,39 @@ public class ADB {
 	}
 
 	public static List<String> listForward(String host, int port) throws IOException {
-		Socket socket = connect(host, port);
-		String cmd = "0011host:list-forward";
-		InputStream inputStream = socket.getInputStream();
-		OutputStream outputStream = socket.getOutputStream();
-		outputStream.write(cmd.getBytes());
-		if (isOkay(inputStream)) {
-			byte[] bytes = readServiceProtocol(inputStream);
-			if (bytes != null) {
-				String[] forwards = new String(bytes).split("\n");
-				List<String> forwardList = Arrays.stream(forwards).map(s -> s.trim()).collect(Collectors.toList());
-				socket.close();
-				return forwardList;
+		try (Socket socket = connect(host, port)) {
+			String cmd = "0011host:list-forward";
+			InputStream inputStream = socket.getInputStream();
+			OutputStream outputStream = socket.getOutputStream();
+			outputStream.write(cmd.getBytes());
+			if (isOkay(inputStream)) {
+				byte[] bytes = readServiceProtocol(inputStream);
+				if (bytes != null) {
+					String[] forwards = new String(bytes).split("\n");
+					return Stream.of(forwards).map(String::trim).collect(Collectors.toList());
+				}
 			}
 		}
-		socket.close();
 		return Collections.emptyList();
 	}
 
 	public static boolean removeForward(String host, int port, String serial, String localPort) throws IOException {
-		Socket socket = connect(host, port);
-		String cmd = String.format("host:killforward:tcp:%s", localPort);
-		cmd = String.format("%04x%s", cmd.length(), cmd);
-		InputStream inputStream = socket.getInputStream();
-		OutputStream outputStream = socket.getOutputStream();
-		boolean ok = false;
-		if (setSerial(serial, outputStream, inputStream)) {
-			outputStream.write(cmd.getBytes());
-			ok = isOkay(inputStream) && isOkay(inputStream);
+		try (Socket socket = connect(host, port)) {
+			String cmd = String.format("host:killforward:tcp:%s", localPort);
+			cmd = String.format("%04x%s", cmd.length(), cmd);
+			InputStream inputStream = socket.getInputStream();
+			OutputStream outputStream = socket.getOutputStream();
+			if (setSerial(serial, outputStream, inputStream)) {
+				outputStream.write(cmd.getBytes());
+				return isOkay(inputStream) && isOkay(inputStream);
+			}
 		}
-		socket.close();
-		return ok;
+		return false;
 	}
 
 	// Little endian
 	private static int readInt(byte[] bytes, int start) {
-		int result = 0;
-		result = (bytes[start] & 0xff);
+		int result = (bytes[start] & 0xff);
 		result += ((bytes[start + 1] & 0xff) << 8);
 		result += ((bytes[start + 2] & 0xff) << 16);
 		result += (bytes[start + 3] & 0xff) << 24;

jadx-gui/src/main/java/jadx/gui/device/protocol/ADBDevice.java

@@ -17,6 +17,7 @@ import org.slf4j.LoggerFactory;
 import io.reactivex.annotations.NonNull;
 
 import jadx.core.utils.StringUtils;
+import jadx.core.utils.log.LogUtils;
 import jadx.gui.device.protocol.ADB.JDWPProcessListener;
 import jadx.gui.device.protocol.ADB.Process;
 
@@ -101,6 +102,9 @@ public class ADBDevice {
 		try (Socket socket = ADB.connect(info.adbHost, info.adbPort)) {
 			String cmd = "am start -D -n " + fullAppName;
 			res = ADB.execShellCommandRaw(info.serial, cmd, socket.getOutputStream(), socket.getInputStream());
+			if (res == null) {
+				return -1;
+			}
 		}
 		String rst = new String(res).trim();
 		if (rst.startsWith("Starting: Intent {") && rst.endsWith(fullAppName + " }")) {
@@ -144,7 +148,7 @@ public class ADBDevice {
 				for (String line : lines) {
 					line = line.trim();
 					if (!line.isEmpty()) {
-						props.add(line.trim());
+						props.add(line);
 					}
 				}
 			}
@@ -169,10 +173,16 @@ public class ADBDevice {
 			if (payload != null) {
 				String ps = new String(payload);
 				String[] psLines = ps.split("\n");
-				for (int i = index; i < psLines.length; i++) {
-					Process proc = Process.make(psLines[i]);
+				for (String line : psLines) {
+					line = line.trim();
+					if (line.isEmpty()) {
+						continue;
+					}
+					Process proc = Process.make(line);
 					if (proc != null) {
 						procs.add(proc);
+					} else {
+						LOG.error("Unexpected process info data received: \"{}\"", LogUtils.escape(line));
 					}
 				}
 			}

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

@@ -125,9 +125,12 @@ public class JadxProject {
 				.map(TabStateViewAdapter::build)
 				.filter(Objects::nonNull)
 				.collect(Collectors.toList());
-		data.setOpenTabs(tabStateList);
-		data.setActiveTab(activeTab);
-		changed();
+		boolean dataChanged;
+		dataChanged = data.setOpenTabs(tabStateList);
+		dataChanged |= data.setActiveTab(activeTab);
+		if (dataChanged) {
+			changed();
+		}
 	}
 
 	public List<EditorViewState> getOpenTabs(MainWindow mw) {

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

@@ -123,10 +123,10 @@ public class JadxSettingsWindow extends JDialog {
 		leftPanel.add(makeAppearanceGroup());
 		leftPanel.add(makeOtherGroup());
 		leftPanel.add(makeSearchResGroup());
-		leftPanel.add(makePluginOptionsGroup());
 		leftPanel.add(Box.createVerticalGlue());
 
 		rightPanel.add(makeDecompilationGroup());
+		rightPanel.add(makePluginOptionsGroup());
 		rightPanel.add(Box.createVerticalGlue());
 
 		JButton saveBtn = new JButton(NLS.str("preferences.save"));

jadx-gui/src/main/java/jadx/gui/settings/data/ProjectData.java

@@ -14,7 +14,7 @@ public class ProjectData {
 	private List<String[]> treeExpansions = new ArrayList<>();
 	private JadxCodeData codeData = new JadxCodeData();
 	private List<TabViewState> openTabs = Collections.emptyList();
-	private int activeTab;
+	private int activeTab = -1;
 
 	public List<Path> getFiles() {
 		return files;
@@ -52,15 +52,33 @@ public class ProjectData {
 		return openTabs;
 	}
 
-	public void setOpenTabs(List<TabViewState> openTabs) {
+	/**
+	 *
+	 * @param openTabs
+	 * @return <code>true></code> if a change was saved
+	 */
+	public boolean setOpenTabs(List<TabViewState> openTabs) {
+		if (this.openTabs.equals(openTabs)) {
+			return false;
+		}
 		this.openTabs = openTabs;
+		return true;
 	}
 
 	public int getActiveTab() {
 		return activeTab;
 	}
 
-	public void setActiveTab(int activeTab) {
+	/**
+	 *
+	 * @param activeTab
+	 * @return <code>true></code> if a change was saved
+	 */
+	public boolean setActiveTab(int activeTab) {
+		if (this.activeTab == activeTab) {
+			return false;
+		}
 		this.activeTab = activeTab;
+		return true;
 	}
 }

jadx-gui/src/main/java/jadx/gui/ui/dialog/ADBDialog.java

@@ -256,8 +256,8 @@ public class ADBDialog extends JDialog implements ADB.DeviceStateListener, ADB.J
 	private void connectToADB() {
 		String tip;
 		try {
-			String host = hostTextField.getText();
-			String port = portTextField.getText();
+			String host = hostTextField.getText().trim();
+			String port = portTextField.getText().trim();
 			tipLabel.setText(NLS.str("adb_dialog.connecting", host, port));
 			deviceSocket = ADB.listenForDeviceState(this, host, Integer.parseInt(port));
 			if (deviceSocket != null) {
@@ -592,7 +592,7 @@ public class ADBDialog extends JDialog implements ADB.DeviceStateListener, ADB.J
 			ver = "8";
 		}
 		ver = getMajorVer(ver);
-		debugSetter.set(device, ver, pid, name);
+		debugSetter.set(device, Integer.parseInt(ver), pid, name);
 		return true;
 	}
 
@@ -606,7 +606,7 @@ public class ADBDialog extends JDialog implements ADB.DeviceStateListener, ADB.J
 
 	private class DebugSetting {
 		private static final int FORWARD_TCP_PORT = 33233;
-		private String ver;
+		private int ver;
 		private String pid;
 		private String name;
 		private ADBDevice device;
@@ -614,7 +614,7 @@ public class ADBDialog extends JDialog implements ADB.DeviceStateListener, ADB.J
 		private String expectPkg = "";
 		private boolean autoAttachPkg = false;
 
-		private void set(ADBDevice device, String ver, String pid, String name) {
+		private void set(ADBDevice device, int ver, String pid, String name) {
 			this.ver = ver;
 			this.pid = pid;
 			this.name = name;

jadx-gui/src/main/java/jadx/gui/ui/panel/IDebugController.java

@@ -4,7 +4,7 @@ import jadx.core.dex.instructions.args.ArgType;
 import jadx.gui.ui.panel.JDebuggerPanel.ValueTreeNode;
 
 public interface IDebugController {
-	boolean startDebugger(JDebuggerPanel panel, String[] args);
+	boolean startDebugger(JDebuggerPanel debuggerPanel, String adbHost, int adbPort, int androidVer);
 
 	boolean run();
 

jadx-gui/src/main/java/jadx/gui/ui/panel/JDebuggerPanel.java

@@ -373,8 +373,8 @@ public class JDebuggerPanel extends JPanel {
 		}
 	}
 
-	public boolean showDebugger(String procName, String host, int port, String androidVer) {
-		boolean ok = controller.startDebugger(this, new String[] { host, String.valueOf(port), androidVer });
+	public boolean showDebugger(String procName, String host, int port, int androidVer) {
+		boolean ok = controller.startDebugger(this, host, port, androidVer);
 		if (ok) {
 			log(String.format("Attached %s %s:%d", procName, host, port));
 			leftSplitter.setDividerLocation(mainWindow.getSettings().getDebuggerStackFrameSplitterLoc());

jadx-gui/src/main/java/jadx/gui/utils/IOUtils.java

@@ -3,16 +3,14 @@ package jadx.gui.utils;
 import java.io.IOException;
 import java.io.InputStream;
 
+import org.jetbrains.annotations.Nullable;
+
 public class IOUtils {
 
 	/**
 	 * This method can be deleted once Jadx is Java11+
-	 *
-	 * @param inputStream
-	 * @param len
-	 * @return
-	 * @throws IOException
 	 */
+	@Nullable
 	public static byte[] readNBytes(InputStream inputStream, int len) throws IOException {
 		byte[] payload = new byte[len];
 		int readSize = 0;

jadx-plugins/jadx-plugins-api/src/main/java/jadx/api/plugins/JadxPluginManager.java

@@ -101,6 +101,9 @@ public class JadxPluginManager {
 	}
 
 	public List<JadxPlugin> getAllPlugins() {
+		if (allPlugins.isEmpty()) {
+			load();
+		}
 		return allPlugins.stream().map(PluginData::getPlugin).collect(Collectors.toList());
 	}