Fix file attribute copy in module mounting logic

Due to various reasons, we cannot directly mount module files in /data
into the real paths. Instead we bind mount the module root directory and
remount this mirror with specific mount-point flags. Relevant to this
bug, the module mount is mounted as read-only, which means the file
attribute copy operation could fail in certain configurations.

The fix here is to always copy file attributes into writable locations,
so either in the tmpfs worker directory, or in the module directory
under /data.

A new test case is added to make sure this regression will no longer
happen again in the future.

Fix #9139

native/src/core/module.rs

@@ -1,4 +1,4 @@
-use crate::consts::{MODULEMNT, WORKERDIR};
+use crate::consts::{MODULEMNT, MODULEROOT, WORKERDIR};
 use crate::ffi::{ModuleInfo, get_magisk_tmp};
 use crate::load_prop_file;
 use base::{
@@ -44,55 +44,85 @@ fn mount_dummy(reason: &str, src: &Utf8CStr, dest: &Utf8CStr, is_dir: bool) -> O
     bind_mount(reason, src, dest, false)
 }
 
-// File paths that act like a stack, popping out the last element
+// File path that act like a stack, popping out the last element
 // automatically when out of scope. Using Rust's lifetime mechanism,
 // we can ensure the buffer will never be incorrectly copied or modified.
-// After calling append or clone, the mutable reference's lifetime is
+// After calling append or reborrow, the mutable reference's lifetime is
 // "transferred" to the returned object, and the compiler will guarantee
 // that the original mutable reference can only be reused if and only if
 // the newly created instance is destroyed.
 struct PathTracker<'a> {
-    real: &'a mut dyn Utf8CStrBuf,
+    path: &'a mut dyn Utf8CStrBuf,
-    tmp: &'a mut dyn Utf8CStrBuf,
+    len: usize,
-    real_len: usize,
-    tmp_len: usize,
 }
 
 impl PathTracker<'_> {
-    fn from<'a>(real: &'a mut dyn Utf8CStrBuf, tmp: &'a mut dyn Utf8CStrBuf) -> PathTracker<'a> {
+    fn from<'a>(path: &'a mut dyn Utf8CStrBuf) -> PathTracker<'a> {
-        let real_len = real.len();
+        let len = path.len();
-        let tmp_len = tmp.len();
+        PathTracker { path, len }
-        PathTracker {
-            real,
-            tmp,
-            real_len,
-            tmp_len,
-        }
     }
 
     fn append(&mut self, name: &str) -> PathTracker {
-        let real_len = self.real.len();
+        let len = self.path.len();
-        let tmp_len = self.tmp.len();
+        self.path.append_path(name);
-        self.real.append_path(name);
-        self.tmp.append_path(name);
         PathTracker {
-            real: self.real,
+            path: self.path,
-            tmp: self.tmp,
+            len,
-            real_len,
-            tmp_len,
         }
     }
 
-    fn clone(&mut self) -> PathTracker {
+    fn reborrow(&mut self) -> PathTracker {
-        Self::from(self.real, self.tmp)
+        Self::from(self.path)
     }
 }
 
 impl Drop for PathTracker<'_> {
     // Revert back to the original state after finish using the buffer
     fn drop(&mut self) {
-        self.real.truncate(self.real_len);
+        self.path.truncate(self.len);
-        self.tmp.truncate(self.tmp_len);
+    }
+}
+
+struct FilePaths<'a> {
+    real: PathTracker<'a>,
+    worker: PathTracker<'a>,
+    module_mnt: PathTracker<'a>,
+    module_root: PathTracker<'a>,
+}
+
+impl FilePaths<'_> {
+    fn append(&mut self, name: &str) -> FilePaths {
+        FilePaths {
+            real: self.real.append(name),
+            worker: self.worker.append(name),
+            module_mnt: self.module_mnt.append(name),
+            module_root: self.module_root.append(name),
+        }
+    }
+
+    fn reborrow(&mut self) -> FilePaths {
+        FilePaths {
+            real: self.real.reborrow(),
+            worker: self.worker.reborrow(),
+            module_mnt: self.module_mnt.reborrow(),
+            module_root: self.module_root.reborrow(),
+        }
+    }
+
+    fn real(&self) -> &Utf8CStr {
+        self.real.path
+    }
+
+    fn worker(&self) -> &Utf8CStr {
+        self.worker.path
+    }
+
+    fn module_mnt(&self) -> &Utf8CStr {
+        self.module_mnt.path
+    }
+
+    fn module(&self) -> &Utf8CStr {
+        self.module_root.path
     }
 }
 
@@ -117,21 +147,20 @@ impl FsNode {
         }
     }
 
-    fn build_from_path(&mut self, path: &mut dyn Utf8CStrBuf) -> LoggedResult<()> {
+    fn collect(&mut self, mut paths: FilePaths) -> LoggedResult<()> {
         let FsNode::Directory { children } = self else {
             return Ok(());
         };
-        let mut dir = Directory::open(path)?;
+        let mut dir = Directory::open(paths.module())?;
-        let path_len = path.len();
 
         while let Some(entry) = dir.read()? {
-            path.truncate(path_len);
+            let entry_paths = paths.append(entry.name());
-            path.append_path(entry.name());
+            let path = entry_paths.module();
             if entry.is_dir() {
                 let node = children
                     .entry(entry.name().to_string())
                     .or_insert_with(FsNode::new_dir);
-                node.build_from_path(path)?;
+                node.collect(entry_paths)?;
             } else if entry.is_symlink() {
                 let mut link = cstr::buf::default();
                 path.read_link(&mut link)?;
@@ -151,10 +180,14 @@ impl FsNode {
                         continue;
                     }
                 }
+                if entry_paths.real().exists() {
+                    clone_attr(entry_paths.real(), path)?;
+                }
                 children
                     .entry(entry.name().to_string())
                     .or_insert_with(|| FsNode::File {
-                        src: path.to_owned(),
+                        // Make sure to mount from module_mnt, not module
+                        src: entry_paths.module_mnt().to_owned(),
                     });
             }
         }
@@ -186,7 +219,7 @@ impl FsNode {
         }
     }
 
-    fn commit(&mut self, mut path: PathTracker, is_root_dir: bool) -> LoggedResult<()> {
+    fn commit(&mut self, mut path: FilePaths, is_root_dir: bool) -> LoggedResult<()> {
         match self {
             FsNode::Directory { children } => {
                 let mut is_tmpfs = false;
@@ -195,7 +228,7 @@ impl FsNode {
                 children.retain(|name, node| {
                     if name == ".replace" {
                         return if is_root_dir {
-                            warn!("Unable to replace '{}', ignore request", path.real);
+                            warn!("Unable to replace '{}', ignore request", path.real());
                             false
                         } else {
                             is_tmpfs = true;
@@ -204,10 +237,10 @@ impl FsNode {
                     }
 
                     let path = path.append(name);
-                    if node.parent_should_be_tmpfs(path.real) {
+                    if node.parent_should_be_tmpfs(path.real()) {
                         if is_root_dir {
                             // Ignore the unsupported child node
-                            warn!("Unable to add '{}', skipped", path.real);
+                            warn!("Unable to add '{}', skipped", path.real());
                             return false;
                         }
                         is_tmpfs = true;
@@ -216,10 +249,10 @@ impl FsNode {
                 });
 
                 if is_tmpfs {
-                    self.commit_tmpfs(path.clone())?;
+                    self.commit_tmpfs(path.reborrow())?;
                     // Transitioning from non-tmpfs to tmpfs, we need to actually mount the
                     // worker dir to dest after all children are committed.
-                    bind_mount("move", path.tmp, path.real, true)?;
+                    bind_mount("move", path.worker(), path.real(), true)?;
                 } else {
                     for (name, node) in children {
                         let path = path.append(name);
@@ -228,26 +261,25 @@ impl FsNode {
                 }
             }
             FsNode::File { src } => {
-                clone_attr(path.real, src)?;
+                bind_mount("mount", src, path.real(), false)?;
-                bind_mount("mount", src, path.real, false)?;
             }
             FsNode::Symlink { .. } | FsNode::Whiteout => {
-                error!("Unable to handle '{}': parent should be tmpfs", path.real);
+                error!("Unable to handle '{}': parent should be tmpfs", path.real());
             }
         }
 
         Ok(())
     }
 
-    fn commit_tmpfs(&mut self, mut path: PathTracker) -> LoggedResult<()> {
+    fn commit_tmpfs(&mut self, mut path: FilePaths) -> LoggedResult<()> {
         match self {
             FsNode::Directory { children } => {
-                path.tmp.mkdirs(0o000)?;
+                path.worker().mkdirs(0o000)?;
-                if path.real.exists() {
+                if path.real().exists() {
-                    clone_attr(path.real, path.tmp)?;
+                    clone_attr(path.real(), path.worker())?;
-                } else if let Some(p) = path.tmp.parent_dir() {
+                } else if let Some(p) = path.worker().parent_dir() {
                     let parent = Utf8CString::from(p);
-                    clone_attr(&parent, path.tmp)?;
+                    clone_attr(&parent, path.worker())?;
                 }
 
                 // Check whether a file name '.replace' exist
@@ -262,7 +294,7 @@ impl FsNode {
                                 bind_mount(
                                     "mount",
                                     &src,
-                                    path.real,
+                                    path.real(),
                                     matches!(node, FsNode::Directory { .. }),
                                 )?;
                             }
@@ -275,7 +307,7 @@ impl FsNode {
                 }
 
                 // Traverse the real directory and mount mirrors
-                if let Ok(mut dir) = Directory::open(path.real) {
+                if let Ok(mut dir) = Directory::open(path.real()) {
                     while let Ok(Some(entry)) = dir.read() {
                         if children.contains_key(entry.name().as_str()) {
                             // Should not be mirrored, next
@@ -296,7 +328,7 @@ impl FsNode {
                                 },
                             );
                         } else {
-                            mount_dummy("mirror", path.real, path.tmp, entry.is_dir())?;
+                            mount_dummy("mirror", path.real(), path.worker(), entry.is_dir())?;
                         }
                     }
                 }
@@ -308,24 +340,21 @@ impl FsNode {
                 }
             }
             FsNode::File { src } => {
-                if path.real.exists() {
+                mount_dummy("mount", src, path.worker(), false)?;
-                    clone_attr(path.real, src)?;
-                }
-                mount_dummy("mount", src, path.tmp, false)?;
             }
             FsNode::Symlink {
                 target,
                 is_magisk_bin,
             } => {
-                module_log!("mklink", path.tmp, target);
+                module_log!("mklink", path.worker(), target);
-                path.tmp.create_symlink_to(target)?;
+                path.worker().create_symlink_to(target)?;
-                // Avoid cloneing existing su attributes to our su
+                // Avoid cloning existing su attributes to our su
-                if !*is_magisk_bin && path.real.exists() {
+                if !*is_magisk_bin && path.real().exists() {
-                    clone_attr(path.real, path.tmp)?;
+                    clone_attr(path.real(), path.worker())?;
                 }
             }
             FsNode::Whiteout => {
-                module_log!("delete", path.real, "null");
+                module_log!("delete", path.real(), "null");
             }
         }
         Ok(())
@@ -535,40 +564,56 @@ fn inject_zygisk_bins(system: &mut FsNode, name: &str) {
 pub fn load_modules(module_list: &[ModuleInfo], zygisk_name: &str) {
     let mut system = FsNode::new_dir();
 
+    // Build all the base "prefix" paths
+    let mut root = cstr::buf::default().join_path("/");
+
+    let mut module_dir = cstr::buf::default().join_path(MODULEROOT);
+
+    let mut module_mnt = cstr::buf::default()
+        .join_path(get_magisk_tmp())
+        .join_path(MODULEMNT);
+
+    let mut worker = cstr::buf::default()
+        .join_path(get_magisk_tmp())
+        .join_path(WORKERDIR);
+
+    // Create a collection of all relevant paths
+    let mut root_paths = FilePaths {
+        real: PathTracker::from(&mut root),
+        worker: PathTracker::from(&mut worker),
+        module_mnt: PathTracker::from(&mut module_mnt),
+        module_root: PathTracker::from(&mut module_dir),
+    };
+
     // Step 1: Create virtual filesystem tree
     //
     // In this step, there is zero logic applied during tree construction; we simply collect
     // and record the union of all module filesystem trees under each of their /system directory.
 
-    let mut path = cstr::buf::default()
-        .join_path(get_magisk_tmp())
-        .join_path(MODULEMNT);
-    let len = path.len();
     for info in module_list {
-        path.truncate(len);
+        let mut module_paths = root_paths.append(&info.name);
-        path.append_path(&info.name);
+        {
-
             // Read props
-        let module_path_len = path.len();
+            let prop = module_paths.append("system.prop");
-        path.append_path("system.prop");
+            if prop.module().exists() {
-        if path.exists() {
                 // Do NOT go through property service as it could cause boot lock
-            load_prop_file(&path, true);
+                load_prop_file(prop.module(), true);
             }
-
+        }
+        {
             // Check whether skip mounting
-        path.truncate(module_path_len);
+            let skip = module_paths.append("skip_mount");
-        path.append_path("skip_mount");
+            if skip.module().exists() {
-        if path.exists() {
                 continue;
             }
-
+        }
+        {
             // Double check whether the system folder exists
-        path.truncate(module_path_len);
+            let sys = module_paths.append("system");
-        path.append_path("system");
+            if sys.module().exists() {
-        if path.exists() {
                 info!("{}: loading module files", &info.name);
-            system.build_from_path(&mut path).log_ok();
+                system.collect(sys).log_ok();
+            }
         }
     }
 
@@ -611,16 +656,6 @@ pub fn load_modules(module_list: &[ModuleInfo], zygisk_name: &str) {
     }
     roots.insert("system", system);
 
-    // Reuse the path buffer
-    path.clear();
-    path.push_str("/");
-
-    // Build the base worker directory path
-    let mut tmp = cstr::buf::default()
-        .join_path(get_magisk_tmp())
-        .join_path(WORKERDIR);
-
-    let mut tracker = PathTracker::from(&mut path, &mut tmp);
     for (dir, mut root) in roots {
         // Step 4: Convert virtual filesystem tree into concrete operations
         //
@@ -629,7 +664,7 @@ pub fn load_modules(module_list: &[ModuleInfo], zygisk_name: &str) {
         // The "core" of the logic is to decide which directories need to be rebuilt in the
         // tmpfs worker directory, and real sub-nodes need to be mirrored inside it.
 
-        let path = tracker.append(dir);
+        let path = root_paths.append(dir);
         root.commit(path, true).log_ok();
     }
 }