improve: companion.c, dl.c and utils.c code

This commit improves the code for multiple files by making "read_string" function already make the string NULL-terminated, avoiding code duplication. Also for "companion.c" fixes an "if" where it would read "client_fd" and check if "fd" is equal to "-1", instead of "client_fd", also does some overall code improvements there like detaching the thread, avoiding memory leaks in the exit, of the thread itself.

zygiskd/src/companion.c

@@ -14,59 +14,56 @@
 
 #include <android/log.h>
 
-#include "companion.h"
 #include "dl.h"
 #include "utils.h"
 
-typedef void (*zygisk_companion_entry_func)(int);
+typedef void (*zygisk_companion_entry)(int);
 
 struct companion_module_thread_args {
   int fd;
-  zygisk_companion_entry_func entry;
+  zygisk_companion_entry entry;
 };
 
-zygisk_companion_entry_func load_module(int fd) {
+zygisk_companion_entry load_module(int fd) {
   char path[PATH_MAX];
   snprintf(path, sizeof(path), "/proc/self/fd/%d", fd);
 
   void *handle = android_dlopen(path, RTLD_NOW);
   void *entry = dlsym(handle, "zygisk_companion_entry");
-  
-  return (zygisk_companion_entry_func)entry;
+
+  return (zygisk_companion_entry)entry;
 }
 
 void *entry_thread(void *arg) {
   struct companion_module_thread_args *args = (struct companion_module_thread_args *)arg;
 
   int fd = args->fd;
-  zygisk_companion_entry_func module_entry = args->entry;
+  zygisk_companion_entry module_entry = args->entry;
 
-  struct stat st0;
+  struct stat st0 = { 0 };
   if (fstat(fd, &st0) == -1) {
-    LOGE("Failed to get client fd stats\n");
+    LOGE("Failed to get initial client fd stats: %s\n", strerror(errno));
 
-    close(fd);
     free(args);
 
-    pthread_exit(NULL);
+    return NULL;
   }
 
   module_entry(fd);
 
+  /* INFO: Only attempt to close the client fd if it appears to be the same file
+             and if we can successfully stat it again. This prevents double closes
+             if the module companion already closed the fd.
+  */
   struct stat st1;
-  if (fstat(fd, &st1) == -1) {
-    LOGE("Failed to get client fd stats\n");
+  if (fstat(fd, &st1) == 0) {
+    if (st0.st_dev == st1.st_dev && st0.st_ino == st1.st_ino) {
+      LOGI("Client fd stats unchanged. Closing.\n");
 
-    close(fd);
-    free(args);
-
-    pthread_exit(NULL);
-  }
-
-  if (st0.st_dev != st1.st_dev || st0.st_ino != st1.st_ino) {
-    LOGI("Client fd changed. Closing.\n");
-
-    close(fd);
+      close(fd);
+    } else {
+      LOGI("Client fd stats changed, assuming module handled closing.\n");
+    }
   }
 
   free(args);
@@ -78,45 +75,44 @@ void *entry_thread(void *arg) {
 void companion_entry(int fd) {
   LOGI("New companion entry.\n - Client fd: %d\n", fd);
 
-  /* TODO: Use non-NULL string termination */
   char name[256 + 1];
-  ssize_t name_length = read_string(fd, name, sizeof(name) - 1);
-  if (name_length == -1) {
+  ssize_t ret = read_string(fd, name, sizeof(name));
+  if (ret == -1) {
     LOGE("Failed to read module name\n");
 
-    ssize_t ret = write_uint8_t(fd, 2);
-    ASSURE_SIZE_WRITE("ZygiskdCompanion", "name", ret, sizeof(uint8_t));
+    /* TODO: Is that appropriate? */
+    close(fd);
 
     exit(0);
   }
-  name[name_length] = '\0';
 
   LOGI(" - Module name: \"%s\"\n", name);
 
   int library_fd = read_fd(fd);
-  ssize_t ret = 0;
   if (library_fd == -1) {
     LOGE("Failed to receive library fd\n");
 
-    ret = write_uint8_t(fd, 2);
-    ASSURE_SIZE_WRITE("ZygiskdCompanion", "library_fd", ret, sizeof(uint8_t));
+    /* TODO: Is that appropriate? */
+    close(fd);
 
     exit(0);
   }
 
   LOGI(" - Library fd: %d\n", library_fd);
 
-  zygisk_companion_entry_func module_entry = load_module(library_fd);
+  zygisk_companion_entry module_entry = load_module(library_fd);
   close(library_fd);
 
   if (module_entry == NULL) {
-    LOGE("No companion module entry for module: %s\n", name);
+    LOGE(" - No companion module entry for module: %s\n", name);
 
     ret = write_uint8_t(fd, 0);
     ASSURE_SIZE_WRITE("ZygiskdCompanion", "module_entry", ret, sizeof(uint8_t));
 
     exit(0);
   } else {
+    LOGI(" - Module entry found\n");
+
     ret = write_uint8_t(fd, 1);
     ASSURE_SIZE_WRITE("ZygiskdCompanion", "module_entry", ret, sizeof(uint8_t));
   }
@@ -126,12 +122,10 @@ void companion_entry(int fd) {
       LOGE("Something went wrong in companion. Bye!\n");
 
       exit(0);
-
-      break;
     }
-  
+
     int client_fd = read_fd(fd);
-    if (fd == -1) {
+    if (client_fd == -1) {
       LOGE("Failed to receive client fd\n");
 
       exit(0);
@@ -149,12 +143,15 @@ void companion_entry(int fd) {
     args->fd = client_fd;
     args->entry = module_entry;
 
-    LOGI("New companion request.\n - Module name: %s\n - Client fd: %d\n", name, args->fd);
+    LOGI("New companion request.\n - Module name: %s\n - Client fd: %d\n", name, client_fd);
 
-    ret = write_uint8_t(args->fd, 1);
+    ret = write_uint8_t(client_fd, 1);
     ASSURE_SIZE_WRITE("ZygiskdCompanion", "client_fd", ret, sizeof(uint8_t));
-    
+
     pthread_t thread;
     pthread_create(&thread, NULL, entry_thread, args);
+    pthread_detach(thread);
+
+    LOGI(" - Spawned companion thread for client fd: %d\n", client_fd);
   }
 }

zygiskd/src/dl.c

@@ -13,14 +13,13 @@
 #include <android/log.h>
 
 #include "companion.h"
-#include "dl.h"
 #include "utils.h"
 
 #define ANDROID_NAMESPACE_TYPE_SHARED 0x2
 #define ANDROID_DLEXT_USE_NAMESPACE 0x200
 
 typedef struct AndroidNamespace {
-  unsigned char _unused[0];
+  uint8_t _unused[0];
 } AndroidNamespace;
 
 typedef struct AndroidDlextinfo {
@@ -33,6 +32,8 @@ typedef struct AndroidDlextinfo {
   AndroidNamespace *library_namespace;
 } AndroidDlextinfo;
 
+extern void *android_dlopen_ext(const char *filename, int flags, const AndroidDlextinfo *extinfo);
+
 typedef AndroidNamespace *(*AndroidCreateNamespaceFn)(
   const char *name,
   const char *ld_library_path,
@@ -43,8 +44,6 @@ typedef AndroidNamespace *(*AndroidCreateNamespaceFn)(
   const void *caller_addr
 );
 
-extern void *android_dlopen_ext(const char *filename, int flags, const AndroidDlextinfo *extinfo);
-
 void *android_dlopen(char *path, int flags) {
   char *dir = dirname(path);
   struct AndroidDlextinfo info = {

zygiskd/src/utils.c

@@ -282,18 +282,16 @@ write_func(uint8_t)
 read_func(uint8_t)
 
 ssize_t write_string(int fd, const char *restrict str) {
-  size_t len[1];
-  len[0] = strlen(str);
-
-  ssize_t written_bytes = write(fd, &len, sizeof(size_t));
+  size_t str_len = strlen(str);
+  ssize_t written_bytes = write(fd, &str_len, sizeof(size_t));
   if (written_bytes != sizeof(size_t)) {
     LOGE("Failed to write string length: Not all bytes were written (%zd != %zu).\n", written_bytes, sizeof(size_t));
 
     return -1;
   }
 
-  written_bytes = write(fd, str, len[0]);
-  if ((size_t)written_bytes != len[0]) {
+  written_bytes = write(fd, str, str_len);
+  if ((size_t)written_bytes != str_len) {
     LOGE("Failed to write string: Not all bytes were written.\n");
 
     return -1;
@@ -302,31 +300,30 @@ ssize_t write_string(int fd, const char *restrict str) {
   return written_bytes;
 }
 
-ssize_t read_string(int fd, char *restrict str, size_t len) {
-  size_t str_len_buf[1];
-
-  ssize_t read_bytes = read(fd, &str_len_buf, sizeof(size_t));
+ssize_t read_string(int fd, char *restrict buf, size_t buf_size) {
+  size_t str_len = 0;
+  ssize_t read_bytes = read(fd, &str_len, sizeof(size_t));
   if (read_bytes != (ssize_t)sizeof(size_t)) {
     LOGE("Failed to read string length: Not all bytes were read (%zd != %zu).\n", read_bytes, sizeof(size_t));
 
     return -1;
   }
   
-  size_t str_len = str_len_buf[0];
-
-  if (str_len > len) {
-    LOGE("Failed to read string: Buffer is too small (%zu > %zu).\n", str_len, len);
+  if (str_len > buf_size - 1) {
+    LOGE("Failed to read string: Buffer is too small (%zu > %zu - 1).\n", str_len, buf_size);
 
     return -1;
   }
 
-  read_bytes = read(fd, str, str_len);
+  read_bytes = read(fd, buf, str_len);
   if (read_bytes != (ssize_t)str_len) {
     LOGE("Failed to read string: Promised bytes doesn't exist (%zd != %zu).\n", read_bytes, str_len);
 
     return -1;
   }
 
+  if (str_len > 0) buf[str_len] = '\0';
+
   return read_bytes;
 }
 

zygiskd/src/utils.h

@@ -91,7 +91,7 @@ read_func_def(uint8_t);
 
 ssize_t write_string(int fd, const char *restrict str);
 
-ssize_t read_string(int fd, char *restrict str, size_t len);
+ssize_t read_string(int fd, char *restrict buf, size_t buf_size);
 
 bool exec_command(char *restrict buf, size_t len, const char *restrict file, char *const argv[]);
 

zygiskd/src/zygiskd.c

@@ -465,13 +465,15 @@ void zygiskd_start(char *restrict argv[]) {
 
         break;
       }
+      /* TODO: Move to another thread and save client fds to an epoll list
+                 so that we can, in a single-thread, deal with multiple logcats */
       case RequestLogcatFd: {
         uint8_t level = 0;
         ssize_t ret = read_uint8_t(client_fd, &level);
         ASSURE_SIZE_READ_BREAK("RequestLogcatFd", "level", ret, sizeof(level));
 
         char tag[128 + 1];
-        ret = read_string(client_fd, tag, sizeof(tag) - 1);
+        ret = read_string(client_fd, tag, sizeof(tag));
         if (ret == -1) {
           LOGE("Failed reading logcat tag.\n");
 
@@ -480,10 +482,7 @@ void zygiskd_start(char *restrict argv[]) {
           break;
         }
 
-        tag[ret] = '\0';
-
-        /* INFO: Non-NULL terminated */
-        char message[1024];
+        char message[1024 + 1];
         ret = read_string(client_fd, message, sizeof(message));
         if (ret == -1) {
           LOGE("Failed reading logcat message.\n");
@@ -493,7 +492,7 @@ void zygiskd_start(char *restrict argv[]) {
           break;
         }
 
-        __android_log_print(level, tag, "%.*s", (int)ret, message);
+        __android_log_print(level, tag, "%s", message);
 
         break;
       }