improve: code robustness

This commit improves the robustness of the code by removing debug logs, fixing a memory leak, and adding missing error handling.

zygiskd/src/companion.c

@@ -42,12 +42,8 @@ void *entry_thread(void *arg) {
   int fd = args->fd;
   zygisk_companion_entry_func module_entry = args->entry;
 
-  LOGI("New companion thread (inside the thread!).\n - Client fd: %d\n", fd);
-
   module_entry(fd);
 
-  LOGI("Companion thread has been terminated.\n");
-
   close(fd);
   free(args);
 
@@ -63,7 +59,8 @@ void entry(int fd) {
   if (ret == -1) {
     LOGE("Failed to read module name\n");
 
-    write_uint8_t(fd, 2);
+    ret = write_uint8_t(fd, 2);
+    ASSURE_SIZE_WRITE("ZygiskdCompanion", "name", ret, sizeof(uint8_t));
 
     exit(0);
   }
@@ -74,7 +71,8 @@ void entry(int fd) {
   if (library_fd == -1) {
     LOGE("Failed to receive library fd\n");
 
-    write_uint8_t(fd, 2);
+    ret = write_uint8_t(fd, 2);
+    ASSURE_SIZE_WRITE("ZygiskdCompanion", "library_fd", ret, sizeof(uint8_t));
 
     exit(0);
   }
@@ -87,11 +85,13 @@ void entry(int fd) {
   if (module_entry == NULL) {
     LOGI("No companion module entry for module: %.*s\n", (int)ret, name);
 
-    write_uint8_t(fd, 0);
+    ret = write_uint8_t(fd, 0);
+    ASSURE_SIZE_WRITE("ZygiskdCompanion", "module_entry", ret, sizeof(uint8_t));
 
     exit(0);
   } else {
-    write_uint8_t(fd, 1);
+    ret = write_uint8_t(fd, 1);
+    ASSURE_SIZE_WRITE("ZygiskdCompanion", "module_entry", ret, sizeof(uint8_t));
   }
 
   while (1) {
@@ -102,19 +102,28 @@ void entry(int fd) {
 
       break;
     }
-
-    struct companion_module_thread_args *args = malloc(sizeof(struct companion_module_thread_args));
-    args->entry = module_entry;
   
-    if ((args->fd = read_fd(fd)) == -1) {
+    int fd = read_fd(fd);
+    if (fd == -1) {
       LOGE("Failed to receive client fd\n");
 
       exit(0);
     }
 
+    struct companion_module_thread_args *args = malloc(sizeof(struct companion_module_thread_args));
+    if (args == NULL) {
+      LOGE("Failed to allocate memory for thread args\n");
+
+      exit(0);
+    }
+
+    args->fd = fd;
+    args->entry = module_entry;
+
     LOGI("New companion request.\n - Module name: %.*s\n - Client fd: %d\n", (int)ret, name, args->fd);
 
-    write_uint8_t(args->fd, 1);
+    ret = write_uint8_t(args->fd, 1);
+    ASSURE_SIZE_WRITE("ZygiskdCompanion", "client_fd", ret, sizeof(uint8_t));
     
     pthread_t thread;
     pthread_create(&thread, NULL, entry_thread, args);

zygiskd/src/root_impl/apatch.c

@@ -76,7 +76,7 @@ bool _apatch_get_package_config(struct packages_config *restrict config) {
     return false;
   }
 
-  char line[1048 * 2];
+  char line[1048];
   /* INFO: Skip the CSV header */
   if (fgets(line, sizeof(line), fp) == NULL) {
     LOGE("Failed to read APatch's package_config header: %s\n", strerror(errno));
@@ -166,7 +166,9 @@ bool apatch_uid_should_umount(uid_t uid) {
 bool apatch_uid_is_manager(uid_t uid) {
   struct stat s;
   if (stat("/data/user_de/0/me.bmax.apatch", &s) == -1) {
-    if (errno != ENOENT) LOGE("Failed to stat APatch manager data directory: %s\n", strerror(errno));
+    if (errno != ENOENT) {
+      LOGE("Failed to stat APatch manager data directory: %s\n", strerror(errno));
+    }
     errno = 0;
 
     return false;

zygiskd/src/root_impl/kernelsu.c

@@ -53,7 +53,9 @@ bool ksu_uid_should_umount(uid_t uid) {
 bool ksu_uid_is_manager(uid_t uid) {
   struct stat s;
   if (stat("/data/user_de/0/me.weishu.kernelsu", &s) == -1) {
-    if (errno != ENOENT) LOGE("Failed to stat KSU manager data directory: %s\n", strerror(errno));
+    if (errno != ENOENT) {
+      LOGE("Failed to stat KSU manager data directory: %s\n", strerror(errno));
+    }
     errno = 0;
 
     return false;

zygiskd/src/root_impl/magisk.c

@@ -80,8 +80,6 @@ enum RootImplState magisk_get_existence(void) {
     return Abnormal;
   }
 
-  LOGI("Magisk info: %s\n", magisk_info);
-
   for (unsigned long i = 0; i < sizeof(supported_variants) / sizeof(supported_variants[0]); i++) {
     if (strstr(magisk_info, supported_variants[i])) variant = (enum magisk_variants)(i + 1);
   }
@@ -96,15 +94,11 @@ enum RootImplState magisk_get_existence(void) {
     return Abnormal;
   }
 
-  LOGI("Magisk version: %s\n", magisk_version);
-
   if (atoi(magisk_version) >= MIN_MAGISK_VERSION) return Supported;
   else return TooOld;
 }
 
 bool magisk_uid_granted_root(uid_t uid) {
-  LOGI("Checking if UID %d is granted root access\n", uid);
-
   char sqlite_cmd[256];
   snprintf(sqlite_cmd, sizeof(sqlite_cmd), "select 1 from policies where uid=%d and policy=2 limit 1", uid);
 
@@ -122,8 +116,6 @@ bool magisk_uid_granted_root(uid_t uid) {
 }
 
 bool magisk_uid_should_umount(uid_t uid) {
-  LOGI("Checking if UID %d should unmount\n", uid);
-
   struct dirent *entry;
   DIR *proc = opendir("/proc");
   if (!proc) {
@@ -197,8 +189,6 @@ bool magisk_uid_should_umount(uid_t uid) {
 }
 
 bool magisk_uid_is_manager(uid_t uid) {
-  LOGI("Checking if UID %d is a Magisk manager\n", uid);
-
   char sqlite_cmd[256];
   snprintf(sqlite_cmd, sizeof(sqlite_cmd), "select value from strings where key=\"requester\" limit 1");
 
@@ -212,33 +202,19 @@ bool magisk_uid_is_manager(uid_t uid) {
     return false;
   }
 
-  if (output[0] == '\0') {
+  char stat_path[PATH_MAX];
-    char stat_path[PATH_MAX];
+  if (output[0] == '\0')
     snprintf(stat_path, sizeof(stat_path), "/data/user_de/0/%s", magisk_managers[(int)variant]);
+  else
+    snprintf(stat_path, sizeof(stat_path), "/data/user_de/0/%s", output + strlen("value="));
 
-    struct stat s;
+  struct stat s;
-    if (stat(stat_path, &s) == -1) {
+  if (stat(stat_path, &s) == -1) {
-      LOGE("Failed to stat %s: %s\n", stat_path, strerror(errno));
+    LOGE("Failed to stat %s: %s\n", stat_path, strerror(errno));
-      errno = 0;
+    errno = 0;
 
-      return false;
+    return false;
-    }
-
-    return s.st_uid == uid;
-  } else {
-    char stat_path[PATH_MAX];
-    snprintf(stat_path, sizeof(stat_path), "/data/user_de/0/%s", output + strlen("value=")); /* BUG: ISSUE HERE, OUTPUT IS NOT VALID */
-
-    LOGI("Checking |%s|\n", stat_path);
-
-    struct stat s;
-    if (stat(stat_path, &s) == -1) {
-      LOGE("Failed to stat %s WHAT %s\n", stat_path, strerror(errno));
-      errno = 0;
-
-      return false;
-    }
-
-    return s.st_uid == uid;
   }
+
+  return s.st_uid == uid;
 }

zygiskd/src/zygiskd.c

@@ -101,21 +101,10 @@ static enum Architecture get_arch(void) {
 }
 
 int create_library_fd(const char *restrict so_path) {
-  /* INFO: This is required as older implementations of glibc may not
-             have the memfd_create function call, causing a crash. */
-  int memfd = syscall(SYS_memfd_create, "jit-cache-zygisk", MFD_ALLOW_SEALING);
-  if (memfd == -1) {
-    LOGE("Failed creating memfd: %s\n", strerror(errno));
-
-    return -1;
-  }
-
   int so_fd = open(so_path, O_RDONLY);
   if (so_fd == -1) {
     LOGE("Failed opening so file: %s\n", strerror(errno));
 
-    close(memfd);
-
     return -1;
   }
 
@@ -124,7 +113,6 @@ int create_library_fd(const char *restrict so_path) {
     LOGE("Failed getting so file size: %s\n", strerror(errno));
 
     close(so_fd);
-    close(memfd);
 
     return -1;
   }
@@ -133,7 +121,15 @@ int create_library_fd(const char *restrict so_path) {
     LOGE("Failed seeking so file: %s\n", strerror(errno));
 
     close(so_fd);
-    close(memfd);
+
+    return -1;
+  }
+
+  /* INFO: This is required as older implementations of glibc may not
+             have the memfd_create function call, causing a crash. */
+  int memfd = syscall(SYS_memfd_create, "jit-cache-zygisk", MFD_ALLOW_SEALING);
+  if (memfd == -1) {
+    LOGE("Failed creating memfd: %s\n", strerror(errno));
 
     return -1;
   }
@@ -163,7 +159,7 @@ int create_library_fd(const char *restrict so_path) {
 /* WARNING: Dynamic memory based */
 static void load_modules(enum Architecture arch, struct Context *restrict context) {
   context->len = 0;
-  context->modules = malloc(1);
+  context->modules = NULL;
 
   DIR *dir = opendir(PATH_MODULES_DIR);
   if (dir == NULL) {
@@ -212,7 +208,6 @@ static void load_modules(enum Architecture arch, struct Context *restrict contex
       errno = 0;
     } else continue;
 
-    LOGI("Loading module `%s`...\n", name);
     int lib_fd = create_library_fd(so_path);
     if (lib_fd == -1) {
       LOGE("Failed loading module `%s`\n", name);
@@ -220,9 +215,14 @@ static void load_modules(enum Architecture arch, struct Context *restrict contex
       continue;
     }
 
-    LOGI("Loaded module lib fd: %d\n", lib_fd);
 
     context->modules = realloc(context->modules, ((context->len + 1) * sizeof(struct Module)));
+    if (context->modules == NULL) {
+      LOGE("Failed reallocating memory for modules.\n");
+
+      return;
+    }
+
     context->modules[context->len].name = strdup(name);
     context->modules[context->len].lib_fd = lib_fd;
     context->modules[context->len].companion = -1;
@@ -239,13 +239,11 @@ static void free_modules(struct Context *restrict context) {
 
 static int create_daemon_socket(void) {
   set_socket_create_context("u:r:zygote:s0");
-  
+
   return unix_listener_from_path(PATH_CP_NAME);
 }
 
 static int spawn_companion(char *restrict argv[], char *restrict name, int lib_fd) {
-  LOGI("Spawning a new companion...\n");
-
   int sockets[2];
   if (socketpair(AF_UNIX, SOCK_STREAM, 0, sockets) == -1) {
     LOGE("Failed creating socket pair.\n");
@@ -296,8 +294,6 @@ static int spawn_companion(char *restrict argv[], char *restrict name, int lib_f
         return -1;
       }
 
-      LOGI("Companion response: %hhu\n", response);
-
       switch (response) {
         /* INFO: Even without any entry, we should still just deal with it */
         case 0: { return -2; }
@@ -367,6 +363,12 @@ void zygiskd_start(char *restrict argv[]) {
   enum RootImpl impl = get_impl();
   if (impl == None) {
     struct MsgHead *msg = malloc(sizeof(struct MsgHead) + sizeof("No root implementation found."));
+    if (msg == NULL) {
+      LOGE("Failed allocating memory for message.\n");
+
+      return;
+    }
+
     msg->cmd = DAEMON_SET_ERROR_INFO;
     msg->length = sizeof("No root implementation found.");
     memcpy(msg->data, "No root implementation found.", msg->length);
@@ -376,6 +378,12 @@ void zygiskd_start(char *restrict argv[]) {
     free(msg);
   } else if (impl == Multiple) {
     struct MsgHead *msg = malloc(sizeof(struct MsgHead) + sizeof("Multiple root implementations found. Not supported yet."));
+    if (msg == NULL) {
+      LOGE("Failed allocating memory for message.\n");
+
+      return;
+    }
+  
     msg->cmd = DAEMON_SET_ERROR_INFO;
     msg->length = sizeof("Multiple root implementations found. Not supported yet.");
     memcpy(msg->data, "Multiple root implementations found. Not supported yet.", msg->length);
@@ -421,6 +429,12 @@ void zygiskd_start(char *restrict argv[]) {
 
       if (context.len == 0) {
         msg = malloc(sizeof(struct MsgHead) + strlen("Root: , Modules: None") + root_impl_len + 1);
+        if (msg == NULL) {
+          LOGE("Failed allocating memory for message.\n");
+
+          return;
+        }
+
         msg->cmd = DAEMON_SET_INFO;
         msg->length = strlen("Root: , Modules: None") + root_impl_len + 1;
 
@@ -445,6 +459,12 @@ void zygiskd_start(char *restrict argv[]) {
         }
       } else {
         char *module_list = malloc(1);
+        if (module_list == NULL) {
+          LOGE("Failed allocating memory for module list.\n");
+
+          return;
+        }
+
         size_t module_list_len = 0;
 
         for (int i = 0; i < context.len; i++) {
@@ -466,6 +486,12 @@ void zygiskd_start(char *restrict argv[]) {
         }
 
         msg = malloc(sizeof(struct MsgHead) + strlen("Root: , Modules: ") + root_impl_len + module_list_len + 1);
+        if (msg == NULL) {
+          LOGE("Failed allocating memory for message.\n");
+
+          return;
+        }
+
         msg->cmd = DAEMON_SET_INFO;
         msg->length = strlen("Root: , Modules: ") + root_impl_len + module_list_len + 1;
 
@@ -532,18 +558,12 @@ void zygiskd_start(char *restrict argv[]) {
 
     switch (action) {
       case PingHeartbeat: {
-        LOGI("ZD-- PingHeartbeat\n");
-
         enum DaemonSocketAction msgr = ZYGOTE_INJECTED;
         unix_datagram_sendto(CONTROLLER_SOCKET, &msgr, sizeof(enum DaemonSocketAction));
 
-        LOGI("ZD++ PingHeartbeat\n");
-
         break;
       }
       case ZygoteRestart: {
-        LOGI("ZD-- ZygoteRestart\n");
-
         for (int i = 0; i < context.len; i++) {
           if (context.modules[i].companion != -1) {
             close(context.modules[i].companion);
@@ -551,18 +571,12 @@ void zygiskd_start(char *restrict argv[]) {
           }
         }
 
-        LOGI("ZD++ ZygoteRestart\n");
-
         break;
       }
       case SystemServerStarted: {
-        LOGI("ZD-- SystemServerStarted\n");
-
         enum DaemonSocketAction msgr = SYSTEM_SERVER_STARTED;
         unix_datagram_sendto(CONTROLLER_SOCKET, &msgr, sizeof(enum DaemonSocketAction));
 
-        LOGI("ZD++ SystemServerStarted\n");
-
         break;
       }
       case RequestLogcatFd: {
@@ -598,8 +612,6 @@ void zygiskd_start(char *restrict argv[]) {
         break;
       }
       case GetProcessFlags: {
-        LOGI("ZD-- GetProcessFlags\n");
-
         uint32_t uid = 0;
         ssize_t ret = read_uint32_t(client_fd, &uid);
         ASSURE_SIZE_READ_BREAK("GetProcessFlags", "uid", ret, sizeof(uid));
@@ -639,13 +651,9 @@ void zygiskd_start(char *restrict argv[]) {
         ret = write_int(client_fd, flags);
         ASSURE_SIZE_WRITE_BREAK("GetProcessFlags", "flags", ret, sizeof(flags));
 
-        LOGI("ZD++ GetProcessFlags\n");
-
         break;
       }
       case GetInfo: {
-        LOGI("ZD-- GetInfo\n");
-
         uint32_t flags = 0;
 
         switch (get_impl()) {
@@ -677,34 +685,40 @@ void zygiskd_start(char *restrict argv[]) {
 
         size_t modules_len = context.len;
         ret = write_size_t(client_fd, modules_len);
+        ASSURE_SIZE_WRITE_BREAK("GetInfo", "modules_len", ret, sizeof(modules_len));
         
         for (size_t i = 0; i < modules_len; i++) {
-          write_string(client_fd, context.modules[i].name);
+          ret = write_string(client_fd, context.modules[i].name);
-        }
+          if (ret == -1) {
+            LOGE("Failed writing module name.\n");
 
-        LOGI("ZD++ GetInfo\n");
+            break;
+          }
+        }
 
         break;
       }
       case ReadModules: {
-        LOGI("ZD-- ReadModules\n");
-
         size_t clen = context.len;
         ssize_t ret = write_size_t(client_fd, clen);
         ASSURE_SIZE_WRITE_BREAK("ReadModules", "len", ret, sizeof(clen));
 
         for (size_t i = 0; i < clen; i++) {
-          if (write_string(client_fd, context.modules[i].name) == -1) break;
+          if (write_string(client_fd, context.modules[i].name) == -1) {
-          if (write_fd(client_fd, context.modules[i].lib_fd) == -1) break;
+            LOGE("Failed writing module name.\n");
-        }
 
-        LOGI("ZD++ ReadModules\n");
+            break;
+          }
+          if (write_fd(client_fd, context.modules[i].lib_fd) == -1) {
+            LOGE("Failed writing module fd.\n");
+
+            break;
+          }
+        }
 
         break;
       }
       case RequestCompanionSocket: {
-        LOGI("ZD-- RequestCompanionSocket\n");
-
         size_t index = 0;
         ssize_t ret = read_size_t(client_fd, &index);
         ASSURE_SIZE_READ_BREAK("RequestCompanionSocket", "index", ret, sizeof(index));
@@ -770,10 +784,9 @@ void zygiskd_start(char *restrict argv[]) {
         break;
       }
       case GetModuleDir: {
-        LOGI("ZD-- GetModuleDir\n");
-
         size_t index = 0;
         read_size_t(client_fd, &index);
+        ASSURE_SIZE_READ_BREAK("GetModuleDir", "index", ret, sizeof(index));
 
         char module_dir[PATH_MAX];
         snprintf(module_dir, PATH_MAX, "%s/%s", PATH_MODULES_DIR, context.modules[index].name);
@@ -802,8 +815,6 @@ void zygiskd_start(char *restrict argv[]) {
           break;
         }
 
-        LOGI("ZD++ GetModuleDir\n");
-
         break;
       }
     }