From 7a892e0d62f0dea4e484ccc673b48f4ce9b05d12 Mon Sep 17 00:00:00 2001 From: ThePedroo Date: Sun, 22 Dec 2024 14:57:39 -0300 Subject: [PATCH] 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 | 79 ++++++++++++++++++++--------------------- zygiskd/src/dl.c | 7 ++-- zygiskd/src/utils.c | 27 +++++++------- zygiskd/src/utils.h | 2 +- zygiskd/src/zygiskd.c | 11 +++--- 5 files changed, 59 insertions(+), 67 deletions(-) diff --git a/zygiskd/src/companion.c b/zygiskd/src/companion.c index ee54945..0646a77 100644 --- a/zygiskd/src/companion.c +++ b/zygiskd/src/companion.c @@ -14,59 +14,56 @@ #include -#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); } } diff --git a/zygiskd/src/dl.c b/zygiskd/src/dl.c index 5018b0f..26770da 100644 --- a/zygiskd/src/dl.c +++ b/zygiskd/src/dl.c @@ -13,14 +13,13 @@ #include #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 = { diff --git a/zygiskd/src/utils.c b/zygiskd/src/utils.c index 2285844..eff45d9 100644 --- a/zygiskd/src/utils.c +++ b/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; } diff --git a/zygiskd/src/utils.h b/zygiskd/src/utils.h index f5f6372..f9655f8 100644 --- a/zygiskd/src/utils.h +++ b/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[]); diff --git a/zygiskd/src/zygiskd.c b/zygiskd/src/zygiskd.c index 702bf90..6031955 100644 --- a/zygiskd/src/zygiskd.c +++ b/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; }