From 36e9ecd784ae34492b58511aaeff1c904c4a1731 Mon Sep 17 00:00:00 2001 From: Jakub Konka Date: Tue, 11 Jun 2019 21:25:35 +0200 Subject: [PATCH 1/4] Add fixes so that misc-tests pass --- src/hostcalls/fs.rs | 28 +++++++++++++++++----------- src/sys/unix/hostcalls_impl/fs.rs | 31 +++++++++++++++++++++++++++++-- 2 files changed, 46 insertions(+), 13 deletions(-) diff --git a/src/hostcalls/fs.rs b/src/hostcalls/fs.rs index fed19d1..684bd13 100644 --- a/src/hostcalls/fs.rs +++ b/src/hostcalls/fs.rs @@ -492,7 +492,7 @@ pub fn path_open( Err(e) => return enc_errno(e), }; - let fe = match hostcalls_impl::path_open( + match hostcalls_impl::path_open( wasi_ctx, dirfd, dirflags, @@ -504,18 +504,24 @@ pub fn path_open( needed_inheriting, fs_flags, ) { - Ok(fe) => fe, - Err(e) => return enc_errno(e), - }; + Ok(fe) => { + let guest_fd = match wasi_ctx.insert_fd_entry(fe) { + Ok(fd) => fd, + Err(e) => return enc_errno(e), + }; - let guest_fd = match wasi_ctx.insert_fd_entry(fe) { - Ok(fd) => fd, - Err(e) => return enc_errno(e), - }; + enc_fd_byref(memory, fd_out_ptr, guest_fd) + .map(|_| wasm32::__WASI_ESUCCESS) + .unwrap_or_else(enc_errno) + } + Err(e) => { + if let Err(e) = enc_fd_byref(memory, fd_out_ptr, wasm32::__wasi_fd_t::max_value()) { + return enc_errno(e); + } - enc_fd_byref(memory, fd_out_ptr, guest_fd) - .map(|_| wasm32::__WASI_ESUCCESS) - .unwrap_or_else(enc_errno) + enc_errno(e) + } + } } #[wasi_common_cbindgen] diff --git a/src/sys/unix/hostcalls_impl/fs.rs b/src/sys/unix/hostcalls_impl/fs.rs index 25308e3..0e48034 100644 --- a/src/sys/unix/hostcalls_impl/fs.rs +++ b/src/sys/unix/hostcalls_impl/fs.rs @@ -353,7 +353,7 @@ pub(crate) fn path_open( needed_base |= host::__WASI_RIGHT_PATH_CREATE_FILE; } if nix_all_oflags.contains(OFlag::O_TRUNC) { - needed_inheriting |= host::__WASI_RIGHT_PATH_FILESTAT_SET_SIZE; + needed_base |= host::__WASI_RIGHT_PATH_FILESTAT_SET_SIZE; } // convert file descriptor flags @@ -778,7 +778,34 @@ pub(crate) fn path_unlink_file( // nix doesn't expose unlinkat() yet match unsafe { unlinkat(dir, path_cstr.as_ptr(), 0) } { 0 => Ok(()), - _ => Err(host_impl::errno_from_nix(errno::Errno::last())), + _ => { + let mut e = errno::Errno::last(); + + #[cfg(not(linux))] + { + // Non-Linux implementations may return EPERM when attempting to remove a + // directory without REMOVEDIR. While that's what POSIX specifies, it's + // less useful. Adjust this to EISDIR. It doesn't matter that this is not + // atomic with the unlinkat, because if the file is removed and a directory + // is created before fstatat sees it, we're racing with that change anyway + // and unlinkat could have legitimately seen the directory if the race had + // turned out differently. + use nix::fcntl::AtFlags; + use nix::sys::stat::{fstatat, SFlag}; + + if e == errno::Errno::EPERM { + if let Ok(stat) = fstatat(dir, path.as_os_str(), AtFlags::AT_SYMLINK_NOFOLLOW) { + if SFlag::from_bits_truncate(stat.st_mode).contains(SFlag::S_IFDIR) { + e = errno::Errno::EISDIR; + } + } else { + e = errno::Errno::last(); + } + } + } + + Err(host_impl::errno_from_nix(e)) + } } } From 2003de6866116fdae913fdb46ecfc7b8212366fb Mon Sep 17 00:00:00 2001 From: Jakub Konka Date: Thu, 13 Jun 2019 16:53:20 +0200 Subject: [PATCH 2/4] Refactor path_get and fix flags in path_open --- src/sys/unix/hostcalls_impl/fs.rs | 6 +- src/sys/unix/hostcalls_impl/fs_helpers.rs | 304 ++++++++++------------ 2 files changed, 142 insertions(+), 168 deletions(-) diff --git a/src/sys/unix/hostcalls_impl/fs.rs b/src/sys/unix/hostcalls_impl/fs.rs index 0e48034..27bbf74 100644 --- a/src/sys/unix/hostcalls_impl/fs.rs +++ b/src/sys/unix/hostcalls_impl/fs.rs @@ -337,10 +337,10 @@ pub(crate) fn path_open( let mut nix_all_oflags = if read && write { OFlag::O_RDWR - } else if read { - OFlag::O_RDONLY - } else { + } else if write { OFlag::O_WRONLY + } else { + OFlag::O_RDONLY }; // on non-Capsicum systems, we always want nofollow diff --git a/src/sys/unix/hostcalls_impl/fs_helpers.rs b/src/sys/unix/hostcalls_impl/fs_helpers.rs index 94a3b20..4d5e0fd 100644 --- a/src/sys/unix/hostcalls_impl/fs_helpers.rs +++ b/src/sys/unix/hostcalls_impl/fs_helpers.rs @@ -7,7 +7,8 @@ use crate::host; use nix::libc::{self, c_long}; use std::ffi::{OsStr, OsString}; -use std::os::unix::prelude::{OsStrExt, OsStringExt, RawFd}; +use std::os::unix::prelude::{OsStrExt, RawFd}; +use std::path::{Component, Path}; /// Normalizes a path to ensure that the target path is located under the directory provided. /// @@ -57,6 +58,11 @@ pub fn path_get>( Err(errno) } + if path.as_ref().as_bytes().contains(&b'\0') { + // if contains NUL, return EILSEQ + return Err(host::__WASI_EILSEQ); + } + let dirfe = wasi_ctx.get_fd_entry(dirfd, needed_base, needed_inheriting)?; // Stack of directory file descriptors. Index 0 always corresponds with the directory provided @@ -67,7 +73,7 @@ pub fn path_get>( // Stack of paths left to process. This is initially the `path` argument to this function, but // any symlinks we encounter are processed by pushing them on the stack. - let mut path_stack = vec![path.as_ref().to_owned().into_vec()]; + let mut path_stack = vec![path.as_ref().to_owned()]; // Track the number of symlinks we've expanded, so we can return `ELOOP` after too many. let mut symlink_expansions = 0; @@ -78,190 +84,158 @@ pub fn path_get>( // TODO: rewrite this using a custom posix path type, with a component iterator that respects // trailing slashes. This version does way too much allocation, and is way too fiddly. loop { - let component = if let Some(cur_path) = path_stack.pop() { - // eprintln!( - // "cur_path = {:?}", - // std::str::from_utf8(cur_path.as_slice()).unwrap() - // ); - let mut split = cur_path.splitn(2, |&c| c == '/' as u8); - let head = split.next(); - let tail = split.next(); - match (head, tail) { - (None, _) => { - // split always returns at least a singleton iterator with an empty slice - panic!("unreachable"); - } - // path is empty - (Some([]), None) => { - return ret_error(&mut dir_stack, host::__WASI_ENOENT); - } - // path starts with `/`, is absolute - (Some([]), Some(_)) => { - return ret_error(&mut dir_stack, host::__WASI_ENOTCAPABLE); - } - // the final component of the path with no trailing slash - (Some(component), None) => component.to_vec(), - (Some(component), Some(rest)) => { - if rest.iter().all(|&c| c == '/' as u8) { - // the final component of the path with trailing slashes; put one trailing - // slash back on - let mut component = component.to_vec(); - component.push('/' as u8); - component - } else { - // non-final component; push the rest back on the stack - path_stack.push(rest.to_vec()); - component.to_vec() - } - } - } - } else { - // if the path stack is ever empty, we return rather than going through the loop again - panic!("unreachable"); - }; + match path_stack.pop() { + Some(cur_path) => { + // eprintln!("cur_path = {:?}", cur_path); - // eprintln!( - // "component = {:?}", - // std::str::from_utf8(component.as_slice()).unwrap() - // ); - - match component.as_slice() { - b"." => { - // skip component - } - b".." => { - // pop a directory - let dirfd = dir_stack.pop().expect("dir_stack is never empty"); + let ends_with_slash = cur_path.as_bytes().ends_with(b"/"); + let mut components = Path::new(&cur_path).components(); + let head = match components.next() { + None => return ret_error(&mut dir_stack, host::__WASI_ENOENT), + Some(p) => p, + }; + let tail = components.as_path(); - // we're not allowed to pop past the original directory - if dir_stack.is_empty() { - return ret_error(&mut dir_stack, host::__WASI_ENOTCAPABLE); - } else { - nix::unistd::close(dirfd).unwrap_or_else(|e| { - dbg!(e); - }); + if tail.components().next().is_some() { + let mut tail = tail.as_os_str().to_owned(); + if ends_with_slash { + tail.push("/"); + } + path_stack.push(tail); } - } - // should the component be a directory? it should if there is more path left to process, or - // if it has a trailing slash and `needs_final_component` is not set - component - if !path_stack.is_empty() - || (component.ends_with(b"/") && !needs_final_component) => - { - match openat( - *dir_stack.last().expect("dir_stack is never empty"), - component, - OFlag::O_RDONLY | OFlag::O_DIRECTORY | OFlag::O_NOFOLLOW, - Mode::empty(), - ) { - Ok(new_dir) => { - dir_stack.push(new_dir); + + match head { + Component::Prefix(_) | Component::RootDir => { + // path is absolute! + return ret_error(&mut dir_stack, host::__WASI_ENOTCAPABLE); + } + Component::CurDir => { + // "." so skip continue; } - Err(e) - // Check to see if it was a symlink. Linux indicates - // this with ENOTDIR because of the O_DIRECTORY flag. - if e.as_errno() == Some(Errno::ELOOP) - || e.as_errno() == Some(Errno::EMLINK) - || e.as_errno() == Some(Errno::ENOTDIR) => - { - // attempt symlink expansion - match readlinkat( - *dir_stack.last().expect("dir_stack is never empty"), - component, - readlink_buf.as_mut_slice(), - ) { - Ok(link_path) => { - symlink_expansions += 1; - if symlink_expansions > MAX_SYMLINK_EXPANSIONS { - return ret_error(&mut dir_stack, host::__WASI_ELOOP); - } + Component::ParentDir => { + // ".." so pop a dir + let dirfd = dir_stack.pop().expect("dir_stack is never empty"); - let mut link_path = link_path.as_bytes().to_vec(); - - // append a trailing slash if the component leading to it has one, so - // that we preserve any ENOTDIR that might come from trying to open a - // non-directory - if component.ends_with(b"/") { - link_path.push('/' as u8); - } - - path_stack.push(link_path); - continue; - } - Err(e) => { - return ret_error( - &mut dir_stack, - host_impl::errno_from_nix(e.as_errno().unwrap()), - ); - } + // we're not allowed to pop past the original directory + if dir_stack.is_empty() { + return ret_error(&mut dir_stack, host::__WASI_ENOTCAPABLE); + } else { + nix::unistd::close(dirfd).unwrap_or_else(|e| { + dbg!(e); + }); } } - Err(e) => { - return ret_error( - &mut dir_stack, - host_impl::errno_from_nix(e.as_errno().unwrap()), - ); - } - } - } - // the final component - component => { - // if there's a trailing slash, or if `LOOKUP_SYMLINK_FOLLOW` is set, attempt - // symlink expansion - if component.ends_with(b"/") || (dirflags & host::__WASI_LOOKUP_SYMLINK_FOLLOW) != 0 - { - match readlinkat( - *dir_stack.last().expect("dir_stack is never empty"), - component, - readlink_buf.as_mut_slice(), - ) { - Ok(link_path) => { - symlink_expansions += 1; - if symlink_expansions > MAX_SYMLINK_EXPANSIONS { - return ret_error(&mut dir_stack, host::__WASI_ELOOP); - } + Component::Normal(head) => { + let mut head = OsString::from(head); + if ends_with_slash { + // preserve trailing slash + head.push("/"); + } + + if !path_stack.is_empty() || (ends_with_slash && !needs_final_component) { + match openat( + *dir_stack.last().expect("dir_stack is never empty"), + head.as_os_str(), + OFlag::O_RDONLY | OFlag::O_DIRECTORY | OFlag::O_NOFOLLOW, + Mode::empty(), + ) { + Ok(new_dir) => { + dir_stack.push(new_dir); + continue; + } + Err(e) + // Check to see if it was a symlink. Linux indicates + // this with ENOTDIR because of the O_DIRECTORY flag. + if e.as_errno() == Some(Errno::ELOOP) + || e.as_errno() == Some(Errno::EMLINK) + || e.as_errno() == Some(Errno::ENOTDIR) => + { + // attempt symlink expansion + match readlinkat( + *dir_stack.last().expect("dir_stack is never empty"), + head.as_os_str(), + readlink_buf.as_mut_slice(), + ) { + Ok(link_path) => { + symlink_expansions += 1; + if symlink_expansions > MAX_SYMLINK_EXPANSIONS { + return ret_error(&mut dir_stack, host::__WASI_ELOOP); + } - let mut link_path = link_path.as_bytes().to_vec(); + let mut link_path = OsString::from(link_path); + if head.as_bytes().ends_with(b"/") { + link_path.push("/"); + } - // append a trailing slash if the component leading to it has one, so - // that we preserve any ENOTDIR that might come from trying to open a - // non-directory - if component.ends_with(b"/") { - link_path.push('/' as u8); + path_stack.push(link_path); + continue; + } + Err(e) => { + return ret_error( + &mut dir_stack, + host_impl::errno_from_nix(e.as_errno().unwrap()), + ); + } + } + } + Err(e) => { + return ret_error( + &mut dir_stack, + host_impl::errno_from_nix(e.as_errno().unwrap()), + ); + } } + } else if ends_with_slash + || (dirflags & host::__WASI_LOOKUP_SYMLINK_FOLLOW) != 0 + { + // if there's a trailing slash, or if `LOOKUP_SYMLINK_FOLLOW` is set, attempt + // symlink expansion + match readlinkat( + *dir_stack.last().expect("dir_stack is never empty"), + head.as_os_str(), + readlink_buf.as_mut_slice(), + ) { + Ok(link_path) => { + symlink_expansions += 1; + if symlink_expansions > MAX_SYMLINK_EXPANSIONS { + return ret_error(&mut dir_stack, host::__WASI_ELOOP); + } + let mut link_path = OsString::from(link_path); + if head.as_bytes().ends_with(b"/") { + link_path.push("/"); + } - path_stack.push(link_path); - continue; - } - Err(e) => { - let errno = e.as_errno().unwrap(); - if errno != Errno::EINVAL && errno != Errno::ENOENT { - // only return an error if this path is not actually a symlink - return ret_error(&mut dir_stack, host_impl::errno_from_nix(errno)); + path_stack.push(link_path); + continue; + } + Err(e) => { + let errno = e.as_errno().unwrap(); + if errno != Errno::EINVAL && errno != Errno::ENOENT { + // only return an error if this path is not actually a symlink + return ret_error( + &mut dir_stack, + host_impl::errno_from_nix(errno), + ); + } + } } } + + // not a symlink, so we're done; + return Ok((ret_dir_success(&mut dir_stack), head)); } } - - // not a symlink, so we're done; + } + None => { + // no further components to process. means we've hit a case like "." or "a/..", or if the + // input path has trailing slashes and `needs_final_component` is not set return Ok(( ret_dir_success(&mut dir_stack), - OsStr::from_bytes(component).to_os_string(), + OsStr::new(".").to_os_string(), )); } } - - if path_stack.is_empty() { - // no further components to process. means we've hit a case like "." or "a/..", or if the - // input path has trailing slashes and `needs_final_component` is not set - return Ok(( - ret_dir_success(&mut dir_stack), - OsStr::new(".").to_os_string(), - )); - } else { - continue; - } } } From 262cac28132ea1e912d2defc061227230f0c9ae1 Mon Sep 17 00:00:00 2001 From: Jakub Konka Date: Thu, 13 Jun 2019 19:35:52 +0200 Subject: [PATCH 3/4] Fix fd_renumber when trying to renumber a preopen --- src/sys/unix/hostcalls_impl/fs.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/sys/unix/hostcalls_impl/fs.rs b/src/sys/unix/hostcalls_impl/fs.rs index 27bbf74..ac282a2 100644 --- a/src/sys/unix/hostcalls_impl/fs.rs +++ b/src/sys/unix/hostcalls_impl/fs.rs @@ -78,6 +78,14 @@ pub(crate) fn fd_renumber( Some(fe_to) => fe_to, None => return Err(host::__WASI_EBADF), }; + + // Don't allow renumbering over a pre-opened resource. + // TODO: Eventually, we do want to permit this, once libpreopen in + // userspace is capable of removing entries from its tables as well. + if fe_from.preopen_path.is_some() || fe_to.preopen_path.is_some() { + return Err(host::__WASI_ENOTSUP); + } + if let Err(e) = nix::unistd::dup2(fe_from.fd_object.rawfd, fe_to.fd_object.rawfd) { return Err(host_impl::errno_from_nix(e.as_errno().unwrap())); } From 1eff4b58cf97bfb9c54ea70f00a3eb43db0f02fd Mon Sep 17 00:00:00 2001 From: Jakub Konka Date: Thu, 13 Jun 2019 22:46:46 +0200 Subject: [PATCH 4/4] Fix path_readlink: with a 0-sized buffer should succeed --- src/hostcalls/fs.rs | 16 ++++++++------ src/sys/unix/hostcalls_impl/fs.rs | 35 ++++++++++++++++++++++++++----- 2 files changed, 40 insertions(+), 11 deletions(-) diff --git a/src/hostcalls/fs.rs b/src/hostcalls/fs.rs index 684bd13..ea10594 100644 --- a/src/hostcalls/fs.rs +++ b/src/hostcalls/fs.rs @@ -580,16 +580,20 @@ pub fn path_readlink( Ok(slice) => host_impl::path_from_raw(slice).to_owned(), Err(e) => return enc_errno(e), }; - let rights = host::__WASI_RIGHT_PATH_READLINK; let mut buf = match dec_slice_of_mut::(memory, buf_ptr, buf_len) { Ok(slice) => slice, Err(e) => return enc_errno(e), }; - let host_bufused = - match hostcalls_impl::path_readlink(wasi_ctx, dirfd, path.as_os_str(), rights, &mut buf) { - Ok(host_bufused) => host_bufused, - Err(e) => return enc_errno(e), - }; + let host_bufused = match hostcalls_impl::path_readlink( + wasi_ctx, + dirfd, + path.as_os_str(), + host::__WASI_RIGHT_PATH_READLINK, + &mut buf, + ) { + Ok(host_bufused) => host_bufused, + Err(e) => return enc_errno(e), + }; match enc_usize_byref(memory, buf_used, host_bufused) { Ok(_) => wasm32::__WASI_ESUCCESS, Err(e) => enc_errno(e), diff --git a/src/sys/unix/hostcalls_impl/fs.rs b/src/sys/unix/hostcalls_impl/fs.rs index ac282a2..2e385a8 100644 --- a/src/sys/unix/hostcalls_impl/fs.rs +++ b/src/sys/unix/hostcalls_impl/fs.rs @@ -516,18 +516,43 @@ pub(crate) fn path_readlink( rights: host::__wasi_rights_t, buf: &mut [u8], ) -> Result { - use nix::fcntl::readlinkat; + use nix::errno::Errno; let (dir, path) = match path_get(wasi_ctx, dirfd, 0, path, rights, 0, false) { Ok((dir, path)) => (dir, path), Err(e) => return Err(e), }; - let target_path = match readlinkat(dir, path.as_os_str(), buf) { - Err(e) => return Err(host_impl::errno_from_nix(e.as_errno().unwrap())), - Ok(target_path) => target_path, + let path_cstr = match std::ffi::CString::new(path.as_bytes()) { + Ok(path_cstr) => path_cstr, + Err(_) => return Err(host::__WASI_EINVAL), }; - Ok(target_path.len()) + + // Linux requires that the buffer size is positive, whereas POSIX does not. + // Use a fake buffer to store the results if the size is zero. + // TODO: instead of using raw libc::readlinkat call here, this should really + // be fixed in `nix` crate + let fakebuf: &mut [u8] = &mut [0]; + let buf_len = buf.len(); + let len = unsafe { + libc::readlinkat( + dir, + path_cstr.as_ptr() as *const libc::c_char, + if buf_len == 0 { + fakebuf.as_mut_ptr() + } else { + buf.as_mut_ptr() + } as *mut libc::c_char, + if buf_len == 0 { fakebuf.len() } else { buf_len }, + ) + }; + + if len < 0 { + Err(host_impl::errno_from_nix(Errno::last())) + } else { + let len = len as usize; + Ok(if len < buf_len { len } else { buf_len }) + } } pub(crate) fn path_rename(