Merge pull request #5307 from wasmerio/fix/more-fs-fixes

* Fix resolving relative paths in the presence of not-root CWD and non-root base FD
* Fix opening files after renaming their parent directory

lib/wasix/src/fs/mod.rs

@@ -1,3 +1,10 @@
+// TODO: currently, hard links are broken in the presence or renames.
+// It is impossible to fix them with the current setup, since a hard
+// link must point to the actual file rather than its path, but the
+// only way we can get to a file on a FileSystem instance is by going
+// through its repective FileOpener and giving it a path as input.
+// TODO: refactor away the InodeVal type
+
 mod fd;
 mod fd_list;
 mod inode_guard;
@@ -1262,32 +1269,7 @@ impl WasiFs {
         follow_symlinks: bool,
     ) -> Result<InodeGuard, Errno> {
         let base_inode = self.get_fd_inode(base)?;
-        let name = base_inode.name.read().unwrap();
-
-        let start_inode;
-        if name.starts_with('/') {
-            drop(name);
-            start_inode = base_inode;
-        } else {
-            let kind = base_inode.kind.read().unwrap();
-
-            // HACK: We preopen '/' with an alias of '.' by default in `prepare_webc_env`. If wasix-libc
-            // picks that up as the base FD, we want to do the same thing we do when the base is '/'
-            if *name == "."
-                && matches!(kind.deref(), Kind::Dir { path, .. } if path.as_os_str().as_encoded_bytes() == b"/")
-            {
-                drop(name);
-                drop(kind);
-                start_inode = base_inode;
-            } else {
-                drop(name);
-                drop(kind);
-                let (cur_inode, _) = self.get_current_dir(inodes, base)?;
-                start_inode = cur_inode;
-            }
-        };
-
-        self.get_inode_at_path_inner(inodes, start_inode, path, 0, follow_symlinks)
+        self.get_inode_at_path_inner(inodes, base_inode, path, 0, follow_symlinks)
     }
 
     /// Returns the parent Dir or Root that the file at a given path is in and the file name

lib/wasix/src/runners/wasi_common.rs

@@ -73,10 +73,6 @@ impl CommonWasiOptions {
             builder.add_map_dir(".", path)?;
         }
 
-        // wasix-libc favors later FDs, and we want it to pass in the preopen
-        // for '/' when opening things since that's faster, so do this after
-        // the '.' alias. Purely a performance thing, since we also account
-        // for the alias in `get_inode_at_path`.
         builder.add_preopen_dir("/")?;
 
         builder.set_fs(Box::new(fs));

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

@@ -1,3 +1,7 @@
+use std::path::PathBuf;
+
+use anyhow::Context;
+
 use super::*;
 use crate::syscalls::*;
 
@@ -98,9 +102,7 @@ pub fn path_rename_internal(
                 if entries.contains_key(&target_entry_name) {
                     need_create = false;
                 }
-                let mut out_path = path.clone();
-                out_path.push(std::path::Path::new(&target_entry_name));
-                out_path
+                path.join(&target_entry_name)
             }
             Kind::Root { .. } => return Ok(Errno::Notcapable),
             Kind::Socket { .. }
@@ -157,13 +159,11 @@ pub fn path_rename_internal(
                         return Ok(e);
                     }
                 } else {
-                    {
-                        let mut guard = source_entry.write();
-                        if let Kind::File { ref mut path, .. } = guard.deref_mut() {
-                            *path = host_adjusted_target_path;
-                        } else {
-                            unreachable!()
-                        }
+                    let mut guard = source_entry.write();
+                    if let Kind::File { ref mut path, .. } = guard.deref_mut() {
+                        *path = host_adjusted_target_path;
+                    } else {
+                        unreachable!()
                     }
                 }
             }
@@ -182,19 +182,17 @@ pub fn path_rename_internal(
                     return Ok(e);
                 }
                 {
+                    let source_dir_path = path.clone();
                     drop(guard);
-                    let mut guard = source_entry.write();
-                    if let Kind::Dir { path, .. } = guard.deref_mut() {
-                        *path = host_adjusted_target_path;
-                    }
+                    rename_inode_tree(&source_entry, &source_dir_path, &host_adjusted_target_path);
                 }
             }
-            Kind::Buffer { .. } => {}
-            Kind::Symlink { .. } => {}
-            Kind::Socket { .. } => {}
-            Kind::Pipe { .. } => {}
-            Kind::Epoll { .. } => {}
-            Kind::EventNotifications { .. } => {}
+            Kind::Buffer { .. }
+            | Kind::Symlink { .. }
+            | Kind::Socket { .. }
+            | Kind::Pipe { .. }
+            | Kind::Epoll { .. }
+            | Kind::EventNotifications { .. } => {}
             Kind::Root { .. } => unreachable!("The root can not be moved"),
         }
     }
@@ -213,12 +211,46 @@ pub fn path_rename_internal(
     }
 
     // The target entry is created, one way or the other
-    let target_inode =
-        wasi_try_ok!(state
-            .fs
-            .get_inode_at_path(inodes, target_fd, target_path, true));
+    let target_inode = state
+        .fs
+        .get_inode_at_path(inodes, target_fd, target_path, true)
+        .expect("Expected target inode to exist, and it's too late to safely fail");
     *target_inode.name.write().unwrap() = target_entry_name.into();
     target_inode.stat.write().unwrap().st_size = source_size;
 
     Ok(Errno::Success)
 }
+
+fn rename_inode_tree(inode: &InodeGuard, source_dir_path: &Path, target_dir_path: &Path) {
+    let children;
+
+    let mut guard = inode.write();
+    match guard.deref_mut() {
+        Kind::File { ref mut path, .. } => {
+            *path = adjust_path(path, source_dir_path, target_dir_path);
+            return;
+        }
+        Kind::Dir {
+            ref mut path,
+            entries,
+            ..
+        } => {
+            *path = adjust_path(path, source_dir_path, target_dir_path);
+            children = entries.values().cloned().collect::<Vec<_>>();
+        }
+        _ => return,
+    }
+    drop(guard);
+
+    for child in children {
+        rename_inode_tree(&child, source_dir_path, target_dir_path);
+    }
+}
+
+fn adjust_path(path: &Path, source_dir_path: &Path, target_dir_path: &Path) -> PathBuf {
+    let relative_path = path
+        .strip_prefix(source_dir_path)
+        .with_context(|| format!("Expected path {path:?} to be a subpath of {source_dir_path:?}"))
+        .expect("Fatal filesystem error");
+    target_dir_path.join(relative_path)
+}

tests/wasix/create-move-open/main.c

@@ -0,0 +1,71 @@
+// There used to be an issue where, when moving a file, you could no longer
+// open it afterwards. This is a regression test for that issue.
+
+#include <errno.h>
+#include <fcntl.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+#include <unistd.h>
+
+void error(const char *message)
+{
+    perror(message);
+    exit(-1);
+}
+
+int main()
+{
+    // Create two directories
+    if (mkdir("test1", S_IRWXU))
+    {
+        error("mkdir test1");
+    }
+    if (mkdir("test1/inner", S_IRWXU))
+    {
+        error("mkdir t2");
+    }
+
+    FILE *file;
+    if (!(file = fopen("test1/inner/file", "wt")))
+    {
+        error("create file");
+    }
+    if (!fwrite("hello\n", 1, 6, file))
+    {
+        error("write to file");
+    }
+    if (fflush(file))
+    {
+        error("fflush");
+    }
+    if (fclose(file))
+    {
+        error("fclose");
+    }
+
+    if (rename("test1", "test2"))
+    {
+        error("rename");
+    }
+
+    if (!(file = fopen("test2/inner/file", "rt")))
+    {
+        error("open renamed file");
+    }
+    char buf[7] = {0};
+    if (!fread((void *)buf, 1, 7, file))
+    {
+        error("fread");
+    }
+    if (strcmp(buf, "hello\n"))
+    {
+        printf("Invalid file contents: %s\n", buf);
+        return -1;
+    }
+
+    printf("0");
+    return 0;
+}

tests/wasix/create-move-open/run.sh

@@ -0,0 +1,5 @@
+#!/bin/bash
+
+$WASMER -q run main.wasm --dir . > output
+
+rm -rf test1 test2 2>/dev/null && printf "0" | diff -u output - 1>/dev/null

tests/wasix/fstatat-with-chdir/main.c

@@ -0,0 +1,57 @@
+#include <errno.h>
+#include <fcntl.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+#include <unistd.h>
+
+void error(const char *message)
+{
+    perror(message);
+    exit(-1);
+}
+
+int main()
+{
+    // Create two directories
+    if (mkdir("test1", S_IRWXU))
+    {
+        error("mkdir test1");
+    }
+    if (mkdir("test2", S_IRWXU))
+    {
+        error("mkdir test2");
+    }
+
+    // open the second directory for fstatat
+    int fd;
+    if ((fd = open("test2", O_RDONLY | O_DIRECTORY)) < 0)
+    {
+        error("open");
+    }
+
+    // chdir into the first directory
+    if (chdir("/home/test1"))
+    {
+        error("chdir");
+    }
+
+    // Now stat the second directory with a relative path.
+    // CWD should not be taken into account, and the stat
+    // should succeed.
+    struct stat st;
+    if (fstatat(fd, ".", &st, 0))
+    {
+        error("fstatat");
+    }
+
+    if (!S_ISDIR(st.st_mode))
+    {
+        printf("Expected a directory\n");
+        return -1;
+    }
+
+    printf("0");
+    return 0;
+}

tests/wasix/fstatat-with-chdir/run.sh

@@ -0,0 +1,5 @@
+#!/bin/bash
+
+$WASMER -q run main.wasm --dir . > output
+
+rm -rf test1 test2 2>/dev/null && printf "0" | diff -u output - 1>/dev/null