Merge pull request #3230 from wasmerio/fix_wasi_path_rename

Remove test if dest file exist on path_rename wasi syscall (for #3228)

lib/vfs/src/mem_fs/filesystem.rs

@@ -158,7 +158,11 @@ impl crate::FileSystem for FileSystem {
     }
 
     fn rename(&self, from: &Path, to: &Path) -> Result<()> {
-        let ((position_of_from, inode, inode_of_from_parent), (inode_of_to_parent, name_of_to)) = {
+        let (
+            (position_of_from, inode, inode_of_from_parent),
+            (inode_of_to_parent, name_of_to),
+            inode_dest,
+        ) = {
             // Read lock.
             let fs = self.inner.try_read().map_err(|_| FsError::Lock)?;
 
@@ -180,6 +184,10 @@ impl crate::FileSystem for FileSystem {
             let inode_of_from_parent = fs.inode_of_parent(parent_of_from)?;
             let inode_of_to_parent = fs.inode_of_parent(parent_of_to)?;
 
+            // Find the inode of the dest file if it exists
+            let maybe_position_and_inode_of_file =
+                fs.as_parent_get_position_and_inode_of_file(inode_of_to_parent, &name_of_to)?;
+
             // Get the child indexes to update in the parent nodes, in
             // addition to the inode of the directory to update.
             let (position_of_from, inode) = fs
@@ -189,13 +197,22 @@ impl crate::FileSystem for FileSystem {
             (
                 (position_of_from, inode, inode_of_from_parent),
                 (inode_of_to_parent, name_of_to),
+                maybe_position_and_inode_of_file,
             )
         };
 
         {
             // Write lock.
             let mut fs = self.inner.try_write().map_err(|_| FsError::Lock)?;
 
+            if let Some((position, inode_of_file)) = inode_dest {
+                // Remove the file from the storage.
+                fs.storage.remove(inode_of_file);
+
+                // Remove the child from the parent directory.
+                fs.remove_child_from_node(inode_of_to_parent, position)?;
+            }
+
             // Update the file name, and update the modified time.
             fs.update_node_name(inode, name_of_to)?;
 

lib/wasi/src/syscalls/mod.rs

@@ -2659,18 +2659,13 @@ pub fn path_rename<M: MemorySize>(
         source_path.to_str().as_ref().unwrap(),
         true
     ));
-    if state
-        .fs
-        .get_inode_at_path(
-            inodes.deref_mut(),
-            new_fd,
-            target_path.to_str().as_ref().unwrap(),
-            true,
-        )
-        .is_ok()
-    {
-        return Errno::Exist;
-    }
+    // Create the destination inode if the file exists.
+    let _ = state.fs.get_inode_at_path(
+        inodes.deref_mut(),
+        new_fd,
+        target_path.to_str().as_ref().unwrap(),
+        true,
+    );
     let (source_parent_inode, source_entry_name) =
         wasi_try!(state
             .fs
@@ -2679,13 +2674,14 @@ pub fn path_rename<M: MemorySize>(
         wasi_try!(state
             .fs
             .get_parent_inode_at_path(inodes.deref_mut(), new_fd, target_path, true));
+    let mut need_create = true;
     let host_adjusted_target_path = {
         let guard = inodes.arena[target_parent_inode].read();
         let deref = guard.deref();
         match deref {
             Kind::Dir { entries, path, .. } => {
                 if entries.contains_key(&target_entry_name) {
-                    return Errno::Exist;
+                    need_create = false;
                 }
                 let mut out_path = path.clone();
                 out_path.push(std::path::Path::new(&target_entry_name));
@@ -2778,7 +2774,7 @@ pub fn path_rename<M: MemorySize>(
         }
     }
 
-    {
+    if need_create {
         let mut guard = inodes.arena[target_parent_inode].write();
         if let Kind::Dir { entries, .. } = guard.deref_mut() {
             let result = entries.insert(target_entry_name, source_entry);
@@ -2829,10 +2825,15 @@ pub fn path_symlink<M: MemorySize>(
         wasi_try!(state
             .fs
             .get_parent_inode_at_path(inodes.deref_mut(), fd, old_path_path, true));
-    let depth = wasi_try!(state
+    let depth = state
         .fs
-        .path_depth_from_fd(inodes.deref(), fd, source_inode))
-        - 1;
+        .path_depth_from_fd(inodes.deref(), fd, source_inode);
+
+    // depth == -1 means folder is not relative. See issue #3233.
+    let depth = match depth {
+        Ok(depth) => depth as i32 - 1,
+        Err(_) => -1,
+    };
 
     let new_path_path = std::path::Path::new(&new_path_str);
     let (target_parent_inode, entry_name) =

tests/wasi-wast/wasi/snapshot1/path_rename.wast

@@ -3,5 +3,5 @@
 (wasi_test "path_rename.wasm"
   (temp_dirs "temp")
   (assert_return (i64.const 0))
-  (assert_stdout "The original file does not still exist!\nFound item: path_renamed_file.txt\n柴犬\nrun_with_sub_dir: The original file does not still exist!\nrun_with_different_sub_dirs: The original file does not still exist!\n")
+  (assert_stdout "The original file does not still exist!\nFound item: path_renamed_file.txt\n柴犬\nThe original file does not still exist!\nFound item: path_renamed_file.txt\n柴犬\nrun_with_sub_dir: The original file does not still exist!\nrun_with_different_sub_dirs: The original file does not still exist!\n")
 )

tests/wasi-wast/wasi/tests/path_rename.rs

@@ -74,6 +74,86 @@ fn run_with_toplevel_dir() {
     std::fs::remove_file(file_to_rename_to).unwrap();
 }
 
+fn run_with_toplevel_dir_overwrite() {
+    #[cfg(not(target_os = "wasi"))]
+    let mut base = PathBuf::from("test_fs");
+    #[cfg(target_os = "wasi")]
+    let mut base = PathBuf::from("temp");
+
+    let file_to_create = base.join("path_rename_file.txt");
+    let file_to_rename_to = base.join("path_renamed_file.txt");
+
+    {
+        let mut f = std::fs::OpenOptions::new()
+            .create_new(true)
+            .write(true)
+            .open(&file_to_create)
+            .unwrap();
+
+        // text from https://ja.wikipedia.org/wiki/柴犬
+        let shiba_string = "「柴犬」という名前は中央高地で使われていたもので、文献上では、昭和初期の日本犬保存会の会誌「日本犬」で用いられている。一般的には、「柴」は小ぶりな雑木を指す。
+由来には諸説があり、
+
+    柴藪を巧みにくぐり抜けて猟を助けることから
+    赤褐色の毛色が枯れ柴に似ている（柴赤）ことから
+    小さなものを表す古語の「柴」から
+
+の3つの説が代表的。";
+        f.write_all(shiba_string.as_bytes()).unwrap();
+    }
+
+    {
+        let mut f = std::fs::OpenOptions::new()
+            .create_new(true)
+            .write(true)
+            .open(&file_to_rename_to)
+            .unwrap();
+
+        let lorem_string = "lorem ispum";
+        f.write_all(lorem_string.as_bytes()).unwrap();
+    }
+
+    std::fs::rename(&file_to_create, &file_to_rename_to).unwrap();
+    let mut file = fs::File::open(&file_to_rename_to).expect("Could not open file");
+    if file_to_create.exists() {
+        println!("The original file still exists!");
+        return;
+    } else {
+        println!("The original file does not still exist!");
+    }
+
+
+    if !file_to_rename_to.exists() {
+        println!("The moved file does not exist!");
+        return;
+    }
+
+    // TODO: add temp directory suport for native execution...
+    // until then, don't actually inspect the directory when running native code.
+    #[cfg(target_os = "wasi")]
+    for item in fs::read_dir(&base).unwrap() {
+        println!(
+            "Found item: {}",
+            item.unwrap().path().file_name().unwrap().to_str().unwrap()
+        );
+    }
+    #[cfg(not(target_os = "wasi"))]
+    {
+        println!("Found item: path_renamed_file.txt");
+    }
+
+    let mut out_str = String::new();
+    file.read_to_string(&mut out_str).unwrap();
+    let mut test_str = String::new();
+    let mut out_chars = out_str.chars();
+    out_chars.next().unwrap();
+    test_str.push(out_chars.next().unwrap());
+    test_str.push(out_chars.next().unwrap());
+
+    println!("{}", test_str);
+    std::fs::remove_file(file_to_rename_to).unwrap();
+}
+
 fn run_with_sub_dir() {
     #[cfg(not(target_os = "wasi"))]
     let base = PathBuf::from("test_fs");
@@ -162,6 +242,7 @@ fn run_with_different_sub_dirs() {
 
 fn main() {
     run_with_toplevel_dir();
+    run_with_toplevel_dir_overwrite();
     run_with_sub_dir();
     run_with_different_sub_dirs();
 }