Skip to content

Commit

Permalink
Squash changes:
Browse files Browse the repository at this point in the history
* Fix renaming a directory that was created with mkdir
* Fix inodes' name becoming stale after a rename
* Add test for mkdir -> rename -> mkdir
* Fix opening files in the presence of a non-default CWD
* Fix deadlock when base FD is not / or .
  • Loading branch information
Arshia001 committed Nov 18, 2024
1 parent 5ad8f0a commit de81fbc
Show file tree
Hide file tree
Showing 13 changed files with 133 additions and 32 deletions.
9 changes: 8 additions & 1 deletion lib/virtual-io/src/selector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,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();
Expand Down
2 changes: 1 addition & 1 deletion lib/wasix/src/fs/fd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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>,
}

Expand Down
47 changes: 36 additions & 11 deletions lib/wasix/src/fs/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -646,7 +646,7 @@ impl WasiFs {

/// Converts a relative path into an absolute path
pub(crate) fn relative_path_to_absolute(&self, mut path: String) -> String {
if path.starts_with("./") {
if path.starts_with("./") || path == "." {
let current_dir = self.current_dir.lock().unwrap();
path = format!("{}{}", current_dir.as_str(), &path[1..]);
if path.contains("//") {
Expand Down Expand Up @@ -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),
});

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -1337,13 +1344,31 @@ 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
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 {
self.get_fd_inode(base)?
};
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)
}

Expand Down Expand Up @@ -1498,7 +1523,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(),
}
Expand Down Expand Up @@ -1609,7 +1634,7 @@ impl WasiFs {
inodes.add_inode_val(InodeVal {
stat: RwLock::new(stat),
is_preopened,
name,
name: RwLock::new(name),
kind: RwLock::new(kind),
})
}
Expand Down Expand Up @@ -1995,7 +2020,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),
})
};
Expand Down
9 changes: 7 additions & 2 deletions lib/wasix/src/runners/wasi_common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,14 +66,19 @@ impl CommonWasiOptions {
});
let fs = prepare_filesystem(root_fs, &self.mounts, container_fs)?;

builder.add_preopen_dir("/")?;

// TODO: What's a preopen for '.' supposed to mean anyway? Why do we need it?
if self.mounts.iter().all(|m| m.guest != ".") {
// The user hasn't mounted "." to anything, so let's map it to "/"
let path = builder.get_current_dir().unwrap_or(PathBuf::from("/"));
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));

for pkg in &self.injected_packages {
Expand Down
11 changes: 6 additions & 5 deletions lib/wasix/src/syscalls/wasi/fd_prestat_dir_name.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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?

Expand All @@ -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);

Expand Down
12 changes: 10 additions & 2 deletions lib/wasix/src/syscalls/wasi/fd_readdir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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()
}
Expand Down
25 changes: 18 additions & 7 deletions lib/wasix/src/syscalls/wasi/path_create_directory.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
use std::{path::PathBuf, str::FromStr};
use std::{
path::{Component, PathBuf},
str::FromStr,
};

use super::*;
use crate::syscalls::*;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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() {
Expand All @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion lib/wasix/src/syscalls/wasi/path_filestat_get.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ pub fn path_filestat_get<M: MemorySize>(
let mut path_string = unsafe { get_input_str!(&memory, path, path_len) };

// Convert relative paths into absolute paths
if path_string.starts_with("./") {
if path_string.starts_with("./") || path_string == "." {
path_string = ctx.data().state.fs.relative_path_to_absolute(path_string);
}
tracing::trace!(path = path_string.as_str());
Expand Down
5 changes: 3 additions & 2 deletions lib/wasix/src/syscalls/wasi/path_rename.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down Expand Up @@ -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"
Expand All @@ -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)
Expand Down
9 changes: 9 additions & 0 deletions tests/integration/cli/tests/snapshot.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1362,3 +1362,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);
}
Original file line number Diff line number Diff line change
@@ -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
}
}
}
Binary file not shown.
12 changes: 12 additions & 0 deletions tests/rust-wasi-tests/src/bin/mkdir-rename.rs
Original file line number Diff line number Diff line change
@@ -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());
}

0 comments on commit de81fbc

Please sign in to comment.