More MagiskHide optimizations

- Use a general procfs traversal function with callbacks
- Much better functions for killing processes

native/jni/magiskhide/hide_utils.cpp

@@ -20,7 +20,7 @@ using namespace std;
 // Protect access to both hide_list and hide_uid
 pthread_mutex_t list_lock;
 vector<string> hide_list;
-set<uid_t> hide_uid;
+set<int> hide_uid;
 
 // Treat GMS separately as we're only interested in one component
 int gms_uid = -1;
@@ -37,11 +37,6 @@ static const char *prop_value[] =
 	  "enforcing", "0", "0", "0",
 	  "1", "user", "release-keys", "0", nullptr };
 
-struct ps_arg {
-	const char *name;
-	uid_t uid;
-};
-
 void manage_selinux() {
 	char val;
 	int fd = xopen(SELINUX_ENFORCE, O_RDONLY);
@@ -54,7 +49,7 @@ void manage_selinux() {
 	}
 }
 
-void hide_sensitive_props() {
+static void hide_sensitive_props() {
 	LOGI("hide_utils: Hiding sensitive props\n");
 
 	// Hide all sensitive props
@@ -65,27 +60,32 @@ void hide_sensitive_props() {
 	}
 }
 
-static bool is_digits(const char *s) {
+/*
-	for (const char *c = s; *c; ++c) {
+ * Bionic's atoi runs through strtol().
-		if (*c < '0' || *c > '9')
+ * Use our own implementation for faster conversion.
-			return false;
+ */
+static inline int parse_int(const char *s) {
+	int val = 0;
+	char c;
+	while ((c = *(s++))) {
+		if (c > '9' || c < '0')
+			return -1;
+		val = val * 10 + c - '0';
 	}
-	return true;
+	return val;
 }
 
-static void ps(void (*cb)(int, void*), void *arg) {
+// Leave /proc fd opened as we're going to read from it repeatedly
-	DIR *dir;
+static DIR *procfp;
-	struct dirent *entry;
+void crawl_procfs(const function<bool (int)> &fn) {
-
+	struct dirent *dp;
-	if (!(dir = xopendir("/proc")))
+	int pid;
-		return;
+	rewinddir(procfp);
-
+	while ((dp = readdir(procfp))) {
-	while ((entry = xreaddir(dir))) {
+		pid = parse_int(dp->d_name);
-		if (entry->d_type == DT_DIR && is_digits(entry->d_name))
+		if (pid > 0 && !fn(pid))
-			cb(atoi(entry->d_name), arg);
+			break;
 	}
-
-	closedir(dir);
 }
 
 static bool proc_name_match(int pid, const char *name) {
@@ -94,24 +94,24 @@ static bool proc_name_match(int pid, const char *name) {
 	sprintf(buf, "/proc/%d/comm", pid);
 	if ((f = fopen(buf, "re"))) {
 		fgets(buf, sizeof(buf), f);
+		fclose(f);
 		if (strcmp(buf, name) == 0)
 			return true;
 	} else {
 		// The PID is already killed
 		return false;
 	}
-	fclose(f);
 
 	sprintf(buf, "/proc/%d/cmdline", pid);
 	if ((f = fopen(buf, "re"))) {
 		fgets(buf, sizeof(buf), f);
+		fclose(f);
 		if (strcmp(basename(buf), name) == 0)
 			return true;
 	} else {
 		// The PID is already killed
 		return false;
 	}
-	fclose(f);
 
 	sprintf(buf, "/proc/%d/exe", pid);
 	ssize_t len;
@@ -121,52 +121,38 @@ static bool proc_name_match(int pid, const char *name) {
 	return strcmp(basename(buf), name) == 0;
 }
 
-static void kill_proc_cb(int pid, void *v) {
-	auto args = static_cast<ps_arg *>(v);
-	if (proc_name_match(pid, args->name))
-		kill(pid, SIGTERM);
-	else if (args->uid > 0) {
-		char buf[64];
-		struct stat st;
-		sprintf(buf, "/proc/%d", pid);
-		stat(buf, &st);
-		if (args->uid == st.st_uid)
-			kill(pid, SIGTERM);
-	}
-
-}
-
 static void kill_process(const char *name) {
-	ps_arg args;
+	// We do NOT want to kill GMS itself
-	char *slash = nullptr;
+	if (strcmp(name, SAFETYNET_PKG) == 0)
-	if (strcmp(name, SAFETYNET_COMPONENT) == 0) {
+		name = SAFETYNET_PROCESS;
-		// We do NOT want to kill gms, it will cause massive system crashes
+	crawl_procfs([=](int pid) -> bool {
-		args.name = SAFETYNET_PROCESS;
+		if (proc_name_match(pid, name)) {
-	} else {
+			if (kill(pid, SIGTERM) == 0)
-		// Only leave the package name part of component name temporarily
+				LOGD("hide_utils: killed PID=[%d] (%s)\n", pid, name);
-		slash = strchr((char *)name, '/');
+			return false;
-		if (slash)
-			*slash = '\0';
-		args.name = name;
 		}
-	struct stat st;
+		return true;
-	int fd = xopen("/data/data", O_RDONLY | O_CLOEXEC);
+	});
-	if (fstatat(fd, args.name, &st, 0) == 0)
-		args.uid = st.st_uid;
-	else
-		args.uid = 0;
-	close(fd);
-	ps(kill_proc_cb, &args);
-	// Revert back to component name
-	if (slash)
-		*slash = '/';
 }
 
-static int add_pkg_uid(const char *proc) {
+static void kill_process(int uid) {
+	// We do NOT want to kill all GMS processes
+	if (uid == gms_uid) {
+		kill_process(SAFETYNET_PROCESS);
+		return;
+	}
+	crawl_procfs([=](int pid) -> bool {
+		if (get_uid(pid) == uid && kill(pid, SIGTERM) == 0)
+			LOGD("hide_utils: killed PID=[%d]\n", pid);
+		return true;
+	});
+}
+
+static int add_pkg_uid(const char *pkg) {
 	char path[4096];
 	struct stat st;
 	const char *data = SDK_INT >= 24 ? "/data/user_de/0" : "/data/data";
-	sprintf(path, "%s/%s", data, proc);
+	sprintf(path, "%s/%s", data, pkg);
 	if (xstat(path, &st) == 0) {
 		hide_uid.insert(st.st_uid);
 		return st.st_uid;
@@ -181,53 +167,52 @@ void refresh_uid() {
 }
 
 void clean_magisk_props() {
-	LOGD("hide_utils: Cleaning magisk props\n");
 	getprop([](const char *name, auto, auto) -> void {
 		if (strstr(name, "magisk"))
 			deleteprop(name);
 	}, nullptr, false);
 }
 
-int add_list(const char *proc) {
+int add_list(const char *pkg) {
 	for (auto &s : hide_list) {
-		if (s == proc)
+		if (s == pkg)
 			return HIDE_ITEM_EXIST;
 	}
 
 	// Add to database
 	char sql[4096];
-	snprintf(sql, sizeof(sql), "INSERT INTO hidelist (process) VALUES('%s')", proc);
+	snprintf(sql, sizeof(sql), "INSERT INTO hidelist (process) VALUES('%s')", pkg);
 	char *err = db_exec(sql);
 	db_err_cmd(err, return DAEMON_ERROR);
 
-	LOGI("hide_list add: [%s]\n", proc);
+	LOGI("hide_list add: [%s]\n", pkg);
 
 	// Critical region
 	pthread_mutex_lock(&list_lock);
-	hide_list.emplace_back(proc);
+	hide_list.emplace_back(pkg);
-	add_pkg_uid(proc);
+	int uid = add_pkg_uid(pkg);
 	pthread_mutex_unlock(&list_lock);
 
-	kill_process(proc);
+	kill_process(uid);
 	return DAEMON_SUCCESS;
 }
 
 int add_list(int client) {
-	char *proc = read_string(client);
+	char *pkg = read_string(client);
-	int ret = add_list(proc);
+	int ret = add_list(pkg);
-	free(proc);
+	free(pkg);
 	update_inotify_mask();
 	return ret;
 }
 
-static int rm_list(const char *proc) {
+static int rm_list(const char *pkg) {
 	// Critical region
 	bool remove = false;
 	pthread_mutex_lock(&list_lock);
 	for (auto it = hide_list.begin(); it != hide_list.end(); ++it) {
-		if (*it == proc) {
+		if (*it == pkg) {
 			remove = true;
-			LOGI("hide_list rm: [%s]\n", proc);
+			LOGI("hide_list rm: [%s]\n", pkg);
 			hide_list.erase(it);
 			break;
 		}
@@ -238,7 +223,7 @@ static int rm_list(const char *proc) {
 
 	if (remove) {
 		char sql[4096];
-		snprintf(sql, sizeof(sql), "DELETE FROM hidelist WHERE process='%s'", proc);
+		snprintf(sql, sizeof(sql), "DELETE FROM hidelist WHERE process='%s'", pkg);
 		char *err = db_exec(sql);
 		db_err(err);
 		return DAEMON_SUCCESS;
@@ -248,14 +233,14 @@ static int rm_list(const char *proc) {
 }
 
 int rm_list(int client) {
-	char *proc = read_string(client);
+	char *pkg = read_string(client);
-	int ret = rm_list(proc);
+	int ret = rm_list(pkg);
-	free(proc);
+	free(pkg);
 	update_inotify_mask();
 	return ret;
 }
 
-int init_list(void *, int, char **data, char**) {
+static int init_list(void *, int, char **data, char**) {
 	LOGI("hide_list init: [%s]\n", *data);
 	hide_list.emplace_back(*data);
 	kill_process(*data);
@@ -265,8 +250,8 @@ int init_list(void *, int, char **data, char**) {
 	return 0;
 }
 
-void init_list(const char *proc) {
+static void init_list(const char *pkg) {
-	init_list(nullptr, 0, (char **) &proc, nullptr);
+	init_list(nullptr, 0, (char **) &pkg, nullptr);
 }
 
 #define LEGACY_LIST MODULEROOT "/.core/hidelist"
@@ -311,17 +296,36 @@ static void set_hide_config() {
 	db_err(err);
 }
 
-int launch_magiskhide(int client) {
+static inline void launch_err(int client, int code = DAEMON_ERROR) {
+	if (code == DAEMON_ERROR)
+		hide_enabled = false;
+	if (client >= 0) {
+		write_int(client, code);
+		close(client);
+	}
+	pthread_exit(nullptr);
+}
+
+#define LAUNCH_ERR launch_err(client)
+
+void launch_magiskhide(int client) {
 	if (SDK_INT < 19)
-		goto error;
+		LAUNCH_ERR;
 
 	if (hide_enabled)
-		return HIDE_IS_ENABLED;
+		launch_err(client, HIDE_IS_ENABLED);
 
 	hide_enabled = true;
 	set_hide_config();
 	LOGI("* Starting MagiskHide\n");
 
+	if (procfp == nullptr) {
+		int fd = xopen("/proc", O_RDONLY | O_CLOEXEC);
+		if (fd < 0)
+			LAUNCH_ERR;
+		procfp = fdopendir(fd);
+	}
+
 	hide_sensitive_props();
 
 	// Initialize the mutex lock
@@ -329,20 +333,20 @@ int launch_magiskhide(int client) {
 
 	// Initialize the hide list
 	if (!init_list())
-		goto error;
+		LAUNCH_ERR;
 
 	// Get thread reference
 	proc_monitor_thread = pthread_self();
 	if (client >= 0) {
 		write_int(client, DAEMON_SUCCESS);
 		close(client);
+		client = -1;
 	}
 	// Start monitoring
 	proc_monitor();
 
-error:
+	// proc_monitor should not return
-	hide_enabled = false;
+	LAUNCH_ERR;
-	return DAEMON_ERROR;
 }
 
 int stop_magiskhide() {

native/jni/magiskhide/magiskhide.cpp

@@ -52,8 +52,8 @@ void magiskhide_handler(int client) {
 
 	switch (req) {
 	case LAUNCH_MAGISKHIDE:
-		res = launch_magiskhide(client);
+		launch_magiskhide(client);
-		break;
+		return;
 	case STOP_MAGISKHIDE:
 		res = stop_magiskhide();
 		break;

native/jni/magiskhide/magiskhide.h

@@ -2,9 +2,13 @@
 #define MAGISK_HIDE_H
 
 #include <pthread.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <unistd.h>
 #include <vector>
 #include <string>
 #include <set>
+#include <functional>
 
 #include "daemon.h"
 
@@ -15,7 +19,7 @@
 #define SAFETYNET_PKG        "com.google.android.gms"
 
 // Daemon entries
-int launch_magiskhide(int client);
+void launch_magiskhide(int client);
 int stop_magiskhide();
 int add_list(int client);
 int rm_list(int client);
@@ -29,14 +33,26 @@ void proc_monitor();
 
 // Utility functions
 void manage_selinux();
-void hide_sensitive_props();
 void clean_magisk_props();
 void refresh_uid();
+void crawl_procfs(const std::function<bool (int)> &fn);
+
+static inline int get_uid(const int pid) {
+	char path[16];
+	struct stat st;
+
+	sprintf(path, "/proc/%d", pid);
+	if (stat(path, &st) == -1)
+		return -1;
+
+	// We don't care about multiuser
+	return st.st_uid % 100000;
+}
 
 extern bool hide_enabled;
 extern pthread_mutex_t list_lock;
 extern std::vector<std::string> hide_list;
-extern std::set<uid_t> hide_uid;
+extern std::set<int> hide_uid;
 extern int gms_uid;
 
 enum {

native/jni/magiskhide/proc_monitor.cpp

@@ -11,7 +11,6 @@
 #include <stdio.h>
 #include <string.h>
 #include <unistd.h>
-#include <ctype.h>
 #include <fcntl.h>
 #include <signal.h>
 #include <pthread.h>
@@ -21,7 +20,7 @@
 #include <sys/mount.h>
 #include <vector>
 #include <string>
-#include <unordered_map>
+#include <map>
 
 #include <magisk.h>
 #include <utils.h>
@@ -62,6 +61,9 @@ static inline void lazy_unmount(const char* mountpoint) {
 		LOGD("hide_daemon: Unmounted (%s)\n", mountpoint);
 }
 
+/* APK monitoring doesn't seem to require checking namespace
+ * separation from PPID. Preserve this function just in case */
+#if 0
 static inline int parse_ppid(const int pid) {
 	char path[32];
 	int ppid;
@@ -77,17 +79,7 @@ static inline int parse_ppid(const int pid) {
 
 	return ppid;
 }
-
+#endif
-static inline uid_t get_uid(const int pid) {
-	char path[16];
-	struct stat st;
-
-	sprintf(path, "/proc/%d", pid);
-	if (stat(path, &st) == -1)
-		return -1;
-
-	return st.st_uid;
-}
 
 static bool is_pid_safetynet_process(const int pid) {
 	char path[32];
@@ -109,7 +101,7 @@ static bool is_pid_safetynet_process(const int pid) {
 }
 
 static void hide_daemon(int pid) {
-	LOGD("hide_daemon: handling pid=[%d]\n", pid);
+	LOGD("hide_daemon: handling PID=[%d]\n", pid);
 
 	char buffer[4096];
 	vector<string> mounts;
@@ -151,80 +143,49 @@ exit:
 	_exit(0);
 }
 
-/*
+// A mapping from pid to namespace inode to avoid time-consuming GC
- * Bionic's atoi runs through strtol() and fault-tolerence checkings.
+static map<int, uint64_t> pid_ns_map;
- * Since we don't need it, use our own implementation of atoi()
- * for faster conversion.
- */
-static inline int fast_atoi(const char *str) {
-	int val = 0;
-
-	while (*str)
-		val = val * 10 + (*str++ - '0');
-
-	return val;
-}
-
-// Leave /proc fd opened as we're going to read from it repeatedly
-static DIR *dfd;
-// Use unordered map with pid and namespace inode number to avoid time-consuming GC
-static unordered_map<int, uint64_t> pid_ns_map;
-
-static void detect_new_processes() {
-	struct dirent *dp;
-	struct stat ns, pns;
-	int pid, ppid;
-	uid_t uid;
-
-	// Iterate through /proc and get a process that reads the target APK
-	rewinddir(dfd);
-	pthread_mutex_lock(&list_lock);
-	while ((dp = readdir(dfd))) {
-		if (!isdigit(dp->d_name[0]))
-			continue;
-
-		// dp->d_name is now the pid
-		pid = fast_atoi(dp->d_name);
 
+static bool process_pid(int pid) {
 	// We're only interested in PIDs > 1000
 	if (pid <= 1000)
-			continue;
+		return true;
 
-		uid = get_uid(pid) % 100000; // Handle multiuser
+	struct stat ns;
-		bool is_target = hide_uid.count(uid) != 0;
+	int uid = get_uid(pid);
-		if (is_target) {
+	if (hide_uid.count(uid)) {
-			// Make sure our target is alive
+		// Make sure we can read mount namespace
-			if ((ppid = parse_ppid(pid)) < 0 || read_ns(ppid, &pns) || read_ns(pid, &ns))
+		if (read_ns(pid, &ns))
-				continue;
+			return true;
 
 		// Check if it's a process we haven't already hijacked
 		auto pos = pid_ns_map.find(pid);
-			if (pos == pid_ns_map.end() || pos->second != ns.st_ino) {
+		if (pos != pid_ns_map.end() && pos->second == ns.st_ino)
-				pid_ns_map[pid] = ns.st_ino;
+			return true;
+
 		if (uid == gms_uid) {
 			// Check /proc/uid/cmdline to see if it's SAFETYNET_PROCESS
 			if (!is_pid_safetynet_process(pid))
-						continue;
+				return true;
 
-					LOGI("proc_monitor: found %s\n", SAFETYNET_PROCESS);
+			LOGD("proc_monitor: " SAFETYNET_PROCESS "\n");
 		}
 
 		// Send pause signal ASAP
 		if (kill(pid, SIGSTOP) == -1)
-					continue;
+			return true;
+
+		pid_ns_map[pid] = ns.st_ino;
+		LOGI("proc_monitor: UID=[%d] PID=[%d] ns=[%llu]\n", uid, pid, ns.st_ino);
 
 		/*
 		 * The setns system call do not support multithread processes
 		 * We have to fork a new process, setns, then do the unmounts
 		 */
-				LOGI("proc_monitor: UID=[%ju] PID=[%d] ns=[%llu]\n",
-					 (uintmax_t)uid, pid, ns.st_ino);
 		if (fork_dont_care() == 0)
 			hide_daemon(pid);
 	}
-		}
+	return true;
-	}
-	pthread_mutex_unlock(&list_lock);
 }
 
 static void listdir_apk(const char *name) {
@@ -310,14 +271,6 @@ void proc_monitor() {
 		term_thread(TERM_THREAD);
 	}
 
-	if ((dfd = opendir("/proc")) == NULL) {
-		LOGE("proc_monitor: Unable to open /proc\n");
-		term_thread(TERM_THREAD);
-	}
-
-	// Detect existing processes for the first time
-	detect_new_processes();
-
 	// Read inotify events
 	struct inotify_event *event;
 	ssize_t len;
@@ -337,8 +290,9 @@ void proc_monitor() {
 			if (event->mask & IN_OPEN) {
 				// Since we're just watching files,
 				// extracting file name is not possible from querying event
-				// LOGI("proc_monitor: inotify: APK opened\n");
+				pthread_mutex_lock(&list_lock);
-				detect_new_processes();
+				crawl_procfs(process_pid);
+				pthread_mutex_unlock(&list_lock);
 			} else {
 				LOGI("proc_monitor: inotify: /data/app change detected\n");
 				pthread_mutex_lock(&list_lock);