Merge pull request #5237 from wasmerio/fix/mkdir-rename-mkdir

Fix handling of the root dir in `path_create_directory` and `get_inode_at_path_inner`

lib/virtual-io/src/selector.rs

@@ -138,7 +138,14 @@ impl Selector {
         let mut events = mio::Events::with_capacity(128);
         loop {
             // Wait for an event to trigger
-            poll.poll(&mut events, None).unwrap();
+            if let Err(e) = poll.poll(&mut events, None) {
+                // This can happen when a debugger is attached
+                #[cfg(debug_assertions)]
+                if e.kind() == std::io::ErrorKind::Interrupted {
+                    continue;
+                }
+                panic!("Unexpected error in selector poll loop: {e:?}");
+            }
 
             // Loop through all the events while under a guard lock
             let mut guard = engine.inner.lock().unwrap();

lib/wasix/src/fs/fd.rs

@@ -62,7 +62,7 @@ impl Fd {
 pub struct InodeVal {
     pub stat: RwLock<Filestat>,
     pub is_preopened: bool,
-    pub name: Cow<'static, str>,
+    pub name: RwLock<Cow<'static, str>>,
     pub kind: RwLock<Kind>,
 }
 

lib/wasix/src/fs/mod.rs

@@ -676,7 +676,7 @@ impl WasiFs {
         let root_inode = inodes.add_inode_val(InodeVal {
             stat: RwLock::new(stat),
             is_preopened: true,
-            name: "/".into(),
+            name: RwLock::new("/".into()),
             kind: RwLock::new(root_kind),
         });
 
@@ -984,6 +984,13 @@ impl WasiFs {
 
         // TODO: rights checks
         'path_iter: for (i, component) in path.components().enumerate() {
+            // Since we're resolving the path against the given inode, we want to
+            // assume '/a/b' to be the same as `a/b` relative to the inode, so
+            // we skip over the RootDir component.
+            if matches!(component, Component::RootDir) {
+                continue;
+            }
+
             // used to terminate symlink resolution properly
             let last_component = i + 1 == n_components;
             // for each component traverse file structure
@@ -1337,13 +1344,14 @@ impl WasiFs {
         follow_symlinks: bool,
     ) -> Result<InodeGuard, Errno> {
         let base_inode = self.get_fd_inode(base)?;
-        let start_inode =
-            if !base_inode.deref().name.starts_with('/') && self.is_wasix.load(Ordering::Acquire) {
-                let (cur_inode, _) = self.get_current_dir(inodes, base)?;
-                cur_inode
-            } else {
-                self.get_fd_inode(base)?
-            };
+        let start_inode = if !base_inode.deref().name.read().unwrap().starts_with('/')
+            && self.is_wasix.load(Ordering::Acquire)
+        {
+            let (cur_inode, _) = self.get_current_dir(inodes, base)?;
+            cur_inode
+        } else {
+            self.get_fd_inode(base)?
+        };
         self.get_inode_at_path_inner(inodes, start_inode, path, 0, follow_symlinks)
     }
 
@@ -1498,7 +1506,7 @@ impl WasiFs {
                 // REVIEW:
                 // no need for +1, because there is no 0 end-of-string marker
                 // john: removing the +1 seems cause regression issues
-                pr_name_len: inode_val.name.len() as u32 + 1,
+                pr_name_len: inode_val.name.read().unwrap().len() as u32 + 1,
             }
             .untagged(),
         }
@@ -1609,7 +1617,7 @@ impl WasiFs {
         inodes.add_inode_val(InodeVal {
             stat: RwLock::new(stat),
             is_preopened,
-            name,
+            name: RwLock::new(name),
             kind: RwLock::new(kind),
         })
     }
@@ -1995,7 +2003,7 @@ impl WasiFs {
             inodes.add_inode_val(InodeVal {
                 stat: RwLock::new(stat),
                 is_preopened: true,
-                name: name.to_string().into(),
+                name: RwLock::new(name.to_string().into()),
                 kind: RwLock::new(kind),
             })
         };

lib/wasix/src/syscalls/wasi/fd_prestat_dir_name.rs

@@ -13,7 +13,8 @@ pub fn fd_prestat_dir_name<M: MemorySize>(
     let path_chars = wasi_try_mem!(path.slice(&memory, path_len));
 
     let inode = wasi_try!(state.fs.get_fd_inode(fd));
-    Span::current().record("path", inode.name.as_ref());
+    let name = inode.name.read().unwrap();
+    Span::current().record("path", name.as_ref());
 
     // check inode-val.is_preopened?
 
@@ -22,11 +23,11 @@ pub fn fd_prestat_dir_name<M: MemorySize>(
         Kind::Dir { .. } | Kind::Root { .. } => {
             // TODO: verify this: null termination, etc
             let path_len: u64 = path_len.into();
-            if (inode.name.len() as u64) < path_len {
+            if (name.len() as u64) < path_len {
                 wasi_try_mem!(path_chars
-                    .subslice(0..inode.name.len() as u64)
-                    .write_slice(inode.name.as_bytes()));
-                wasi_try_mem!(path_chars.index(inode.name.len() as u64).write(0));
+                    .subslice(0..name.len() as u64)
+                    .write_slice(name.as_bytes()));
+                wasi_try_mem!(path_chars.index(name.len() as u64).write(0));
 
                 //trace!("=> result: \"{}\"", inode_val.name);
 

lib/wasix/src/syscalls/wasi/fd_readdir.rs

@@ -64,7 +64,11 @@ pub fn fd_readdir<M: MemorySize>(
                 entry_vec.extend(entries.iter().filter(|(_, inode)| inode.is_preopened).map(
                     |(name, inode)| {
                         let stat = inode.stat.read().unwrap();
-                        (inode.name.to_string(), stat.st_filetype, stat.st_ino)
+                        (
+                            inode.name.read().unwrap().to_string(),
+                            stat.st_filetype,
+                            stat.st_ino,
+                        )
                     },
                 ));
                 // adding . and .. special folders
@@ -88,7 +92,11 @@ pub fn fd_readdir<M: MemorySize>(
                     .into_iter()
                     .map(|(name, inode)| {
                         let stat = inode.stat.read().unwrap();
-                        (format!("/{}", inode.name), stat.st_filetype, stat.st_ino)
+                        (
+                            format!("/{}", inode.name.read().unwrap().as_ref()),
+                            stat.st_filetype,
+                            stat.st_ino,
+                        )
                     })
                     .collect()
             }

lib/wasix/src/syscalls/wasi/path_create_directory.rs

@@ -1,4 +1,7 @@
-use std::{path::PathBuf, str::FromStr};
+use std::{
+    path::{Component, PathBuf},
+    str::FromStr,
+};
 
 use super::*;
 use crate::syscalls::*;
@@ -30,7 +33,7 @@ pub fn path_create_directory<M: MemorySize>(
     Span::current().record("path", path_string.as_str());
 
     // Convert relative paths into absolute paths
-    if path_string.starts_with("./") {
+    if !path_string.starts_with("/") {
         path_string = ctx.data().state.fs.relative_path_to_absolute(path_string);
         trace!(
             %path_string
@@ -68,11 +71,14 @@ pub(crate) fn path_create_directory_internal(
     let path = std::path::PathBuf::from(path);
     let path_vec = path
         .components()
-        .map(|comp| {
-            comp.as_os_str()
-                .to_str()
-                .map(|inner_str| inner_str.to_string())
-                .ok_or(Errno::Inval)
+        .filter_map(|comp| match comp {
+            Component::RootDir => None,
+            _ => Some(
+                comp.as_os_str()
+                    .to_str()
+                    .map(|inner_str| inner_str.to_string())
+                    .ok_or(Errno::Inval),
+            ),
         })
         .collect::<Result<Vec<String>, Errno>>()?;
     if path_vec.is_empty() {
@@ -86,6 +92,11 @@ pub(crate) fn path_create_directory_internal(
         let processing_cur_dir_inode = cur_dir_inode.clone();
         let mut guard = processing_cur_dir_inode.write();
         match guard.deref_mut() {
+            // TODO: this depends on entries already being filled in. This is the case when you go
+            // through wasix-libc (which is, realistically, what everybody does) because it stats
+            // the path before calling path_create_directory, at which point entries is filled in.
+            // However, an alternate implementation or a manually implemented module may not always
+            // do this.
             Kind::Dir {
                 ref mut entries,
                 path,

lib/wasix/src/syscalls/wasi/path_rename.rs

@@ -72,7 +72,7 @@ pub fn path_rename_internal(
         }
     }
 
-    // this is to be sure the source file is fetch from filesystem if needed
+    // this is to be sure the source file is fetched from the filesystem if needed
     wasi_try_ok!(state
         .fs
         .get_inode_at_path(inodes, source_fd, source_path, true));
@@ -206,7 +206,7 @@ pub fn path_rename_internal(
     if need_create {
         let mut guard = target_parent_inode.write();
         if let Kind::Dir { entries, .. } = guard.deref_mut() {
-            let result = entries.insert(target_entry_name, source_entry);
+            let result = entries.insert(target_entry_name.clone(), source_entry);
             assert!(
                 result.is_none(),
                 "fatal error: race condition on filesystem detected or internal logic error"
@@ -219,6 +219,7 @@ pub fn path_rename_internal(
         wasi_try_ok!(state
             .fs
             .get_inode_at_path(inodes, target_fd, target_path, true));
+    *target_inode.name.write().unwrap() = target_entry_name.into();
     target_inode.stat.write().unwrap().st_size = source_size;
 
     Ok(Errno::Success)

tests/integration/cli/tests/snapshot.rs

@@ -1359,3 +1359,12 @@ fn test_snapshot_worker_panicking() {
     ));
     assert_json_snapshot!(snapshot);
 }
+
+#[cfg_attr(any(target_env = "musl", target_os = "windows"), ignore)]
+#[test]
+fn test_snapshot_mkdir_rename() {
+    let snapshot = TestBuilder::new()
+        .with_name(function!())
+        .run_wasm(include_bytes!("./wasm/mkdir-rename.wasm"));
+    assert_json_snapshot!(snapshot);
+}

tests/integration/cli/tests/snapshots/snapshot__snapshot_mkdir_rename.snap

@@ -0,0 +1,22 @@
+---
+source: tests/integration/cli/tests/snapshot.rs
+assertion_line: 1369
+expression: snapshot
+---
+{
+  "spec": {
+    "name": "snapshot::test_snapshot_mkdir_rename",
+    "use_packages": [],
+    "include_webcs": [],
+    "cli_args": [],
+    "enable_threads": true,
+    "enable_network": false
+  },
+  "result": {
+    "Success": {
+      "stdout": "",
+      "stderr": "",
+      "exit_code": 0
+    }
+  }
+}

tests/rust-wasi-tests/src/bin/mkdir-rename.rs

@@ -0,0 +1,12 @@
+fn main() {
+    std::fs::create_dir("/a").unwrap();
+    assert!(std::fs::metadata("/a").unwrap().is_dir());
+
+    std::fs::rename("/a", "/b").unwrap();
+    assert!(matches!(std::fs::metadata("/a"), Err(e) if e.kind() == std::io::ErrorKind::NotFound));
+    assert!(std::fs::metadata("/b").unwrap().is_dir());
+
+    std::fs::create_dir("/a").unwrap();
+    assert!(std::fs::metadata("/a").unwrap().is_dir());
+    assert!(std::fs::metadata("/b").unwrap().is_dir());
+}