diff --git a/lib/wasix/src/syscalls/wasi/path_create_directory.rs b/lib/wasix/src/syscalls/wasi/path_create_directory.rs index a998a1f591a..424b1851715 100644 --- a/lib/wasix/src/syscalls/wasi/path_create_directory.rs +++ b/lib/wasix/src/syscalls/wasi/path_create_directory.rs @@ -60,122 +60,75 @@ pub(crate) fn path_create_directory_internal( return Err(Errno::Access); } - let path = std::path::PathBuf::from(path); - let path_vec = path - .components() - .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::, Errno>>()?; - if path_vec.is_empty() { - trace!("path vector is invalid (its empty)"); - return Err(Errno::Inval); - } + let (parent_inode, dir_name) = + state + .fs + .get_parent_inode_at_path(inodes, fd, Path::new(path), true)?; - let mut cur_dir_inode = working_dir.inode; - let mut created_directory = false; - for (comp_idx, comp) in path_vec.iter().enumerate() { - 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, - parent, - } => { - match comp.borrow() { - ".." => { - if let Some(p) = parent.upgrade() { - cur_dir_inode = p; - continue; - } - } - "." => continue, - _ => (), - } - if let Some(child) = entries.get(comp) { - cur_dir_inode = child.clone(); - } else { - let mut adjusted_path = path.clone(); - drop(guard); - - // TODO: double check this doesn't risk breaking the sandbox - adjusted_path.push(comp); - if let Ok(adjusted_path_stat) = path_filestat_get_internal( - &memory, - state, - inodes, - fd, - 0, - &adjusted_path.to_string_lossy(), - ) { - if adjusted_path_stat.st_filetype != Filetype::Directory { - trace!("path is not a directory"); - return Err(Errno::Notdir); - } - } else { - if comp_idx != path_vec.len() - 1 { - return Err(Errno::Noent); - } - - created_directory = true; - state.fs_create_dir(&adjusted_path)?; - } - let kind = Kind::Dir { - parent: cur_dir_inode.downgrade(), - path: adjusted_path, - entries: Default::default(), - }; - let new_inode = state - .fs - .create_inode(inodes, kind, false, comp.to_string())?; - - // reborrow to insert - { - let mut guard = cur_dir_inode.write(); - if let Kind::Dir { - ref mut entries, .. - } = guard.deref_mut() - { - entries.insert(comp.to_string(), new_inode.clone()); - } - } - cur_dir_inode = new_inode; - } + let mut guard = parent_inode.write(); + match guard.deref_mut() { + Kind::Dir { + ref entries, + ref path, + .. + } => { + if let Some(child) = entries.get(&dir_name) { + return Err(Errno::Exist); } - Kind::Root { entries } => { - match comp.borrow() { - "." | ".." => continue, - _ => (), - } - if let Some(child) = entries.get(comp) { - cur_dir_inode = child.clone(); - } else { - trace!("the root node can no create a directory"); - return Err(Errno::Access); - } + + let mut new_dir_path = path.clone(); + new_dir_path.push(&dir_name); + + drop(guard); + + // TODO: This condition should already have been checked by the entries.get check + // above, but it was in the code before my refactor and I'm keeping it just in case. + if path_filestat_get_internal( + &memory, + state, + inodes, + fd, + 0, + &new_dir_path.to_string_lossy(), + ) + .is_ok() + { + return Err(Errno::Exist); } - _ => { - trace!("path is not a directory"); - return Err(Errno::Notdir); + + state.fs_create_dir(&new_dir_path)?; + + let kind = Kind::Dir { + parent: parent_inode.downgrade(), + path: new_dir_path, + entries: Default::default(), + }; + let new_inode = state + .fs + .create_inode(inodes, kind, false, dir_name.clone())?; + + // reborrow to insert + { + let mut guard = parent_inode.write(); + let Kind::Dir { + ref mut entries, .. + } = guard.deref_mut() + else { + unreachable!(); + }; + + entries.insert(dir_name, new_inode.clone()); } } + Kind::Root { .. } => { + trace!("the root node can only contain pre-opened directories"); + return Err(Errno::Access); + } + _ => { + trace!("path is not a directory"); + return Err(Errno::Notdir); + } } - if created_directory { - Ok(()) - } else { - Err(Errno::Exist) - } + Ok(()) } diff --git a/lib/wasix/src/syscalls/wasi/path_remove_directory.rs b/lib/wasix/src/syscalls/wasi/path_remove_directory.rs index d254b48838c..818f4da0bda 100644 --- a/lib/wasix/src/syscalls/wasi/path_remove_directory.rs +++ b/lib/wasix/src/syscalls/wasi/path_remove_directory.rs @@ -1,3 +1,5 @@ +use std::fs; + use super::*; use crate::syscalls::*; @@ -42,123 +44,54 @@ pub(crate) fn path_remove_directory_internal( let (memory, state, inodes) = unsafe { env.get_memory_and_wasi_state_and_inodes(&ctx, 0) }; let working_dir = state.fs.get_fd(fd)?; - 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) - }) - .collect::, Errno>>()?; - if path_vec.is_empty() { - trace!("path vector is invalid (its empty)"); - return Err(Errno::Inval); - } - - let (child, parent) = path_vec.split_last().unwrap(); - - // if path only contains one component (the root), operation is not permitted - if child.is_empty() { - return Err(Errno::Access); - } - - let mut cur_dir_inode = working_dir.inode; - for comp in path_vec.iter() { - let processing_cur_dir_inode = cur_dir_inode.clone(); - let mut guard = processing_cur_dir_inode.write(); - match guard.deref_mut() { - Kind::Dir { - ref mut entries, - path, - parent, - } => { - match comp.borrow() { - ".." => { - if let Some(p) = parent.upgrade() { - cur_dir_inode = p; - continue; - } - } - "." => continue, - _ => (), + let (parent_inode, dir_name) = + state + .fs + .get_parent_inode_at_path(inodes, fd, Path::new(path), true)?; + + let mut guard = parent_inode.write(); + match guard.deref_mut() { + Kind::Dir { + entries: ref mut parent_entries, + .. + } => { + let Some(child_inode) = parent_entries.get(&dir_name) else { + return Err(Errno::Noent); + }; + + { + let Kind::Dir { + entries: ref child_entries, + path: ref child_path, + .. + } = *child_inode.read() + else { + return Err(Errno::Notdir); + }; + + if !child_entries.is_empty() { + return Err(Errno::Notempty); } - if let Some(child) = entries.get(comp) { - cur_dir_inode = child.clone(); - } else { - let parent_path = path.clone(); - let mut adjusted_path = path.clone(); - drop(guard); - - // TODO: double check this doesn't risk breaking the sandbox - adjusted_path.push(comp); - if let Ok(adjusted_path_stat) = path_filestat_get_internal( - &memory, - state, - inodes, - fd, - 0, - &adjusted_path.to_string_lossy(), - ) { - if adjusted_path_stat.st_filetype != Filetype::Directory { - trace!("path is not a directory"); - return Err(Errno::Notdir); - } - } else { - return Err(Errno::Noent); - } - let kind = Kind::Dir { - parent: cur_dir_inode.downgrade(), - path: adjusted_path, - entries: Default::default(), - }; - let new_inode = state - .fs - .create_inode(inodes, kind, false, comp.to_string())?; - // reborrow to insert - { - let mut guard = cur_dir_inode.write(); - if let Kind::Dir { - ref mut entries, .. - } = guard.deref_mut() - { - entries.insert(comp.to_string(), new_inode.clone()); - } - } - cur_dir_inode = new_inode; + if let Err(e) = state.fs_remove_dir(child_path) { + tracing::warn!(path = ?child_path, error = ?e, "failed to remove directory"); + return Err(e); } } - Kind::Root { .. } => { - trace!("the root node can no create a directory"); - return Err(Errno::Access); - } - _ => { - trace!("path is not a directory"); - return Err(Errno::Notdir); - } - } - } - if let Kind::Dir { - parent, - path: child_path, - entries, - } = cur_dir_inode.write().deref_mut() - { - let parent = parent.upgrade().ok_or(Errno::Noent)?; + parent_entries.remove(&dir_name).expect( + "Entry should exist since we checked before and have an exclusive write lock", + ); - if let Kind::Dir { entries, .. } = parent.write().deref_mut() { - let child_inode = entries.remove(child).ok_or(Errno::Noent)?; - - if let Err(e) = state.fs_remove_dir(&child_path) { - tracing::warn!(path = ?child_path, error = ?e, "failed to remove directory"); - } + Ok(()) + } + Kind::Root { .. } => { + trace!("directories directly in the root node can not be removed"); + Err(Errno::Access) + } + _ => { + trace!("path is not a directory"); + Err(Errno::Notdir) } - - drop(parent) } - - Ok(()) } diff --git a/tests/wasix/create-and-remove-dirs/main.c b/tests/wasix/create-and-remove-dirs/main.c new file mode 100644 index 00000000000..a7371e31a1f --- /dev/null +++ b/tests/wasix/create-and-remove-dirs/main.c @@ -0,0 +1,99 @@ +#include +#include +#include +#include +#include +#include +#include + +int ensure_dir_accessible(const char *dir_name) +{ + struct stat st; + char buf[256]; + + if (stat(dir_name, &st) != 0 || !S_ISDIR(st.st_mode)) + { + return -1; + } + + sprintf(buf, "./%s", dir_name); + if (stat(buf, &st) != 0 || !S_ISDIR(st.st_mode)) + { + return -1; + } + + sprintf(buf, "/home/%s", dir_name); + if (stat(buf, &st) != 0 || !S_ISDIR(st.st_mode)) + { + return -1; + } + + return 0; +} + +int ensure_dir_removed(const char *dir_name) +{ + struct stat st; + + if (stat(dir_name, &st) == 0 || errno != ENOENT) + { + return -1; + } + + errno = 0; + return 0; +} + +void error(const char *message) +{ + perror(message); + exit(-1); +} + +int main() +{ + if (mkdir("test1/test2", S_IRWXU | S_IRWXG | S_IRWXO) == 0) + { + printf("Expected recursive directory creation to fail\n"); + return -1; + } + + if (mkdir("test1", S_IRWXU | S_IRWXG | S_IRWXO) != 0 || ensure_dir_accessible("test1") != 0) + { + error("mkdir test1"); + } + + if (mkdir("test1/test2", S_IRWXU | S_IRWXG | S_IRWXO) != 0 || ensure_dir_accessible("test1/test2") != 0) + { + error("mkdir test2"); + } + + if (rmdir("test1") == 0) + { + printf("Expected removing non-empty directory to fail\n"); + return -1; + } + + if (rmdir("test1/test2") != 0 || ensure_dir_removed("test1/test2") != 0) + { + error("rmdir test2"); + } + + if (rmdir("test1") != 0 || ensure_dir_removed("test1") != 0) + { + error("rmdir test1"); + } + + if (mkdir("test1", S_IRWXU | S_IRWXG | S_IRWXO) != 0 || ensure_dir_accessible("test1") != 0) + { + error("re-create test1"); + } + + if (rmdir("test1") != 0 || ensure_dir_removed("test1") != 0) + { + error("re-remove test1"); + } + + printf("0"); + return 0; +} diff --git a/tests/wasix/create-and-remove-dirs/run.sh b/tests/wasix/create-and-remove-dirs/run.sh new file mode 100755 index 00000000000..12e547d274a --- /dev/null +++ b/tests/wasix/create-and-remove-dirs/run.sh @@ -0,0 +1,5 @@ +#!/bin/bash + +$WASMER -q run main.wasm --dir . > output + +rm -rf test1 2>/dev/null && printf "0" | diff -u output - 1>/dev/null