From da766f2a4e21ff9442eef9a553049baf9e20a9ef Mon Sep 17 00:00:00 2001 From: topjohnwu Date: Wed, 1 Nov 2023 02:01:18 -0700 Subject: [PATCH] Do not go through magiskd for getting the log pipe --- native/src/core/daemon.rs | 8 +--- native/src/core/lib.rs | 3 +- native/src/core/logging.rs | 93 +++++++++++++++++++++++++----------- native/src/zygisk/entry.cpp | 56 +--------------------- native/src/zygisk/hook.cpp | 8 ++-- native/src/zygisk/zygisk.hpp | 3 +- 6 files changed, 73 insertions(+), 98 deletions(-) diff --git a/native/src/core/daemon.rs b/native/src/core/daemon.rs index 3aa146c6c..47cffb30f 100644 --- a/native/src/core/daemon.rs +++ b/native/src/core/daemon.rs @@ -5,7 +5,7 @@ use std::sync::{Mutex, OnceLock}; use base::{cstr, Directory, ResultExt, Utf8CStr, Utf8CStrBuf, Utf8CStrBufRef, WalkResult}; use crate::get_prop; -use crate::logging::{magisk_logging, zygisk_logging}; +use crate::logging::magisk_logging; // Global magiskd singleton pub static MAGISKD: OnceLock = OnceLock::new(); @@ -50,12 +50,6 @@ pub fn daemon_entry() { magisk_logging(); } -pub fn zygisk_entry() { - let magiskd = MagiskD::default(); - MAGISKD.set(magiskd).ok(); - zygisk_logging(); -} - pub fn get_magiskd() -> &'static MagiskD { unsafe { MAGISKD.get().unwrap_unchecked() } } diff --git a/native/src/core/lib.rs b/native/src/core/lib.rs index 3e1310a5d..49b6ab0f8 100644 --- a/native/src/core/lib.rs +++ b/native/src/core/lib.rs @@ -3,7 +3,7 @@ use base::Utf8CStr; use cert::read_certificate; -use daemon::{daemon_entry, find_apk_path, get_magiskd, zygisk_entry, MagiskD}; +use daemon::{daemon_entry, find_apk_path, get_magiskd, MagiskD}; use logging::{android_logging, magisk_logging, zygisk_logging}; use resetprop::{persist_delete_prop, persist_get_prop, persist_get_props, persist_set_prop}; @@ -41,7 +41,6 @@ pub mod ffi { #[namespace = "rust"] extern "Rust" { fn daemon_entry(); - fn zygisk_entry(); type MagiskD; fn get_magiskd() -> &'static MagiskD; diff --git a/native/src/core/logging.rs b/native/src/core/logging.rs index 6054af5d1..c50d1f41a 100644 --- a/native/src/core/logging.rs +++ b/native/src/core/logging.rs @@ -3,8 +3,9 @@ use std::ffi::{c_char, c_void}; use std::fmt::Write as FmtWrite; use std::fs::File; use std::io::{IoSlice, Read, Write}; -use std::os::fd::{AsRawFd, FromRawFd, IntoRawFd, RawFd}; +use std::os::fd::{AsRawFd, FromRawFd, RawFd}; use std::ptr::null_mut; +use std::sync::atomic::{AtomicI32, Ordering}; use std::{fs, io}; use bytemuck::{bytes_of, bytes_of_mut, write_zeroes, Pod, Zeroable}; @@ -42,9 +43,6 @@ type ThreadEntry = extern "C" fn(*mut c_void) -> *mut c_void; extern "C" { fn __android_log_write(prio: i32, tag: *const c_char, msg: *const c_char); fn strftime(buf: *mut c_char, len: usize, fmt: *const c_char, tm: *const tm) -> usize; - - fn zygisk_fetch_logd() -> RawFd; - fn zygisk_close_logd(); fn new_daemon_thread(entry: ThreadEntry, arg: *mut c_void); } @@ -113,13 +111,6 @@ struct LogMeta { tid: i32, } -fn print_log_error(msg: &str, e: io::Error) { - let mut buf = Utf8CStrBufArr::default(); - buf.write_fmt(format_args_nl!("Error in {}: {}", msg, e)) - .ok(); - write_android_log(level_to_prio(LogLevel::Error), &buf); -} - fn write_android_log(prio: i32, msg: &Utf8CStr) { unsafe { __android_log_write(prio, raw_cstr!("Magisk"), msg.as_ptr()); @@ -142,7 +133,14 @@ fn write_log_to_pipe(logd: &mut File, prio: i32, msg: &Utf8CStr) -> io::Result i32 { + ZYGISK_LOGD.load(Ordering::Relaxed) +} + fn zygisk_log_to_pipe(prio: i32, msg: &Utf8CStr) { - let mut logd = unsafe { - let fd = zygisk_fetch_logd(); - if fd < 0 { + // If we don't have the log pipe set, open the log pipe FIFO. This could actually happen + // multiple times in the zygote daemon (parent process) because we had to close this + // file descriptor to prevent crashing. + // + // For some reason, zygote sanitizes and checks FDs *before* forking. This results in the fact + // that *every* time before zygote forks, it has to close all logging related FDs in order + // to pass FD checks, just to have it re-initialized immediately after any + // logging happens ¯\_(ツ)_/¯. + // + // To be consistent with this behavior, we also have to close the log pipe to magiskd + // to make zygote NOT crash if necessary. We accomplish this by hooking __android_log_close + // and closing it at the same time as the rest of logging FDs. + + let mut fd = ZYGISK_LOGD.load(Ordering::Relaxed); + if fd < 0 { + android_logging(); + let mut buf = Utf8CStrBufArr::default(); + let path = FsPathBuf::new(&mut buf) + .join(get_magisk_tmp()) + .join(LOG_PIPE!()); + // Open as RW as sometimes it may block + fd = unsafe { libc::open(path.as_ptr(), O_RDWR | O_CLOEXEC) }; + if fd >= 0 { + // Only re-enable zygisk logging if success + zygisk_logging(); + unsafe { + libc::close(ZYGISK_LOGD.swap(fd, Ordering::Relaxed)); + } + } else { + // Cannot talk to pipe, abort return; } - File::from_raw_fd(fd) - }; + } // Block SIGPIPE let mut mask: sigset_t; @@ -185,24 +222,24 @@ fn zygisk_log_to_pipe(prio: i32, msg: &Utf8CStr) { pthread_sigmask(SIG_BLOCK, &mask, &mut orig_mask); } - let result = write_log_to_pipe(&mut logd, prio, msg); - // Make sure the file descriptor is not closed after out of scope - logd.into_raw_fd(); + let result = { + let mut logd = unsafe { File::from_raw_fd(fd) }; + let result = write_log_to_pipe(&mut logd, prio, msg); + // Make sure the file descriptor is not closed after out of scope + std::mem::forget(logd); + result + }; // Consume SIGPIPE if exists, then restore mask unsafe { let ts: timespec = std::mem::zeroed(); - sigtimedwait(&mask, null_mut(), &ts); pthread_sigmask(SIG_SETMASK, &orig_mask, null_mut()); } // If any error occurs, shut down the logd pipe - if let Err(e) = result { - print_log_error("zygisk_log", e); - unsafe { - zygisk_close_logd(); - } + if result.is_err() { + zygisk_close_logd(); } } diff --git a/native/src/zygisk/entry.cpp b/native/src/zygisk/entry.cpp index c021ead11..6e10e73c9 100644 --- a/native/src/zygisk/entry.cpp +++ b/native/src/zygisk/entry.cpp @@ -41,7 +41,7 @@ extern "C" void unload_first_stage() { } extern "C" void zygisk_inject_entry(void *handle) { - rust::zygisk_entry(); + zygisk_logging(); ZLOGD("load success\n"); char *ld = getenv("LD_PRELOAD"); @@ -62,47 +62,6 @@ extern "C" void zygisk_inject_entry(void *handle) { // The following code runs in zygote/app process -int zygisk_logd = -1; - -extern "C" int zygisk_fetch_logd() { - // If we don't have the log pipe set, request magiskd for it. This could actually happen - // multiple times in the zygote daemon (parent process) because we had to close this - // file descriptor to prevent crashing. - // - // For some reason, zygote sanitizes and checks FDs *before* forking. This results in the fact - // that *every* time before zygote forks, it has to close all logging related FDs in order - // to pass FD checks, just to have it re-initialized immediately after any - // logging happens ¯\_(ツ)_/¯. - // - // To be consistent with this behavior, we also have to close the log pipe to magiskd - // to make zygote NOT crash if necessary. For nativeForkAndSpecialize, we can actually - // add this FD into fds_to_ignore to pass the check. For other cases, we accomplish this by - // hooking __android_log_close and closing it at the same time as the rest of logging FDs. - - if (zygisk_logd < 0) { - android_logging(); - if (int fd = zygisk_request(ZygiskRequest::GET_LOG_PIPE); fd >= 0) { - int log_pipe = -1; - if (read_int(fd) == 0) { - log_pipe = recv_fd(fd); - } - close(fd); - if (log_pipe >= 0) { - zygisk_logd = log_pipe; - // Only re-enable zygisk logging if success - zygisk_logging(); - } - } - } - - return zygisk_logd; -} - -extern "C" void zygisk_close_logd() { - close(zygisk_logd); - zygisk_logd = -1; -} - static inline bool should_load_modules(uint32_t flags) { return (flags & UNMOUNT_MASK) != UNMOUNT_MASK && (flags & PROCESS_IS_MAGISK_APP) != PROCESS_IS_MAGISK_APP; @@ -332,16 +291,6 @@ static void get_process_info(int client, const sock_cred *cred) { } } -static void send_log_pipe(int fd) { - int logd_fd = rust::get_magiskd().get_log_pipe(); - if (logd_fd >= 0) { - write_int(fd, 0); - send_fd(fd, logd_fd); - } else { - write_int(fd, 1); - } -} - static void get_moddir(int client) { int id = read_int(client); char buf[4096]; @@ -364,9 +313,6 @@ void zygisk_handler(int client, const sock_cred *cred) { case ZygiskRequest::GET_INFO: get_process_info(client, cred); break; - case ZygiskRequest::GET_LOG_PIPE: - send_log_pipe(client); - break; case ZygiskRequest::CONNECT_COMPANION: if (get_exe(cred->pid, buf, sizeof(buf))) { connect_companion(client, str_ends(buf, "64")); diff --git a/native/src/zygisk/hook.cpp b/native/src/zygisk/hook.cpp index 8b6ecacfa..591df092c 100644 --- a/native/src/zygisk/hook.cpp +++ b/native/src/zygisk/hook.cpp @@ -421,8 +421,8 @@ void HookContext::fork_pre() { // The dirfd will be closed once out of scope allowed_fds[dirfd(dir.get())] = false; // logd_fd should be handled separately - if (zygisk_logd >= 0) { - allowed_fds[zygisk_logd] = false; + if (int fd = zygisk_get_logd(); fd >= 0) { + allowed_fds[fd] = false; } } @@ -448,8 +448,8 @@ void HookContext::sanitize_fds() { auto &allowed_fds = *g_allowed_fds; if (can_exempt_fd()) { - if (zygisk_logd >= 0) { - exempted_fds.push_back(zygisk_logd); + if (int fd = zygisk_get_logd(); fd >= 0) { + exempted_fds.push_back(fd); } auto update_fd_array = [&](int old_len) -> jintArray { diff --git a/native/src/zygisk/zygisk.hpp b/native/src/zygisk/zygisk.hpp index 8acee474f..bfb5ab91d 100644 --- a/native/src/zygisk/zygisk.hpp +++ b/native/src/zygisk/zygisk.hpp @@ -15,7 +15,6 @@ namespace ZygiskRequest { enum : int { SETUP, GET_INFO, - GET_LOG_PIPE, CONNECT_COMPANION, GET_MODDIR, PASSTHROUGH, @@ -37,7 +36,7 @@ enum : int { #define HIJACK_BIN HIJACK_BIN32 #endif -extern int zygisk_logd; +extern "C" int zygisk_get_logd(); extern "C" void zygisk_close_logd(); // Unmap all pages matching the name