Minor code cleanup

native/src/base/files.rs

@@ -3,7 +3,7 @@ use crate::{
     cstr, errno, error, FsPath, FsPathBuf, LibcReturn, Utf8CStr, Utf8CStrBuf, Utf8CStrBufArr,
     Utf8CStrWrite,
 };
-use bytemuck::{bytes_of_mut, Pod};
+use bytemuck::{bytes_of, 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,
@@ -14,13 +14,11 @@ 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> {
@@ -123,6 +121,7 @@ impl<T: BufRead> BufReadExt for T {
 
 pub trait WriteExt {
     fn write_zeros(&mut self, len: usize) -> io::Result<()>;
+    fn write_pod<F: Pod>(&mut self, data: &F) -> io::Result<()>;
 }
 
 impl<T: Write> WriteExt for T {
@@ -135,6 +134,10 @@ impl<T: Write> WriteExt for T {
         }
         Ok(())
     }
+
+    fn write_pod<F: Pod>(&mut self, data: &F) -> io::Result<()> {
+        self.write_all(bytes_of(data))
+    }
 }
 
 pub struct FileAttr {
@@ -1030,32 +1033,3 @@ pub fn parse_mount_info(pid: &str) -> Vec<MountInfo> {
     }
     res
 }
-
-#[derive(Default, Clone)]
-pub enum SharedFd {
-    #[default]
-    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()))),
-        }
-    }
-}

native/src/core/daemon.rs

@@ -7,10 +7,9 @@ use crate::package::ManagerInfo;
 use base::libc::{O_CLOEXEC, O_RDONLY};
 use base::{
     cstr, info, libc, open_fd, BufReadExt, Directory, FsPath, FsPathBuf, LoggedResult, ReadExt,
-    Utf8CStr, Utf8CStrBufArr,
+    Utf8CStr, Utf8CStrBufArr, WriteExt,
 };
 use bit_set::BitSet;
-use bytemuck::bytes_of;
 use std::fs::File;
 use std::io;
 use std::io::{BufReader, Read, Write};
@@ -270,7 +269,7 @@ pub trait IpcWrite {
 
 impl<T: Write> IpcWrite for T {
     fn ipc_write_int(&mut self, val: i32) -> io::Result<()> {
-        self.write_all(bytes_of(&val))
+        self.write_pod(&val)
     }
 
     fn ipc_write_string(&mut self, val: &str) -> io::Result<()> {

native/src/core/logging.rs

@@ -6,8 +6,8 @@ use base::libc::{
     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,
+    const_format::concatcp, libc, raw_cstr, FsPathBuf, LogLevel, Logger, ReadExt, Utf8CStr,
+    Utf8CStrBuf, Utf8CStrBufArr, Utf8CStrWrite, WriteExt, LOGGER,
 };
 use bytemuck::{bytes_of, write_zeroes, Pod, Zeroable};
 use num_derive::{FromPrimitive, ToPrimitive};
@@ -18,10 +18,10 @@ use std::fmt::Write as FmtWrite;
 use std::fs::File;
 use std::io::{IoSlice, Read, Write};
 use std::mem::ManuallyDrop;
-use std::os::fd::{FromRawFd, OwnedFd, RawFd};
+use std::os::fd::{FromRawFd, RawFd};
 use std::ptr::null_mut;
 use std::sync::atomic::{AtomicI32, Ordering};
-use std::sync::Mutex;
+use std::sync::{Arc, Mutex};
 use std::time::{SystemTime, UNIX_EPOCH};
 use std::{fs, io};
 
@@ -114,7 +114,7 @@ struct LogMeta {
 
 const MAX_MSG_LEN: usize = PIPE_BUF - size_of::<LogMeta>();
 
-fn write_log_to_pipe(logd: &mut File, prio: i32, msg: &Utf8CStr) -> io::Result<usize> {
+fn write_log_to_pipe(mut logd: &File, prio: i32, msg: &Utf8CStr) -> io::Result<usize> {
     // Truncate message if needed
     let len = min(MAX_MSG_LEN, msg.len());
     let msg = &msg.as_bytes()[..len];
@@ -131,28 +131,27 @@ fn write_log_to_pipe(logd: &mut File, prio: i32, msg: &Utf8CStr) -> io::Result<u
     let result = logd.write_vectored(&[io1, io2]);
     if let Err(ref e) = result {
         let mut buf = Utf8CStrBufArr::default();
-        buf.write_fmt(format_args_nl!("Cannot write_log_to_pipe: {}", e))
+        buf.write_fmt(format_args!("Cannot write_log_to_pipe: {}", e))
             .ok();
         android_log_write(LogLevel::Error, &buf);
     }
     result
 }
 
-static MAGISK_LOGD_FD: Mutex<SharedFd> = Mutex::new(SharedFd::new());
+static MAGISK_LOGD_FD: Mutex<Option<Arc<File>>> = Mutex::new(None);
 
-fn with_logd_fd<F: FnOnce(&mut File) -> io::Result<()>>(f: F) {
+fn with_logd_fd<R, F: FnOnce(&File) -> io::Result<R>>(f: F) {
     let fd = MAGISK_LOGD_FD.lock().unwrap().clone();
-    // SAFETY: writing less than PIPE_BUF bytes is guaranteed to be atomic on Linux
-    if let Some(mut logd) = unsafe { fd.as_file() } {
-        if f(&mut logd).is_err() {
+    if let Some(logd) = fd {
+        if f(&logd).is_err() {
             // If any error occurs, shut down the logd pipe
-            *MAGISK_LOGD_FD.lock().unwrap() = SharedFd::default();
+            *MAGISK_LOGD_FD.lock().unwrap() = None;
         }
     }
 }
 
 fn magisk_log_to_pipe(prio: i32, msg: &Utf8CStr) {
-    with_logd_fd(|logd| write_log_to_pipe(logd, prio, msg).map(|_| ()));
+    with_logd_fd(|logd| write_log_to_pipe(logd, prio, msg));
 }
 
 // SAFETY: zygisk client code runs single threaded, so no need to prevent data race
@@ -217,8 +216,8 @@ fn zygisk_log_to_pipe(prio: i32, msg: &Utf8CStr) {
         pthread_sigmask(SIG_BLOCK, &mask, &mut orig_mask);
     }
 
-    let mut logd = ManuallyDrop::new(unsafe { File::from_raw_fd(fd) });
-    let result = write_log_to_pipe(&mut logd, prio, msg);
+    let logd = ManuallyDrop::new(unsafe { File::from_raw_fd(fd) });
+    let result = write_log_to_pipe(&logd, prio, msg);
 
     // Consume SIGPIPE if exists, then restore mask
     unsafe {
@@ -347,19 +346,19 @@ 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
-    *MAGISK_LOGD_FD.lock().unwrap() = SharedFd::default();
+    *MAGISK_LOGD_FD.lock().unwrap() = None;
     null_mut()
 }
 
 pub fn setup_logfile() {
-    with_logd_fd(|logd| {
+    with_logd_fd(|mut logd| {
         let meta = LogMeta {
             prio: -1,
             len: 0,
             pid: 0,
             tid: 0,
         };
-        logd.write_all(bytes_of(&meta))
+        (&mut logd).write_pod(&meta)
     });
 }
 
@@ -374,7 +373,7 @@ pub fn start_log_daemon() {
         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));
+        *MAGISK_LOGD_FD.lock().unwrap() = Some(Arc::new(File::from_raw_fd(write)));
         new_daemon_thread(logfile_writer, read as *mut c_void);
     }
 }