Rewrite magisk logging implementation

native/src/base/files.rs

@@ -1,27 +1,27 @@
-use mem::MaybeUninit;
-use std::cmp::min;
-use std::ffi::CStr;
-use std::fs::File;
-use std::io::{BufRead, BufReader, Read, Seek, SeekFrom, Write};
-use std::ops::Deref;
-use std::os::fd::{AsFd, BorrowedFd, IntoRawFd};
-use std::os::unix::ffi::OsStrExt;
-use std::os::unix::io::{AsRawFd, FromRawFd, OwnedFd, RawFd};
-use std::path::Path;
-use std::{io, mem, ptr, slice};
-
-use bytemuck::{bytes_of_mut, Pod};
-use libc::{
-    c_uint, dirent, makedev, mode_t, EEXIST, ENOENT, F_OK, O_CLOEXEC, O_CREAT, O_PATH, O_RDONLY,
-    O_RDWR, O_TRUNC, O_WRONLY,
-};
-use num_traits::AsPrimitive;
-
 use crate::cxx_extern::readlinkat_for_cxx;
 use crate::{
     cstr, errno, error, FsPath, FsPathBuf, LibcReturn, Utf8CStr, Utf8CStrBuf, Utf8CStrBufArr,
     Utf8CStrWrite,
 };
+use bytemuck::{bytes_of_mut, Pod};
+use libc::{
+    c_uint, dirent, makedev, mode_t, EEXIST, ENOENT, F_OK, O_CLOEXEC, O_CREAT, O_PATH, O_RDONLY,
+    O_RDWR, O_TRUNC, O_WRONLY,
+};
+use mem::MaybeUninit;
+use num_traits::AsPrimitive;
+use std::cmp::min;
+use std::ffi::CStr;
+use std::fs::File;
+use std::io::{BufRead, BufReader, Read, Seek, SeekFrom, Write};
+use std::mem::ManuallyDrop;
+use std::ops::Deref;
+use std::os::fd::{AsFd, BorrowedFd, IntoRawFd};
+use std::os::unix::ffi::OsStrExt;
+use std::os::unix::io::{AsRawFd, FromRawFd, OwnedFd, RawFd};
+use std::path::Path;
+use std::sync::Arc;
+use std::{io, mem, ptr, slice};
 
 pub fn __open_fd_impl(path: &Utf8CStr, flags: i32, mode: mode_t) -> io::Result<OwnedFd> {
     unsafe {
@@ -1030,3 +1030,37 @@ pub fn parse_mount_info(pid: &str) -> Vec<MountInfo> {
     }
     res
 }
+
+#[derive(Clone)]
+pub enum SharedFd {
+    None,
+    Shared(Arc<OwnedFd>),
+}
+
+impl From<OwnedFd> for SharedFd {
+    fn from(fd: OwnedFd) -> Self {
+        SharedFd::Shared(Arc::new(fd))
+    }
+}
+
+impl SharedFd {
+    pub const fn new() -> Self {
+        SharedFd::None
+    }
+
+    // This is unsafe because we cannot create multiple mutable references to the same fd.
+    // This can only be safely used if and only if the underlying fd points to a pipe,
+    // and the read/write operations performed on the file involves bytes less than PIPE_BUF.
+    pub unsafe fn as_file(&self) -> Option<ManuallyDrop<File>> {
+        match self {
+            SharedFd::None => None,
+            SharedFd::Shared(arc) => Some(ManuallyDrop::new(File::from_raw_fd(arc.as_raw_fd()))),
+        }
+    }
+}
+
+impl Default for SharedFd {
+    fn default() -> Self {
+        SharedFd::None
+    }
+}

native/src/core/daemon.cpp

@@ -183,7 +183,7 @@ static void handle_request_sync(int client, int code) {
         write_int(client, MAGISK_VER_CODE);
         break;
     case +RequestCode::START_DAEMON:
-        MagiskD().setup_logfile();
+        setup_logfile();
         break;
     case +RequestCode::STOP_DAEMON: {
         // Unmount all overlays

native/src/core/daemon.rs

@@ -2,7 +2,7 @@ use crate::consts::{MAGISK_FULL_VER, MAIN_CONFIG};
 use crate::db::Sqlite3;
 use crate::ffi::{get_magisk_tmp, RequestCode};
 use crate::get_prop;
-use crate::logging::magisk_logging;
+use crate::logging::{magisk_logging, start_log_daemon};
 use base::libc::{O_CLOEXEC, O_RDONLY};
 use base::{
     cstr, info, libc, open_fd, BufReadExt, Directory, FsPath, FsPathBuf, ReadExt, ResultExt,
@@ -41,7 +41,6 @@ impl BootStateFlags {
 
 #[derive(Default)]
 pub struct MagiskD {
-    pub logd: Mutex<Option<File>>,
     pub sql_connection: Mutex<Option<Sqlite3>>,
     boot_stage_lock: Mutex<BootStateFlags>,
     sdk_int: i32,
@@ -96,6 +95,10 @@ impl MagiskD {
 }
 
 pub fn daemon_entry() {
+    start_log_daemon();
+    magisk_logging();
+    info!("Magisk {} daemon started", MAGISK_FULL_VER);
+
     let is_emulator = get_prop(cstr!("ro.kernel.qemu"), false) == "1"
         || get_prop(cstr!("ro.boot.qemu"), false) == "1"
         || get_prop(cstr!("ro.product.device"), false).contains("vsoc");
@@ -134,6 +137,7 @@ pub fn daemon_entry() {
             .parse::<i32>()
             .unwrap_or(-1);
     }
+    info!("* Device API level: {}", sdk_int);
 
     let magiskd = MagiskD {
         sdk_int,
@@ -141,12 +145,7 @@ pub fn daemon_entry() {
         is_recovery,
         ..Default::default()
     };
-    magiskd.start_log_daemon();
     MAGISKD.set(magiskd).ok();
-    magisk_logging();
-
-    info!("Magisk {} daemon started", MAGISK_FULL_VER);
-    info!("* Device API level: {}", sdk_int);
 }
 
 fn check_data() -> bool {

native/src/core/lib.rs

@@ -8,9 +8,7 @@ use base::Utf8CStr;
 use cert::read_certificate;
 use daemon::{daemon_entry, find_apk_path, get_magiskd, MagiskD};
 use db::get_default_db_settings;
-use logging::{
+use logging::{android_logging, setup_logfile, zygisk_close_logd, zygisk_get_logd, zygisk_logging};
-    android_logging, magisk_logging, zygisk_close_logd, zygisk_get_logd, zygisk_logging,
-};
 use mount::{clean_mounts, find_preinit_device, revert_unmount, setup_mounts};
 use resetprop::{persist_delete_prop, persist_get_prop, persist_get_props, persist_set_prop};
 use su::get_default_root_settings;
@@ -158,10 +156,10 @@ pub mod ffi {
     extern "Rust" {
         fn rust_test_entry();
         fn android_logging();
-        fn magisk_logging();
         fn zygisk_logging();
         fn zygisk_close_logd();
         fn zygisk_get_logd() -> i32;
+        fn setup_logfile();
         fn find_apk_path(pkg: Utf8CStrRef, data: &mut [u8]) -> usize;
         fn read_certificate(fd: i32, version: i32) -> Vec<u8>;
         fn setup_mounts();
@@ -180,7 +178,6 @@ pub mod ffi {
     // FFI for MagiskD
     extern "Rust" {
         type MagiskD;
-        fn setup_logfile(&self);
         fn is_recovery(&self) -> bool;
         fn sdk_int(&self) -> i32;
         fn boot_stage_handler(&self, client: i32, code: i32);

native/src/core/logging.rs

@@ -1,32 +1,30 @@
+use crate::consts::{LOGFILE, LOG_PIPE};
+use crate::ffi::get_magisk_tmp;
+use crate::logging::LogFile::{Actual, Buffer};
+use base::libc::{
+    getpid, gettid, localtime_r, pthread_sigmask, sigaddset, sigset_t, sigtimedwait, time_t,
+    timespec, tm, O_CLOEXEC, O_RDWR, O_WRONLY, PIPE_BUF, SIGPIPE, SIG_BLOCK, SIG_SETMASK,
+};
+use base::{
+    const_format::concatcp, libc, raw_cstr, FsPathBuf, LogLevel, Logger, ReadExt, SharedFd,
+    Utf8CStr, Utf8CStrBuf, Utf8CStrBufArr, Utf8CStrWrite, LOGGER,
+};
+use bytemuck::{bytes_of, write_zeroes, Pod, Zeroable};
+use num_derive::{FromPrimitive, ToPrimitive};
+use num_traits::FromPrimitive;
 use std::cmp::min;
 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::{FromRawFd, RawFd};
+use std::mem::ManuallyDrop;
+use std::os::fd::{FromRawFd, OwnedFd, RawFd};
 use std::ptr::null_mut;
 use std::sync::atomic::{AtomicI32, Ordering};
+use std::sync::Mutex;
+use std::time::{SystemTime, UNIX_EPOCH};
 use std::{fs, io};
 
-use bytemuck::{bytes_of, bytes_of_mut, write_zeroes, Pod, Zeroable};
-use num_derive::{FromPrimitive, ToPrimitive};
-use num_traits::FromPrimitive;
-
-use base::libc::{
-    clock_gettime, getpid, gettid, localtime_r, pthread_sigmask, sigaddset, sigset_t, sigtimedwait,
-    timespec, tm, CLOCK_REALTIME, O_CLOEXEC, O_RDWR, O_WRONLY, PIPE_BUF, SIGPIPE, SIG_BLOCK,
-    SIG_SETMASK,
-};
-use base::{
-    const_format::concatcp, exit_on_error, libc, raw_cstr, FsPathBuf, LogLevel, Logger, Utf8CStr,
-    Utf8CStrBuf, Utf8CStrBufArr, Utf8CStrWrite, LOGGER,
-};
-
-use crate::consts::{LOGFILE, LOG_PIPE};
-use crate::daemon::{MagiskD, MAGISKD};
-use crate::ffi::get_magisk_tmp;
-use crate::logging::LogFile::{Actual, Buffer};
-
 #[allow(dead_code, non_camel_case_types)]
 #[derive(FromPrimitive, ToPrimitive)]
 #[repr(i32)]
@@ -59,16 +57,17 @@ fn level_to_prio(level: LogLevel) -> i32 {
     }
 }
 
-pub fn android_logging() {
+fn android_log_write(level: LogLevel, msg: &Utf8CStr) {
-    fn android_log_write(level: LogLevel, msg: &Utf8CStr) {
+    unsafe {
-        write_android_log(level_to_prio(level), msg);
+        __android_log_write(level_to_prio(level), raw_cstr!("Magisk"), msg.as_ptr());
     }
+}
 
+pub fn android_logging() {
     let logger = Logger {
         write: android_log_write,
         flags: 0,
     };
-    exit_on_error(false);
     unsafe {
         LOGGER = logger;
     }
@@ -76,7 +75,7 @@ pub fn android_logging() {
 
 pub fn magisk_logging() {
     fn magisk_log_write(level: LogLevel, msg: &Utf8CStr) {
-        write_android_log(level_to_prio(level), msg);
+        android_log_write(level, msg);
         magisk_log_to_pipe(level_to_prio(level), msg);
     }
 
@@ -84,7 +83,6 @@ pub fn magisk_logging() {
         write: magisk_log_write,
         flags: 0,
     };
-    exit_on_error(false);
     unsafe {
         LOGGER = logger;
     }
@@ -92,7 +90,7 @@ pub fn magisk_logging() {
 
 pub fn zygisk_logging() {
     fn zygisk_log_write(level: LogLevel, msg: &Utf8CStr) {
-        write_android_log(level_to_prio(level), msg);
+        android_log_write(level, msg);
         zygisk_log_to_pipe(level_to_prio(level), msg);
     }
 
@@ -100,7 +98,6 @@ pub fn zygisk_logging() {
         write: zygisk_log_write,
         flags: 0,
     };
-    exit_on_error(false);
     unsafe {
         LOGGER = logger;
     }
@@ -115,13 +112,7 @@ struct LogMeta {
     tid: i32,
 }
 
-fn write_android_log(prio: i32, msg: &Utf8CStr) {
+const MAX_MSG_LEN: usize = PIPE_BUF - size_of::<LogMeta>();
-    unsafe {
-        __android_log_write(prio, raw_cstr!("Magisk"), msg.as_ptr());
-    }
-}
-
-const MAX_MSG_LEN: usize = PIPE_BUF - std::mem::size_of::<LogMeta>();
 
 fn write_log_to_pipe(logd: &mut File, prio: i32, msg: &Utf8CStr) -> io::Result<usize> {
     // Truncate message if needed
@@ -138,35 +129,36 @@ fn write_log_to_pipe(logd: &mut File, prio: i32, msg: &Utf8CStr) -> io::Result<u
     let io1 = IoSlice::new(bytes_of(&meta));
     let io2 = IoSlice::new(msg);
     let result = logd.write_vectored(&[io1, io2]);
-    if let Err(e) = result.as_ref() {
+    if let Err(ref e) = result {
         let mut buf = Utf8CStrBufArr::default();
         buf.write_fmt(format_args_nl!("Cannot write_log_to_pipe: {}", e))
             .ok();
-        write_android_log(level_to_prio(LogLevel::Error), &buf);
+        android_log_write(LogLevel::Error, &buf);
     }
     result
 }
 
-fn magisk_log_to_pipe(prio: i32, msg: &Utf8CStr) {
+static MAGISK_LOGD_FD: Mutex<SharedFd> = Mutex::new(SharedFd::new());
-    let magiskd = match MAGISKD.get() {
+
+fn with_logd_fd<F: FnOnce(&mut File) -> io::Result<()>>(f: F) {
+    let fd = MAGISK_LOGD_FD.lock().unwrap().clone();
+    // SAFETY: writing less than PIPE_BUF bytes is guaranteed to be atomic on Linux
+    match unsafe { fd.as_file() } {
         None => return,
-        Some(s) => s,
+        Some(mut logd) => {
-    };
+            if f(&mut logd).is_err() {
-
+                // If any error occurs, shut down the logd pipe
-    let mut guard = magiskd.logd.lock().unwrap();
+                *MAGISK_LOGD_FD.lock().unwrap() = SharedFd::default();
-    let logd = match guard.as_mut() {
+            }
-        None => return,
+        }
-        Some(s) => s,
-    };
-
-    let result = write_log_to_pipe(logd, prio, msg);
-
-    // If any error occurs, shut down the logd pipe
-    if result.is_err() {
-        *guard = None;
     }
 }
 
+fn magisk_log_to_pipe(prio: i32, msg: &Utf8CStr) {
+    with_logd_fd(|logd| write_log_to_pipe(logd, prio, msg).map(|_| ()));
+}
+
+// SAFETY: zygisk client code runs single threaded, so no need to prevent data race
 static ZYGISK_LOGD: AtomicI32 = AtomicI32::new(-1);
 
 pub fn zygisk_close_logd() {
@@ -228,13 +220,8 @@ fn zygisk_log_to_pipe(prio: i32, msg: &Utf8CStr) {
         pthread_sigmask(SIG_BLOCK, &mask, &mut orig_mask);
     }
 
-    let result = {
+    let mut logd = ManuallyDrop::new(unsafe { File::from_raw_fd(fd) });
-        let mut logd = unsafe { File::from_raw_fd(fd) };
+    let result = write_log_to_pipe(&mut logd, prio, msg);
-        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 {
@@ -251,12 +238,12 @@ fn zygisk_log_to_pipe(prio: i32, msg: &Utf8CStr) {
 
 // The following is implementation for the logging daemon
 
-enum LogFile<'a> {
+enum LogFile {
-    Buffer(&'a mut Vec<u8>),
+    Buffer(Vec<u8>),
     Actual(File),
 }
 
-impl Write for LogFile<'_> {
+impl Write for LogFile {
     fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
         match self {
             Buffer(e) => e.write(buf),
@@ -282,8 +269,7 @@ impl Write for LogFile<'_> {
 extern "C" fn logfile_writer(arg: *mut c_void) -> *mut c_void {
     fn writer_loop(pipefd: RawFd) -> io::Result<()> {
         let mut pipe = unsafe { File::from_raw_fd(pipefd) };
-        let mut tmp = Vec::new();
+        let mut logfile: LogFile = Buffer(Vec::new());
-        let mut logfile: LogFile = Buffer(&mut tmp);
 
         let mut meta = LogMeta::zeroed();
         let mut msg_buf = [0u8; MAX_MSG_LEN];
@@ -292,14 +278,13 @@ extern "C" fn logfile_writer(arg: *mut c_void) -> *mut c_void {
         loop {
             // Read request
             write_zeroes(&mut meta);
-            pipe.read_exact(bytes_of_mut(&mut meta))?;
+            pipe.read_pod(&mut meta)?;
 
             if meta.prio < 0 {
-                if matches!(logfile, Buffer(_)) {
+                if let Buffer(ref mut buf) = logfile {
                     fs::rename(LOGFILE, concatcp!(LOGFILE, ".bak")).ok();
                     let mut out = File::create(LOGFILE)?;
-                    out.write_all(tmp.as_slice())?;
+                    out.write_all(buf.as_slice())?;
-                    tmp = Vec::new();
                     logfile = Actual(out);
                 }
                 continue;
@@ -327,15 +312,15 @@ extern "C" fn logfile_writer(arg: *mut c_void) -> *mut c_void {
                 _ => continue,
             };
 
+            let now = SystemTime::now().duration_since(UNIX_EPOCH).unwrap();
+
             // Note: the obvious better implementation is to use the rust chrono crate, however
             // the crate cannot fetch the proper local timezone without pulling in a bunch of
             // timezone handling code. To reduce binary size, fallback to use localtime_r in libc.
             unsafe {
-                let mut ts: timespec = std::mem::zeroed();
+                let secs: time_t = now.as_secs() as time_t;
                 let mut tm: tm = std::mem::zeroed();
-                if clock_gettime(CLOCK_REALTIME, &mut ts) < 0
+                if localtime_r(&secs, &mut tm).is_null() {
-                    || localtime_r(&ts.tv_sec, &mut tm).is_null()
-                {
                     continue;
                 }
                 let len = strftime(
@@ -345,15 +330,17 @@ extern "C" fn logfile_writer(arg: *mut c_void) -> *mut c_void {
                     &tm,
                 );
                 aux.set_len(len);
-                let ms = ts.tv_nsec / 1000000;
                 aux.write_fmt(format_args!(
                     ".{:03} {:5} {:5} {} : ",
-                    ms, meta.pid, meta.tid, prio
+                    now.subsec_millis(),
+                    meta.pid,
+                    meta.tid,
+                    prio
                 ))
                 .ok();
             }
 
-            let io1 = IoSlice::new(aux.as_bytes_with_nul());
+            let io1 = IoSlice::new(aux.as_bytes());
             let io2 = IoSlice::new(msg);
             // We don't need to care the written len because we are writing less than PIPE_BUF
             // It's guaranteed to always write the whole thing atomically
@@ -363,47 +350,34 @@ extern "C" fn logfile_writer(arg: *mut c_void) -> *mut c_void {
 
     writer_loop(arg as RawFd).ok();
     // If any error occurs, shut down the logd pipe
-    if let Some(magiskd) = MAGISKD.get() {
+    *MAGISK_LOGD_FD.lock().unwrap() = SharedFd::default();
-        magiskd.close_log_pipe();
-    }
     null_mut()
 }
 
-impl MagiskD {
+pub fn setup_logfile() {
-    pub fn start_log_daemon(&self) {
+    with_logd_fd(|logd| {
-        let mut buf = Utf8CStrBufArr::default();
-        let path = FsPathBuf::new(&mut buf)
-            .join(get_magisk_tmp())
-            .join(LOG_PIPE);
-
-        unsafe {
-            libc::mkfifo(path.as_ptr(), 0o666);
-            libc::chown(path.as_ptr(), 0, 0);
-            let read = libc::open(path.as_ptr(), O_RDWR | O_CLOEXEC);
-            let write = libc::open(path.as_ptr(), O_WRONLY | O_CLOEXEC);
-            *self.logd.lock().unwrap() = Some(File::from_raw_fd(write));
-            new_daemon_thread(logfile_writer, read as *mut c_void);
-        }
-    }
-
-    pub fn close_log_pipe(&self) {
-        *self.logd.lock().unwrap() = None;
-    }
-
-    pub fn setup_logfile(&self) {
-        let mut guard = self.logd.lock().unwrap();
-        let logd = match guard.as_mut() {
-            None => return,
-            Some(s) => s,
-        };
-
         let meta = LogMeta {
             prio: -1,
             len: 0,
             pid: 0,
             tid: 0,
         };
+        logd.write_all(bytes_of(&meta))
+    });
+}
 
-        logd.write_all(bytes_of(&meta)).ok();
+pub fn start_log_daemon() {
+    let mut buf = Utf8CStrBufArr::default();
+    let path = FsPathBuf::new(&mut buf)
+        .join(get_magisk_tmp())
+        .join(LOG_PIPE);
+
+    unsafe {
+        libc::mkfifo(path.as_ptr(), 0o666);
+        libc::chown(path.as_ptr(), 0, 0);
+        let read = libc::open(path.as_ptr(), O_RDWR | O_CLOEXEC);
+        let write = libc::open(path.as_ptr(), O_WRONLY | O_CLOEXEC);
+        *MAGISK_LOGD_FD.lock().unwrap() = SharedFd::from(OwnedFd::from_raw_fd(write));
+        new_daemon_thread(logfile_writer, read as *mut c_void);
     }
 }

native/src/init/logging.rs

@@ -2,7 +2,7 @@ use base::libc::{
     makedev, mknod, syscall, SYS_dup3, O_CLOEXEC, O_RDWR, O_WRONLY, STDERR_FILENO, STDIN_FILENO,
     STDOUT_FILENO, S_IFCHR,
 };
-use base::{cstr, exit_on_error, open_fd, raw_cstr, FsPath, LogLevel, Logger, Utf8CStr, LOGGER};
+use base::{cstr, open_fd, raw_cstr, FsPath, LogLevel, Logger, Utf8CStr, LOGGER};
 use std::fs::File;
 use std::io::{IoSlice, Write};
 use std::mem;
@@ -60,7 +60,6 @@ pub fn setup_klog() {
         write: kmsg_log_write,
         flags: 0,
     };
-    exit_on_error(false);
     unsafe {
         LOGGER = logger;
     }