Skip to content
This repository has been archived by the owner on Nov 9, 2019. It is now read-only.

Nix fixes #27

Merged
merged 4 commits into from
Jun 24, 2019
Merged

Nix fixes #27

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
36e9ecd
Add fixes so that misc-tests pass
Jun 11, 2019
2003de6
Refactor path_get and fix flags in path_open
Jun 13, 2019
262cac2
Fix fd_renumber when trying to renumber a preopen
Jun 13, 2019
1eff4b5
Fix path_readlink: with a 0-sized buffer should succeed
Jun 13, 2019
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
44 changes: 27 additions & 17 deletions src/hostcalls/fs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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]
Expand Down Expand Up @@ -574,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::<u8>(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),
Expand Down
80 changes: 70 additions & 10 deletions src/sys/unix/hostcalls_impl/fs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()));
}
Expand Down Expand Up @@ -337,10 +345,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
Expand All @@ -353,7 +361,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
Expand Down Expand Up @@ -508,18 +516,43 @@ pub(crate) fn path_readlink(
rights: host::__wasi_rights_t,
buf: &mut [u8],
) -> Result<usize, host::__wasi_errno_t> {
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),
};

// 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 },
)
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix for the zero-length readlink buffer in bytecodealliance/lucet#213 looks a little simpler; would it make sense to do the same thing here? That said, I do like the comment here about investigating having nix take care of this detail transparently, so please preserve that comment in any case :-).

Copy link
Member Author

@kubkon kubkon Jun 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, do you know if they've double checked it for this particular use case? I'm asking because actually, I've already tried something similar before, but due to the way nix::fcntl::readlinkat is implemented, it wouldn't work for me. Perhaps I was doing something wrong. But to elaborate, here's the readlinkat impl taken verbatim from the nix crate (here's the link):

pub fn readlinkat<'a, P: ?Sized + NixPath>(dirfd: RawFd, path: &P, buffer: &'a mut [u8]) -> Result<&'a OsStr> {
    let res = path.with_nix_path(|cstr| {
        unsafe { libc::readlinkat(dirfd, cstr.as_ptr(), buffer.as_mut_ptr() as *mut c_char, buffer.len() as size_t) }
    })?;

    wrap_readlink_result(buffer, res)
}

And in turn wrap_readlink_result does the following:

fn wrap_readlink_result(buffer: &mut[u8], res: ssize_t) -> Result<&OsStr> {
    match Errno::result(res) {
        Err(err) => Err(err),
        Ok(len) => {
            if (len as usize) >= buffer.len() {
                Err(Error::Sys(Errno::ENAMETOOLONG))
            } else {
                Ok(OsStr::from_bytes(&buffer[..(len as usize)]))
            }
        }
    }
}

Due to the wrap_readlink_result, if we ever try to read link that's longer than the buffer we provide, nix will trip with ENAMETOOLONG which is exactly the error I was getting when trying the same approach as bytecodealliance/lucet#213 for zero-length buffer.

@jedisct1, @sunfishcode any thoughts and help on this will be much appreciated! :-)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the comment above, when I said "it wouldn't work for me" I meant that bytecodealliance/lucet#213 approach wouldn't pass the test_readlink_no_buffer test in sunfishcode/misc-tests. As I pointed out above, this is due to the fact that, the nix crate doesn't only wrap libc::readlinkat syscall but actually checks whether the passed in buffer is larger than the actual number of bytes written to the buffer. So, even if we pass in a buffer of length 1, due to the above additional check, I believe that nix::fcntl::readlinkat will always return ENAMETOOLONG for that particular case.

Ok(target_path.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(
Expand Down Expand Up @@ -778,7 +811,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))
}
}
}

Expand Down
Loading