Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix handling of the root dir in path_create_directory and get_inode_at_path_inner #5237

Merged
merged 3 commits into from
Nov 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion lib/virtual-io/src/selector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
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
30 changes: 19 additions & 11 deletions lib/wasix/src/fs/mod.rs
Original file line number Diff line number Diff line change
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,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)
}

Expand Down Expand Up @@ -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(),
}
Expand Down Expand Up @@ -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),
})
}
Expand Down Expand Up @@ -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),
})
};
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
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 @@ -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);
}
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());
}
Loading