Prevent Magisk database race condition

The database should only be accessed by a single process, which is magiskd.
This means 'magisk --sqlite [SQL]' has to be updated to pass the SQL command to the daemon.
In addition, open the database connection with SQLITE_OPEN_FULLMUTEX to support multithread in magiskd.

native/jni/daemon/bootstages.cpp

@@ -884,16 +884,12 @@ core_only:
 		install_apk("/data/magisk.apk");
 	} else {
 		// Check whether we have a valid manager installed
-		sqlite3 *db = get_magiskdb();
+		db_strings str;
-		if (db) {
+		get_db_strings(&str, SU_MANAGER);
-			db_strings str;
+		if (validate_manager(str[SU_MANAGER], 0, nullptr)) {
-			get_db_strings(db, &str, SU_MANAGER);
+			// There is no manager installed, install the stub
-			if (validate_manager(str[SU_MANAGER], 0, nullptr)) {
+			exec_command_sync("/sbin/magiskinit", "-x", "manager", "/data/magisk.apk", nullptr);
-				// There is no manager installed, install the stub
+			install_apk("/data/magisk.apk");
-				exec_command_sync("/sbin/magiskinit", "-x", "manager", "/data/magisk.apk", nullptr);
-				install_apk("/data/magisk.apk");
-			}
-			sqlite3_close_v2(db);
 		}
 	}
 

native/jni/daemon/daemon.cpp

@@ -18,6 +18,7 @@
 #include "utils.h"
 #include "daemon.h"
 #include "selinux.h"
+#include "db.h"
 #include "flags.h"
 
 static void get_client_cred(int fd, struct ucred *cred) {
@@ -39,6 +40,7 @@ static void *request_handler(void *args) {
 	case POST_FS_DATA:
 	case LATE_START:
 	case BOOT_COMPLETE:
+	case SQLITE_CMD:
 		if (credential.uid != 0) {
 			write_int(client, ROOT_REQUIRED);
 			close(client);
@@ -75,6 +77,10 @@ static void *request_handler(void *args) {
 	case HANDSHAKE:
 		/* Do NOT close the client, make it hold */
 		break;
+	case SQLITE_CMD:
+		exec_sql(client);
+		close(client);
+		break;
 	default:
 		close(client);
 		break;

native/jni/daemon/db.cpp

@@ -7,9 +7,12 @@
 
 #include "magisk.h"
 #include "db.h"
+#include "daemon.h"
 
 #define DB_VERSION 7
 
+static sqlite3 *mDB = nullptr;
+
 db_strings::db_strings() {
 	memset(data, 0, sizeof(data));
 }
@@ -76,24 +79,17 @@ static int ver_cb(void *ver, int, char **data, char **) {
 	return 0;
 }
 
-#define err_abort(err) \
+#define err_ret(e) if (e) return e;
-if (err) { \
-	LOGE("sqlite3_exec: %s\n", err); \
-	sqlite3_free(err); \
-	return nullptr; \
-}
 
-static sqlite3 *open_and_init_db() {
+static char *open_and_init_db(sqlite3 *&db) {
-	sqlite3 *db;
+	int ret = sqlite3_open_v2(MAGISKDB, &db,
-	int ret = sqlite3_open(MAGISKDB, &db);
+			SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE | SQLITE_OPEN_FULLMUTEX, nullptr);
-	if (ret) {
+	if (ret)
-		LOGE("sqlite3 open failure: %s\n", sqlite3_errstr(ret));
+		return strdup(sqlite3_errstr(ret));
-		return nullptr;
-	}
 	int ver, upgrade = 0;
 	char *err;
 	sqlite3_exec(db, "PRAGMA user_version", ver_cb, &ver, &err);
-	err_abort(err);
+	err_ret(err);
 	if (ver > DB_VERSION) {
 		// Don't support downgrading database
 		sqlite3_close_v2(db);
@@ -106,20 +102,20 @@ static sqlite3 *open_and_init_db() {
 					 "(uid INT, package_name TEXT, policy INT, until INT, "
 					 "logging INT, notification INT, PRIMARY KEY(uid))",
 					 nullptr, nullptr, &err);
-		err_abort(err);
+		err_ret(err);
 		// Logs
 		sqlite3_exec(db,
 					 "CREATE TABLE IF NOT EXISTS logs "
 					 "(from_uid INT, package_name TEXT, app_name TEXT, from_pid INT, "
 					 "to_uid INT, action INT, time INT, command TEXT)",
 					 nullptr, nullptr, &err);
-		err_abort(err);
+		err_ret(err);
 		// Settings
 		sqlite3_exec(db,
 					 "CREATE TABLE IF NOT EXISTS settings "
 					 "(key TEXT, value INT, PRIMARY KEY(key))",
 					 nullptr, nullptr, &err);
-		err_abort(err);
+		err_ret(err);
 		ver = 3;
 		upgrade = 1;
 	}
@@ -129,13 +125,13 @@ static sqlite3 *open_and_init_db() {
 					 "CREATE TABLE IF NOT EXISTS strings "
 					 "(key TEXT, value TEXT, PRIMARY KEY(key))",
 					 nullptr, nullptr, &err);
-		err_abort(err);
+		err_ret(err);
 		ver = 4;
 		upgrade = 1;
 	}
 	if (ver == 4) {
 		sqlite3_exec(db, "UPDATE policies SET uid=uid%100000", nullptr, nullptr, &err);
-		err_abort(err);
+		err_ret(err);
 		/* Skip version 5 */
 		ver = 6;
 		upgrade = 1;
@@ -146,7 +142,7 @@ static sqlite3 *open_and_init_db() {
 					 "CREATE TABLE IF NOT EXISTS hidelist "
 					 "(process TEXT, PRIMARY KEY(process))",
 					 nullptr, nullptr, &err);
-		err_abort(err);
+		err_ret(err);
 		ver = 7;
 		upgrade =1 ;
 	}
@@ -156,19 +152,27 @@ static sqlite3 *open_and_init_db() {
 		char query[32];
 		sprintf(query, "PRAGMA user_version=%d", ver);
 		sqlite3_exec(db, query, nullptr, nullptr, &err);
-		err_abort(err);
+		err_ret(err);
 	}
-	return db;
+	return nullptr;
 }
 
-sqlite3 *get_magiskdb() {
+char *db_exec(const char *sql, int (*cb)(void *, int, char**, char**), void *v) {
-	sqlite3 *db = open_and_init_db();
+	char *err;
-	if (db == nullptr) {
+	if (mDB == nullptr) {
-		// Open fails, remove and reconstruct
+		err = open_and_init_db(mDB);
-		unlink(MAGISKDB);
+		db_err_cmd(err,
-		db = open_and_init_db();
+			// Open fails, remove and reconstruct
+			unlink(MAGISKDB);
+			err = open_and_init_db(mDB);
+			err_ret(err);
+		);
 	}
-	return db;
+	if (mDB) {
+		sqlite3_exec(mDB, sql, cb, v, &err);
+		return err;
+	}
+	return nullptr;
 }
 
 static int settings_cb(void *v, int col_num, char **data, char **col_name) {
@@ -189,22 +193,16 @@ static int settings_cb(void *v, int col_num, char **data, char **col_name) {
 	return 0;
 }
 
-int get_db_settings(sqlite3 *db, db_settings *dbs, int key) {
+int get_db_settings(db_settings *dbs, int key) {
-	if (db == nullptr)
-		return 1;
 	char *err;
 	if (key > 0) {
 		char query[128];
 		sprintf(query, "SELECT key, value FROM settings WHERE key='%s'", DB_SETTING_KEYS[key]);
-		sqlite3_exec(db, query, settings_cb, dbs, &err);
+		err = db_exec(query, settings_cb, dbs);
 	} else {
-		sqlite3_exec(db, "SELECT key, value FROM settings", settings_cb, dbs, &err);
+		err = db_exec("SELECT key, value FROM settings", settings_cb, dbs);
-	}
-	if (err) {
-		LOGE("sqlite3_exec: %s\n", err);
-		sqlite3_free(err);
-		return 1;
 	}
+	db_err_cmd(err, return 1);
 	return 0;
 }
 
@@ -225,16 +223,14 @@ static int strings_cb(void *v, int col_num, char **data, char **col_name) {
 	return 0;
 }
 
-int get_db_strings(sqlite3 *db, db_strings *str, int key) {
+int get_db_strings(db_strings *str, int key) {
-	if (db == nullptr)
-		return 1;
 	char *err;
 	if (key > 0) {
 		char query[128];
 		sprintf(query, "SELECT key, value FROM strings WHERE key='%s'", DB_STRING_KEYS[key]);
-		sqlite3_exec(db, query, strings_cb, str, &err);
+		err = db_exec(query, strings_cb, str);
 	} else {
-		sqlite3_exec(db, "SELECT key, value FROM strings", strings_cb, str, &err);
+		err = db_exec("SELECT key, value FROM strings", strings_cb, str);
 	}
 	if (err) {
 		LOGE("sqlite3_exec: %s\n", err);
@@ -258,18 +254,12 @@ static int policy_cb(void *v, int col_num, char **data, char **col_name) {
 	return 0;
 }
 
-int get_uid_policy(sqlite3 *db, int uid, struct su_access *su) {
+int get_uid_policy(int uid, struct su_access *su) {
-	if (db == nullptr)
-		return 1;
 	char query[256], *err;
 	sprintf(query, "SELECT policy, logging, notification FROM policies "
 			"WHERE uid=%d AND (until=0 OR until>%li)", uid, time(nullptr));
-	sqlite3_exec(db, query, policy_cb, su, &err);
+	err = db_exec(query, policy_cb, su);
-	if (err) {
+	db_err_cmd(err, return 1);
-		LOGE("sqlite3_exec: %s\n", err);
-		sqlite3_free(err);
-		return 1;
-	}
 	return 0;
 }
 
@@ -299,26 +289,24 @@ int validate_manager(char *alt_pkg, int userid, struct stat *st) {
 }
 
 static int print_cb(void *v, int col_num, char **data, char **col_name) {
+	FILE *out = (FILE *) v;
 	for (int i = 0; i < col_num; ++i) {
-		if (i) printf("|");
+		if (i) fprintf(out, "|");
-		printf("%s=%s", col_name[i], data[i]);
+		fprintf(out, "%s=%s", col_name[i], data[i]);
 	}
-	printf("\n");
+	fprintf(out, "\n");
 	return 0;
 }
 
-int exec_sql(const char *sql) {
+void exec_sql(int client) {
-	sqlite3 *db = get_magiskdb();
+	char *sql = read_string(client);
-	if (db) {
+	FILE *out = fdopen(recv_fd(client), "a");
-		char *err;
+	char *err = db_exec(sql, print_cb, out);
-		sqlite3_exec(db, sql, print_cb, nullptr, &err);
+	free(sql);
-		sqlite3_close_v2(db);
+	fclose(out);
-		if (err) {
+	db_err_cmd(err,
-			fprintf(stderr, "sql_err: %s\n", err);
+		write_int(client, 1);
-			sqlite3_free(err);
+		return;
-			return 1;
+	);
-		}
+	write_int(client, 0);
-		return 0;
-	}
-	return 1;
 }

native/jni/daemon/magisk.cpp

@@ -113,7 +113,11 @@ int magisk_main(int argc, char *argv[]) {
 		write_int(fd, BOOT_COMPLETE);
 		return read_int(fd);
 	} else if (strcmp(argv[1], "--sqlite") == 0) {
-		return exec_sql(argv[2]);
+		int fd = connect_daemon();
+		write_int(fd, SQLITE_CMD);
+		write_string(fd, argv[2]);
+		send_fd(fd, STDOUT_FILENO);
+		return read_int(fd);
 	}
 
 	usage();

native/jni/include/daemon.h

@@ -20,7 +20,8 @@ enum {
 	BOOT_COMPLETE,
 	MAGISKHIDE,
 	HIDE_CONNECT,
-	HANDSHAKE
+	HANDSHAKE,
+	SQLITE_CMD,
 };
 
 // Return codes for daemon

native/jni/include/db.h

@@ -4,6 +4,13 @@
 #include <sqlite3.h>
 #include <sys/stat.h>
 
+#define db_err(e) db_err_cmd(e, )
+#define db_err_cmd(e, cmd) if (e) { \
+	LOGE("sqlite3_exec: %s\n", e); \
+	sqlite3_free(e); \
+	cmd;\
+}
+
 /***************
  * DB Settings *
  ***************/
@@ -128,11 +135,12 @@ struct su_access {
  * Public Functions *
  ********************/
 
-sqlite3 *get_magiskdb();
+sqlite3 *get_magiskdb(sqlite3 *&db);
-int get_db_settings(sqlite3 *db, db_settings *dbs, int key = -1);
+int get_db_settings(db_settings *dbs, int key);
-int get_db_strings(sqlite3 *db, db_strings *str, int key = -1);
+int get_db_strings(db_strings *str, int key);
-int get_uid_policy(sqlite3 *db, int uid, struct su_access *su);
+int get_uid_policy(int uid, struct su_access *su);
 int validate_manager(char *alt_pkg, int userid, struct stat *st);
-int exec_sql(const char *sql);
+void exec_sql(int client);
+char *db_exec(const char *sql, int (*cb)(void *, int, char**, char**) = nullptr, void *v = nullptr);
 
 #endif //DB_H

native/jni/magiskhide/hide_utils.cpp

@@ -140,7 +140,7 @@ void clean_magisk_props() {
 	}, nullptr, false);
 }
 
-static int add_list(sqlite3 *db, const char *proc) {
+int add_list(const char *proc) {
 	for (auto &s : hide_list) {
 		// They should be unique
 		if (s == proc)
@@ -149,30 +149,21 @@ static int add_list(sqlite3 *db, const char *proc) {
 
 	LOGI("hide_list add: [%s]\n", proc);
 
+	// Add to database
+	char sql[4096];
+	snprintf(sql, sizeof(sql), "INSERT INTO hidelist (process) VALUES('%s')", proc);
+	char *err = db_exec(sql);
+	db_err_cmd(err, return DAEMON_ERROR);
+
 	// Critical region
 	pthread_mutex_lock(&list_lock);
 	hide_list.push_back(proc);
 	kill_process(proc);
 	pthread_mutex_unlock(&list_lock);
 
-	// Add to database
-	char sql[4096];
-	snprintf(sql, sizeof(sql), "INSERT INTO hidelist (process) VALUES('%s')", proc);
-	sqlite3_exec(db, sql, nullptr, nullptr, nullptr);
-
 	return DAEMON_SUCCESS;
 }
 
-int add_list(const char *proc) {
-	sqlite3 *db = get_magiskdb();
-	if (db) {
-		int ret = add_list(db, proc);
-		sqlite3_close_v2(db);
-		return ret;
-	}
-	return DAEMON_ERROR;
-}
-
 int add_list(int client) {
 	char *proc = read_string(client);
 	int ret = add_list(proc);
@@ -196,13 +187,10 @@ static int rm_list(const char *proc) {
 	pthread_mutex_unlock(&list_lock);
 
 	if (do_rm) {
-		sqlite3 *db = get_magiskdb();
-		if (db == nullptr)
-			return DAEMON_ERROR;
 		char sql[4096];
 		snprintf(sql, sizeof(sql), "DELETE FROM hidelist WHERE process='%s'", proc);
-		sqlite3_exec(db, sql, nullptr, nullptr, nullptr);
+		char *err = db_exec(sql);
-		sqlite3_close_v2(db);
+		db_err(err);
 		return DAEMON_SUCCESS;
 	} else {
 		return HIDE_ITEM_NOT_EXIST;
@@ -218,31 +206,27 @@ int rm_list(int client) {
 
 #define LEGACY_LIST MOUNTPOINT "/.core/hidelist"
 
+static int collect_list(void *, int, char **data, char**) {
+	LOGI("hide_list: [%s]\n", data[0]);
+	hide_list.push_back(data[0]);
+	return 0;
+}
+
 bool init_list() {
 	LOGD("hide_list: initialize\n");
 
-	sqlite3 *db = get_magiskdb();
+	char *err = db_exec("SELECT process FROM hidelist", collect_list);
-	if (db == nullptr)
+	db_err_cmd(err, return false);
-		return false;
-
-	sqlite3_exec(db, "SELECT process FROM hidelist",
-				 [] (auto, auto, char **data, auto) -> int
-				 {
-				 	LOGI("hide_list: [%s]\n", data[0]);
-				 	hide_list.push_back(data[0]);
-				 	return 0;
-				 }, nullptr, nullptr);
 
 	// Migrate old hide list into database
 	if (access(LEGACY_LIST, R_OK) == 0) {
 		Vector<CharArray> tmp;
 		file_to_vector(LEGACY_LIST, tmp);
 		for (auto &s : tmp)
-			add_list(db, s);
+			add_list(s);
 		unlink(LEGACY_LIST);
 	}
 
-	sqlite3_close_v2(db);
 	return true;
 }
 
@@ -256,12 +240,11 @@ void ls_list(int client) {
 }
 
 static void set_hide_config() {
-	sqlite3 *db = get_magiskdb();
 	char sql[64];
 	sprintf(sql, "REPLACE INTO settings (key,value) VALUES('%s',%d)",
 			DB_SETTING_KEYS[HIDE_CONFIG], hide_enabled);
-	sqlite3_exec(db, sql, nullptr, nullptr, nullptr);
+	char *err = db_exec(sql);
-	sqlite3_close_v2(db);
+	db_err(err);
 }
 
 int launch_magiskhide(int client) {
@@ -314,10 +297,8 @@ int stop_magiskhide() {
 void auto_start_magiskhide() {
 	if (!start_log_daemon())
 		return;
-	sqlite3 *db = get_magiskdb();
 	db_settings dbs;
-	get_db_settings(db, &dbs, HIDE_CONFIG);
+	get_db_settings(&dbs, HIDE_CONFIG);
-	sqlite3_close_v2(db);
 	if (dbs[HIDE_CONFIG]) {
 		pthread_t thread;
 		xpthread_create(&thread, nullptr, [](void*) -> void* {

native/jni/su/su_daemon.cpp

@@ -60,32 +60,28 @@ static void *info_collector(void *node) {
 
 static void database_check(su_info *info) {
 	int uid = info->uid;
-	sqlite3 *db = get_magiskdb();
+	get_db_settings(&info->cfg, 0);
-	if (db) {
+	get_db_strings(&info->str, 0);
-		get_db_settings(db, &info->cfg);
-		get_db_strings(db, &info->str);
 
-		// Check multiuser settings
+	// Check multiuser settings
-		switch (info->cfg[SU_MULTIUSER_MODE]) {
+	switch (info->cfg[SU_MULTIUSER_MODE]) {
-			case MULTIUSER_MODE_OWNER_ONLY:
+		case MULTIUSER_MODE_OWNER_ONLY:
-				if (info->uid / 100000) {
+			if (info->uid / 100000) {
-					uid = -1;
+				uid = -1;
-					info->access = NO_SU_ACCESS;
+				info->access = NO_SU_ACCESS;
-				}
+			}
-				break;
+			break;
-			case MULTIUSER_MODE_OWNER_MANAGED:
+		case MULTIUSER_MODE_OWNER_MANAGED:
-				uid = info->uid % 100000;
+			uid = info->uid % 100000;
-				break;
+			break;
-			case MULTIUSER_MODE_USER:
+		case MULTIUSER_MODE_USER:
-			default:
+		default:
-				break;
+			break;
-		}
-
-		if (uid > 0)
-			get_uid_policy(db, uid, &info->access);
-		sqlite3_close_v2(db);
 	}
 
+	if (uid > 0)
+		get_uid_policy(uid, &info->access);
+
 	// We need to check our manager
 	if (info->access.log || info->access.notify)
 		validate_manager(info->str[SU_MANAGER], uid / 100000, &info->mgr_st);