Simplify fd sanitization

native/src/zygisk/hook.cpp

@@ -45,6 +45,8 @@ enum {
     FLAG_MAX
 };
 
+#define MAX_FD_SIZE 1024
+
 // Global variables
 vector<tuple<dev_t, ino_t, const char *, void **>> *plt_hook_list;
 map<string, vector<JNINativeMethod>, StringCmp> *jni_hook_list;
@@ -53,6 +55,7 @@ bool should_unmap_zygisk = false;
 
 // Current context
 HookContext *g_ctx;
+bitset<MAX_FD_SIZE> *g_allowed_fds = nullptr;
 const JNINativeInterface *old_functions = nullptr;
 JNINativeInterface *new_functions = nullptr;
 
@@ -60,8 +63,6 @@ JNINativeInterface *new_functions = nullptr;
 void name##_pre();         \
 void name##_post();
 
-#define MAX_FD_SIZE 1024
-
 struct HookContext {
     JNIEnv *env;
     union {
@@ -77,7 +78,6 @@ struct HookContext {
     int pid;
     bitset<FLAG_MAX> flags;
     uint32_t info_flags;
-    bitset<MAX_FD_SIZE> allowed_fds;
     vector<int> exempted_fds;
 
     struct RegisterInfo {
@@ -121,6 +121,7 @@ struct HookContext {
     void sanitize_fds();
     bool exempt_fd(int fd);
     bool is_child() const { return pid <= 0; }
+    bool can_exempt_fd() const { return flags[APP_FORK_AND_SPECIALIZE] && args.app->fds_to_ignore; }
 
     // Compatibility shim
     void plt_hook_register(const char *regex, const char *symbol, void *fn, void **backup);
@@ -169,19 +170,13 @@ DCL_HOOK_FUNC(int, unshare, int flags) {
     return res;
 }
 
-// Close logd_fd if necessary to prevent crashing
+// Sanitize file descriptors to prevent crashing
-// For more info, check comments in zygisk_log_write
 DCL_HOOK_FUNC(void, android_log_close) {
     if (g_ctx == nullptr) {
         // Happens during un-managed fork like nativeForkApp, nativeForkUsap
         get_magiskd().close_log_pipe();
-    } else if (!g_ctx->flags[SKIP_FD_SANITIZATION]) {
+    } else {
-        g_ctx->magiskd.close_log_pipe();
+        g_ctx->sanitize_fds();
-        if (g_ctx->is_child()) {
-            // Switch to plain old android logging because we cannot talk
-            // to magiskd to fetch our log pipe afterwards anyways.
-            android_logging();
-        }
     }
     old_android_log_close();
 }
@@ -412,26 +407,32 @@ int sigmask(int how, int signum) {
 }
 
 void HookContext::fork_pre() {
+    if (g_allowed_fds == nullptr) {
+        default_new(g_allowed_fds);
+
+        auto &allowed_fds = *g_allowed_fds;
+        // Record all open fds
+        auto dir = xopen_dir("/proc/self/fd");
+        for (dirent *entry; (entry = xreaddir(dir.get()));) {
+            int fd = parse_int(entry->d_name);
+            if (fd < 0 || fd >= MAX_FD_SIZE) {
+                close(fd);
+                continue;
+            }
+            allowed_fds[fd] = true;
+        }
+        // The dirfd will be closed once out of scope
+        allowed_fds[dirfd(dir.get())] = false;
+        // logd_fd should be handled separately
+        if (int logd_fd = magiskd.get_log_pipe(); logd_fd >= 0) {
+            allowed_fds[logd_fd] = false;
+        }
+    }
+
     // Do our own fork before loading any 3rd party code
     // First block SIGCHLD, unblock after original fork is done
     sigmask(SIG_BLOCK, SIGCHLD);
     pid = old_fork();
-
-    if (pid != 0 || flags[SKIP_FD_SANITIZATION])
-        return;
-
-    // Record all open fds
-    auto dir = xopen_dir("/proc/self/fd");
-    for (dirent *entry; (entry = xreaddir(dir.get()));) {
-        int fd = parse_int(entry->d_name);
-        if (fd < 0 || fd >= MAX_FD_SIZE) {
-            close(fd);
-            continue;
-        }
-        allowed_fds[fd] = true;
-    }
-    // The dirfd should not be allowed
-    allowed_fds[dirfd(dir.get())] = false;
 }
 
 void HookContext::fork_post() {
@@ -443,17 +444,27 @@ void HookContext::sanitize_fds() {
     if (flags[SKIP_FD_SANITIZATION])
         return;
 
-    if (flags[APP_FORK_AND_SPECIALIZE] && args.app->fds_to_ignore) {
+    if (!is_child() || g_allowed_fds == nullptr) {
-        auto update_fd_array = [&](int off) -> jintArray {
+        magiskd.close_log_pipe();
+        return;
+    }
+
+    auto &allowed_fds = *g_allowed_fds;
+    if (can_exempt_fd()) {
+        if (int logd_fd = magiskd.get_log_pipe(); logd_fd >= 0) {
+            exempted_fds.push_back(logd_fd);
+        }
+
+        auto update_fd_array = [&](int old_len) -> jintArray {
             if (exempted_fds.empty())
                 return nullptr;
 
-            jintArray array = env->NewIntArray(static_cast<int>(off + exempted_fds.size()));
+            jintArray array = env->NewIntArray(static_cast<int>(old_len + exempted_fds.size()));
             if (array == nullptr)
                 return nullptr;
 
-            env->SetIntArrayRegion(array, off, static_cast<int>(exempted_fds.size()),
+            env->SetIntArrayRegion(
-                                   exempted_fds.data());
+                    array, old_len, static_cast<int>(exempted_fds.size()), exempted_fds.data());
             for (int fd : exempted_fds) {
                 if (fd >= 0 && fd < MAX_FD_SIZE) {
                     allowed_fds[fd] = true;
@@ -480,6 +491,11 @@ void HookContext::sanitize_fds() {
         } else {
             update_fd_array(0);
         }
+    } else {
+        magiskd.close_log_pipe();
+        // Switch to plain old android logging because we cannot talk
+        // to magiskd to fetch our log pipe afterwards anyways.
+        android_logging();
     }
 
     // Close all forbidden fds to prevent crashing
@@ -487,8 +503,7 @@ void HookContext::sanitize_fds() {
     int dfd = dirfd(dir.get());
     for (dirent *entry; (entry = xreaddir(dir.get()));) {
         int fd = parse_int(entry->d_name);
-        int logd_fd = magiskd.get_log_pipe();
+        if ((fd < 0 || fd >= MAX_FD_SIZE || !allowed_fds[fd]) && fd != dfd) {
-        if ((fd < 0 || fd >= MAX_FD_SIZE || !allowed_fds[fd]) && fd != dfd && fd != logd_fd) {
             close(fd);
         }
     }
@@ -645,7 +660,7 @@ HookContext::~HookContext() {
 bool HookContext::exempt_fd(int fd) {
     if (flags[POST_SPECIALIZE] || flags[SKIP_FD_SANITIZATION])
         return true;
-    if (!flags[APP_FORK_AND_SPECIALIZE])
+    if (!can_exempt_fd())
         return false;
     exempted_fds.push_back(fd);
     return true;
@@ -673,7 +688,6 @@ void HookContext::nativeForkSystemServer_pre() {
     fork_pre();
     if (is_child()) {
         server_specialize_pre();
-        sanitize_fds();
     }
 }
 
@@ -688,24 +702,11 @@ void HookContext::nativeForkSystemServer_post() {
 void HookContext::nativeForkAndSpecialize_pre() {
     process = env->GetStringUTFChars(args.app->nice_name, nullptr);
     ZLOGV("pre  forkAndSpecialize [%s]\n", process);
-
     flags[APP_FORK_AND_SPECIALIZE] = true;
-    if (args.app->fds_to_ignore == nullptr) {
-        // if fds_to_ignore does not exist, we do not have a good way to determine
-        // whether keeping fd open during fork is allowed, as needed symbols may be
-        // inlined. Better be safe than sorry.
-        flags[SKIP_FD_SANITIZATION] = false;
-    } else {
-        int logd_fd = magiskd.get_log_pipe();
-        if (logd_fd >= 0) {
-            exempted_fds.push_back(logd_fd);
-        }
-    }
 
     fork_pre();
     if (is_child()) {
         app_specialize_pre();
-        sanitize_fds();
     }
 }