From e7e1b8c7d340926dea237b144333824b7ce76047 Mon Sep 17 00:00:00 2001 From: Mark McCaskey Date: Wed, 17 Jul 2019 15:32:47 -0700 Subject: [PATCH] get tests passing! (more tests and clean up required) --- .../wasitests/wasi_sees_virtual_root.out | 24 ++--- .../wasitests/wasi_sees_virtual_root.rs | 2 +- lib/wasi/src/state.rs | 99 ++++++++++++++----- lib/wasi/src/syscalls/mod.rs | 37 +++++-- 4 files changed, 117 insertions(+), 45 deletions(-) diff --git a/lib/wasi-tests/wasitests/wasi_sees_virtual_root.out b/lib/wasi-tests/wasitests/wasi_sees_virtual_root.out index 6790f6328..1ce8a9c6a 100644 --- a/lib/wasi-tests/wasitests/wasi_sees_virtual_root.out +++ b/lib/wasi-tests/wasitests/wasi_sees_virtual_root.out @@ -1,12 +1,12 @@ -/act1 -act1-again -/act2 -/act1 -act1-again -/act2 -/act1 -act1-again -/act2 -/act1 -act1-again -/act2 +"/act1" +"/act1-again" +"/act2" +"/act1" +"/act1-again" +"/act2" +"/act1" +"/act1-again" +"/act2" +"/act1" +"/act1-again" +"/act2" diff --git a/lib/wasi-tests/wasitests/wasi_sees_virtual_root.rs b/lib/wasi-tests/wasitests/wasi_sees_virtual_root.rs index 9b20f5f04..f3ebb71fd 100644 --- a/lib/wasi-tests/wasitests/wasi_sees_virtual_root.rs +++ b/lib/wasi-tests/wasitests/wasi_sees_virtual_root.rs @@ -9,7 +9,7 @@ fn main() { // just cheat in this test because there is no comparison for native #[cfg(not(target_os = "wasi"))] let results = { - let start = vec!["/act1", "act1-again", "/act2"]; + let start = vec!["\"/act1\"", "\"/act1-again\"", "\"/act2\""]; let mut out = vec![]; for _ in 0..4 { diff --git a/lib/wasi/src/state.rs b/lib/wasi/src/state.rs index 89edee997..4396be115 100644 --- a/lib/wasi/src/state.rs +++ b/lib/wasi/src/state.rs @@ -117,10 +117,18 @@ pub enum Kind { Root { entries: HashMap, }, + /// The first two fields are data _about_ the symlink + /// the last field is the data _inside_ the symlink + /// + /// `base_po_dir` should never be the root because: + /// - Right now symlinks are not allowed in the immutable root + /// - There is always a closer pre-opened dir to the symlink file (by definition of the root being a collection of preopened dirs) Symlink { - /// The preopened dir that this symlink is relative to + /// The preopened dir that this symlink file is relative to (via `path_to_symlink`) base_po_dir: __wasi_fd_t, - /// the relative path from theroot of the preopened dir + /// The path to the symlink from the `base_po_dir` + path_to_symlink: PathBuf, + /// the value of the symlink as a relative path relative_path: PathBuf, }, Buffer { @@ -207,7 +215,7 @@ impl WasiFs { .expect("Could not open fd"); if let Kind::Root { entries } = &mut wasi_fs.inodes[root_inode].kind { // todo handle collisions - assert!(entries.insert(dbg!(dir.to_string()), inode).is_none()) + assert!(entries.insert(dir.to_string(), inode).is_none()) } wasi_fs.preopen_fds.push(fd); } @@ -245,7 +253,7 @@ impl WasiFs { .expect("Could not open fd"); if let Kind::Root { entries } = &mut wasi_fs.inodes[root_inode].kind { // todo handle collisions - assert!(dbg!(entries.insert(dbg!(alias.clone()), inode)).is_none()); + assert!(entries.insert(alias.clone(), inode).is_none()); } wasi_fs.preopen_fds.push(fd); } @@ -376,7 +384,8 @@ impl WasiFs { &mut self, base: __wasi_fd_t, path: &str, - symlink_count: u32, + mut symlink_count: u32, + follow_symlinks: bool, ) -> Result { if symlink_count > MAX_SYMLINKS { return Err(__WASI_EMLINK); @@ -390,7 +399,7 @@ impl WasiFs { 'path_iter: for component in path.components() { // for each component traverse file structure // loading inodes as necessary - 'symlink_resolution: loop { + 'symlink_resolution: while symlink_count < MAX_SYMLINKS { match &mut self.inodes[cur_inode].kind { Kind::Buffer { .. } => unimplemented!("state::get_inode_at_path for buffers"), Kind::Dir { @@ -407,6 +416,8 @@ impl WasiFs { return Err(__WASI_EACCES); } } + // used for full resolution of symlinks + let mut loop_for_symlink = false; if let Some(entry) = entries.get(component.as_os_str().to_string_lossy().as_ref()) { @@ -437,11 +448,25 @@ impl WasiFs { } else if file_type.is_symlink() { let link_value = file.read_link().ok().ok_or(__WASI_EIO)?; debug!("attempting to decompose path {:?}", link_value); - let (pre_open_dir_fd, relative_path) = - self.path_into_pre_open_and_relative_path(&link_value)?; + + let (pre_open_dir_fd, relative_path) = if link_value.is_relative() { + // the symlink resolution part of canonicalize is not what we want: + // this should help tests pass, then we can make it fail with a new test + // actually, it might be fine + /*let canon_link_value = dbg!(link_value.canonicalize()) + .ok() + .ok_or(__WASI_EINVAL)?;*/ + dbg!(self.path_into_pre_open_and_relative_path(&file))? + } else { + unimplemented!("ABSOLUTE SYMLINKS ARE NO GOOD"); + //dbg!(self.path_into_pre_open_and_relative_path(&link_value))? + }; + loop_for_symlink = true; + symlink_count += 1; Kind::Symlink { base_po_dir: pre_open_dir_fd, - relative_path: relative_path, + path_to_symlink: relative_path, + relative_path: link_value, } } else { unimplemented!("state::get_inode_at_path unknown file type: not file, directory, or symlink"); @@ -449,6 +474,9 @@ impl WasiFs { cur_inode = self.create_inode(kind, false, file.to_string_lossy().to_string())?; + if loop_for_symlink && follow_symlinks { + continue 'symlink_resolution; + } } } Kind::Root { entries } => { @@ -473,18 +501,30 @@ impl WasiFs { } Kind::Symlink { base_po_dir, + path_to_symlink, relative_path, } => { let new_base_dir = *base_po_dir; // allocate to reborrow mutabily to recur - let new_path = relative_path.to_string_lossy().to_string(); + let new_path = { + /*if let Kind::Root { .. } = self.inodes[base_po_dir].kind { + assert!(false, "symlinks should never be relative to the root"); + }*/ + let mut base = path_to_symlink.clone(); + // remove the symlink file itself from the path, leaving just the path from the base + // to the dir containing the symlink + base.pop(); + base.push(relative_path); + base.to_string_lossy().to_string() + }; let symlink_inode = self.get_inode_at_path_inner( new_base_dir, &new_path, symlink_count + 1, + follow_symlinks, )?; cur_inode = symlink_inode; - continue 'symlink_resolution; + //continue 'symlink_resolution; } } break 'symlink_resolution; @@ -520,12 +560,17 @@ impl WasiFs { /// gets a host file from a base directory and a path /// this function ensures the fs remains sandboxed + // NOTE: follow symlinks is super weird right now + // even if it's false, it still follows symlinks, just not the last + // symlink so + // This will be resolved when we have tests asserting the correct behavior pub fn get_inode_at_path( &mut self, base: __wasi_fd_t, path: &str, + follow_symlinks: bool, ) -> Result { - self.get_inode_at_path_inner(base, path, 0) + self.get_inode_at_path_inner(base, path, 0, follow_symlinks) } pub fn get_fd(&self, fd: __wasi_fd_t) -> Result<&Fd, __wasi_errno_t> { @@ -679,22 +724,28 @@ impl WasiFs { Kind::Dir { path, .. } => path.metadata().ok()?, Kind::Symlink { base_po_dir, - relative_path, + path_to_symlink, + .. } => { let base_po_inode = &self.fd_map[base_po_dir].inode; let base_po_inode_v = &self.inodes[*base_po_inode]; - dbg!(&base_po_inode_v.name); - if let Kind::Dir { path, .. } = &base_po_inode_v.kind { - let mut real_path = path.clone(); - // PHASE 1: ignore all possible symlinks in `relative_path` - // TODO: walk the segments of `relative_path` via the entries of the Dir - // use helper function to avoid duplicating this logic (walking this will require - // &self to be &mut sel - real_path.push(relative_path); - real_path.metadata().ok()? - } else { + match &base_po_inode_v.kind { + Kind::Root { .. } => { + path_to_symlink.clone().symlink_metadata().ok()? + } + Kind::Dir { path, .. } => { + let mut real_path = path.clone(); + // PHASE 1: ignore all possible symlinks in `relative_path` + // TODO: walk the segments of `relative_path` via the entries of the Dir + // use helper function to avoid duplicating this logic (walking this will require + // &self to be &mut sel + // TODO: adjust size of symlink, too + // for all paths adjusted think about this + real_path.push(path_to_symlink); + real_path.symlink_metadata().ok()? + } // if this triggers, there's a bug in the symlink code - unreachable!("Symlink pointing to something that's not a directory as its base preopened directory"); + _ => unreachable!("Symlink pointing to something that's not a directory as its base preopened directory"), } } __ => return None, diff --git a/lib/wasi/src/syscalls/mod.rs b/lib/wasi/src/syscalls/mod.rs index c524a19e9..1f5cc372c 100644 --- a/lib/wasi/src/syscalls/mod.rs +++ b/lib/wasi/src/syscalls/mod.rs @@ -1217,7 +1217,11 @@ pub fn path_filestat_get( .map_err(|_| __WASI_EINVAL)); debug!("=> path: {}", &path_string); - let file_inode = wasi_try!(state.fs.get_inode_at_path(fd, path_string)); + let file_inode = wasi_try!(state.fs.get_inode_at_path( + fd, + path_string, + flags & __WASI_LOOKUP_SYMLINK_FOLLOW != 0, + )); let stat = wasi_try!(state .fs .get_stat_for_kind(&state.fs.inodes[file_inode].kind) @@ -1328,6 +1332,10 @@ pub fn path_open( fd: WasmPtr<__wasi_fd_t>, ) -> __wasi_errno_t { debug!("wasi::path_open"); + if dirflags & __WASI_LOOKUP_SYMLINK_FOLLOW != 0 { + // TODO: resolution fn needs to get this bit + debug!(" - will follow symlinks when opening path"); + } let memory = ctx.memory(0); /* TODO: find actual upper bound on name size (also this is a path, not a name :think-fish:) */ if path_len > 1024 * 1024 { @@ -1352,7 +1360,11 @@ pub fn path_open( let path_string = wasi_try!(path.get_utf8_string(memory, path_len).ok_or(__WASI_EINVAL)); let path = std::path::PathBuf::from(path_string); - let inode = wasi_try!(state.fs.get_inode_at_path(dirfd, path_string)); + let inode = wasi_try!(state.fs.get_inode_at_path( + dirfd, + path_string, + dirflags & __WASI_LOOKUP_SYMLINK_FOLLOW != 0, + )); match &mut state.fs.inodes[inode].kind { Kind::File { @@ -1387,13 +1399,22 @@ pub fn path_open( } } } - Kind::Symlink { .. } => { - // TODO: figure out what to do here - unimplemented!("wasi::path_open on symlink"); + Kind::Symlink { + base_po_dir, + path_to_symlink, + relative_path, + } => { + unimplemented!("SYMLINKS IN PATH_OPEN"); + /*if dirflags & __WASI_LOOKUP_SYMLINK_FOLLOW != 0 { + //state.fs.get_inode_at_path(base_po_dir, ) + + } else { + // TODO: figure out what to do here + return __WASI_EINVAL; + }*/ } } - // TODO: reimplement all the flag checking logic debug!( "inode {:?} value {:#?} found!", inode, state.fs.inodes[inode] @@ -1429,12 +1450,12 @@ pub fn path_readlink( return __WASI_EACCES; } let path_str = wasi_try!(path.get_utf8_string(memory, path_len).ok_or(__WASI_EINVAL)); - let inode = wasi_try!(state.fs.get_inode_at_path(dir_fd, path_str)); + let inode = wasi_try!(state.fs.get_inode_at_path(dir_fd, path_str, false)); if let Kind::Symlink { relative_path, .. } = &state.fs.inodes[inode].kind { let rel_path_str = relative_path.to_string_lossy(); let bytes = rel_path_str.bytes(); - if bytes.len() < buf_len as usize { + if bytes.len() >= buf_len as usize { return __WASI_EOVERFLOW; }