improve: companion handler fd closing; fix: PIPE signal handling (#103)

This commit improves how we decide to close the fd that connects the injected module with the companion, avoiding both double close and fd leaks.

zygiskd/src/companion.c

@@ -7,6 +7,7 @@
 #include <dlfcn.h>
 #include <errno.h>
 #include <fcntl.h>
+#include <signal.h>
 
 #include <unistd.h>
 #include <linux/limits.h>
@@ -17,6 +18,9 @@
 #include "dl.h"
 #include "utils.h"
 
+#undef LOG_TAG
+#define LOG_TAG lp_select("zygiskd-companion32", "zygiskd-companion64")
+
 typedef void (*zygisk_companion_entry)(int);
 
 struct companion_module_thread_args {
@@ -28,12 +32,23 @@ 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 *handle = dlopen_ext(path, RTLD_NOW);
+
+  if (!handle) return NULL;
+
   void *entry = dlsym(handle, "zygisk_companion_entry");
+  if (!entry) {
+    LOGE("Failed to dlsym zygisk_companion_entry: %s\n", dlerror());
+
+    dlclose(handle);
+
+    return NULL;
+  }
 
   return (zygisk_companion_entry)entry;
 }
 
+/* WARNING: Dynamic memory based */
 void *entry_thread(void *arg) {
   struct companion_module_thread_args *args = (struct companion_module_thread_args *)arg;
 
@@ -42,7 +57,7 @@ void *entry_thread(void *arg) {
 
   struct stat st0 = { 0 };
   if (fstat(fd, &st0) == -1) {
-    LOGE("Failed to get initial client fd stats: %s\n", strerror(errno));
+    LOGE(" - Failed to get initial client fd stats: %s\n", strerror(errno));
 
     free(args);
 
@@ -56,14 +71,10 @@ void *entry_thread(void *arg) {
              if the module companion already closed the fd.
   */
   struct stat st1;
-  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");
+  if (fstat(fd, &st1) != -1 || st0.st_ino == st1.st_ino) {
+    LOGI(" - Client fd changed after module entry\n");
 
-      close(fd);
-    } else {
-      LOGI("Client fd stats changed, assuming module handled closing.\n");
-    }
+    close(fd);
   }
 
   free(args);
@@ -80,10 +91,7 @@ void companion_entry(int fd) {
   if (ret == -1) {
     LOGE("Failed to read module name\n");
 
-    /* TODO: Is that appropriate? */
-    close(fd);
-
-    exit(0);
+    goto cleanup;
   }
 
   LOGI(" - Module name: \"%s\"\n", name);
@@ -92,10 +100,7 @@ void companion_entry(int fd) {
   if (library_fd == -1) {
     LOGE("Failed to receive library fd\n");
 
-    /* TODO: Is that appropriate? */
-    close(fd);
-
-    exit(0);
+    goto cleanup;
   }
 
   LOGI(" - Library fd: %d\n", library_fd);
@@ -109,26 +114,33 @@ void companion_entry(int fd) {
     ret = write_uint8_t(fd, 0);
     ASSURE_SIZE_WRITE("ZygiskdCompanion", "module_entry", ret, sizeof(uint8_t));
 
-    exit(0);
+    goto cleanup;
   } else {
     LOGI(" - Module entry found\n");
 
     ret = write_uint8_t(fd, 1);
     ASSURE_SIZE_WRITE("ZygiskdCompanion", "module_entry", ret, sizeof(uint8_t));
   }
 
+  struct sigaction sa;
+  memset(&sa, 0, sizeof(sa));
+
+  sigemptyset(&sa.sa_mask);
+  sa.sa_handler = SIG_IGN;
+  sigaction(SIGPIPE, &sa, NULL);
+
   while (1) {
     if (!check_unix_socket(fd, true)) {
       LOGE("Something went wrong in companion. Bye!\n");
 
-      exit(0);
+      break;
     }
 
     int client_fd = read_fd(fd);
     if (client_fd == -1) {
       LOGE("Failed to receive client fd\n");
 
-      exit(0);
+      break;
     }
 
     struct companion_module_thread_args *args = malloc(sizeof(struct companion_module_thread_args));
@@ -137,7 +149,7 @@ void companion_entry(int fd) {
 
       close(client_fd);
 
-      exit(0);
+      break;
     }
 
     args->fd = client_fd;
@@ -149,9 +161,20 @@ void companion_entry(int fd) {
     ASSURE_SIZE_WRITE("ZygiskdCompanion", "client_fd", ret, sizeof(uint8_t));
 
     pthread_t thread;
-    pthread_create(&thread, NULL, entry_thread, args);
-    pthread_detach(thread);
+    if (pthread_create(&thread, NULL, entry_thread, (void *)args) == 0)
+      continue;
+
+    LOGE(" - Failed to create thread for companion module\n");
 
-    LOGI(" - Spawned companion thread for client fd: %d\n", client_fd);
+    close(client_fd);
+    free(args);
+
+    break;
   }
+
+  cleanup:
+    close(fd);
+    LOGE("Companion thread exited\n");
+
+    exit(0);
 }

zygiskd/src/dl.c

@@ -11,77 +11,47 @@
 #include <stdint.h>
 
 #include <android/log.h>
+#include <android/dlext.h>
 
 #include "companion.h"
 #include "utils.h"
 
-#define ANDROID_NAMESPACE_TYPE_SHARED 0x2
-#define ANDROID_DLEXT_USE_NAMESPACE 0x200
-
-typedef struct AndroidNamespace {
-  uint8_t _unused[0];
-} AndroidNamespace;
-
-typedef struct AndroidDlextinfo {
-  uint64_t flags;
-  void *reserved_addr;
-  size_t reserved_size;
-  int relro_fd;
-  int library_fd;
-  off64_t library_fd_offset;
-  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,
-  const char *default_library_path,
-  uint64_t type,
-  const char *permitted_when_isolated_path,
-  AndroidNamespace *parent,
-  const void *caller_addr
-);
-
-void *android_dlopen(char *path, int flags) {
+#define __LOADER_ANDROID_CREATE_NAMESPACE_TYPE(name) struct android_namespace_t *(*name)( \
+  const char *name,                                                                      \
+  const char *ld_library_path,                                                           \
+  const char *default_library_path,                                                      \
+  uint64_t type,                                                                         \
+  const char *permitted_when_isolated_path,                                              \
+  struct android_namespace_t *parent,                                                    \
+  const void *caller_addr)
+
+void *dlopen_ext(const char* path, int flags) {
+  android_dlextinfo info = { 0 };
   char *dir = dirname(path);
-  struct AndroidDlextinfo info = {
-    .flags = 0,
-    .reserved_addr = NULL,
-    .reserved_size = 0,
-    .relro_fd = 0,
-    .library_fd = 0,
-    .library_fd_offset = 0,
-    .library_namespace = NULL,
-  };
 
-  void *handle = dlsym(RTLD_DEFAULT, "__loader_android_create_namespace");
-  AndroidCreateNamespaceFn android_create_namespace_fn = (AndroidCreateNamespaceFn)handle;
+  __LOADER_ANDROID_CREATE_NAMESPACE_TYPE(__loader_android_create_namespace) = (__LOADER_ANDROID_CREATE_NAMESPACE_TYPE( ))dlsym(RTLD_DEFAULT, "__loader_android_create_namespace");
 
-  AndroidNamespace *ns = android_create_namespace_fn(
-    path,
-    dir,
-    NULL,
-    ANDROID_NAMESPACE_TYPE_SHARED,
-    NULL,
-    NULL,
-    (const void *)&android_dlopen
-  );
+  struct android_namespace_t *ns = __loader_android_create_namespace == NULL ? NULL :
+              __loader_android_create_namespace(path, dir, NULL,
+                                                2, /* ANDROID_NAMESPACE_TYPE_SHARED */
+                                                NULL, NULL,
+                                                (void *)&dlopen_ext);
 
-  if (ns != NULL) {
+  if (ns) {
     info.flags = ANDROID_DLEXT_USE_NAMESPACE;
     info.library_namespace = ns;
 
-    LOGI("Open %s with namespace %p\n", path, (void *)ns);
+    LOGI("Open %s with namespace %p", path, (void *)ns);
   } else {
-    LOGI("Cannot create namespace for %s\n", path);
+    LOGW("Cannot create namespace for %s", path);
   }
 
-  void *result = android_dlopen_ext(path, flags, &info);
-  if (result == NULL) {
-    LOGE("Failed to dlopen %s: %s\n", path, dlerror());
+  void *handle = android_dlopen_ext(path, flags, &info);
+  if (handle) {
+    LOGI("dlopen %s: %p", path, handle);
+  } else {
+    LOGE("dlopen %s: %s", path, dlerror());
   }
 
-  return result;
+  return handle;
 }

zygiskd/src/dl.h

@@ -1,6 +1,6 @@
 #ifndef DL_H
 #define DL_H
 
-void *android_dlopen(char *path, int flags);
+void *dlopen_ext(char *path, int flags);
 
 #endif /* DL_H */

zygiskd/src/utils.c

@@ -228,11 +228,11 @@ ssize_t write_fd(int fd, int sendfd) {
 
 int read_fd(int fd) {
   char cmsgbuf[CMSG_SPACE(sizeof(int))];
-  char buf[1] = { 0 };
-
+
+  int cnt = 1;
   struct iovec iov = {
-    .iov_base = buf,
-    .iov_len = 1
+    .iov_base = &cnt,
+    .iov_len = sizeof(cnt)
   };
 
   struct msghdr msg = {
@@ -242,7 +242,7 @@ int read_fd(int fd) {
     .msg_controllen = sizeof(cmsgbuf)
   };
 
-  ssize_t ret = recvmsg(fd, &msg, 0);
+  ssize_t ret = recvmsg(fd, &msg, MSG_WAITALL);
   if (ret == -1) {
     LOGE("recvmsg: %s\n", strerror(errno));
 

zygiskd/src/utils.h

@@ -9,12 +9,18 @@
 #define CONCAT_(x,y) x##y
 #define CONCAT(x,y) CONCAT_(x,y)
 
-#define LOGI(...)                                                                           \
-  __android_log_print(ANDROID_LOG_INFO, lp_select("zygiskd32", "zygiskd64"), __VA_ARGS__);  \
+#define LOG_TAG lp_select("zygiskd32", "zygiskd64")
+
+#define LOGI(...)                                              \
+  __android_log_print(ANDROID_LOG_INFO, LOG_TAG, __VA_ARGS__); \
+  printf(__VA_ARGS__);
+
+#define LOGW(...)                                                \
+  __android_log_print(ANDROID_LOG_WARN, LOG_TAG, __VA_ARGS__);   \
   printf(__VA_ARGS__);
 
-#define LOGE(...)                                                                            \
-  __android_log_print(ANDROID_LOG_ERROR , lp_select("zygiskd32", "zygiskd64"), __VA_ARGS__); \
+#define LOGE(...)                                                \
+  __android_log_print(ANDROID_LOG_ERROR , LOG_TAG, __VA_ARGS__); \
   printf(__VA_ARGS__);
 
 #define ASSURE_SIZE_WRITE(area_name, subarea_name, sent_size, expected_size)                                     \