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

merged 4 commits into from
Jun 24, 2019

Conversation

kubkon
Copy link
Member

@kubkon kubkon commented Jun 11, 2019

This PR builds upon #26. It patches up all problems that cause sunfishcode/misc-tests to fail.

Summary of fixes:

  • fd_renumber should currently fail when trying to renumber a preopen
  • path_readlink should be able to handle 0-sized buffer (as pointed out by @sunfishcode in the original C impl, Linux requires the input buffer to have positive size, however, POSIX does not enforce that)
  • path_open had the OFlags the other way around: the else branch when determining read/write attributes should have OFlags::O_RDONLY as a fallthrough and not OFlags::O_WRONLY as it was until now
  • path_get now correctly handles "interesting paths" with a plethora of trailing slashes, "..", etc. To achieve this, instead of iterating over raw paths (using bytes), we use std::path::Components functionality instead; it is still necessary to preserver the path's trailing slash so that opening a file as a dir fails as expected
  • path_get now correctly fails with EILSEQ in the presence of spurious NULs in the input path
  • applies fix Fixes incorrect guest fd encoding bytecodealliance/wasmtime#173 to path_open
  • fixes a typo in path_open to do with setting the needed_base flags in the presence of OFlag::O_TRUNC: up to now we were incorrectly adjusting needed_inheriting flags instead of needed_base
  • finally, path_unlink_file now correctly handles non-Linux *nix OSes, i.e., unlinkat may return EPERM on non-Linux host and it should be adjusted to EISDIR

@kubkon kubkon requested review from sunfishcode and jedisct1 June 13, 2019 21:04
@kubkon kubkon marked this pull request as ready for review June 13, 2019 21:04
Copy link
Member

@sunfishcode sunfishcode left a comment

Choose a reason for hiding this comment

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

Very cool!

} 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

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.

@sunfishcode
Copy link
Member

Ok, the PR here looks good to me. I'm going to file a separate issue about readlinkat so we can continue to track that without holding up other progress.

@sunfishcode sunfishcode merged commit dc05d89 into CraneStation:master Jun 24, 2019
@sunfishcode
Copy link
Member

@kubkon kubkon deleted the nix-fixes branch June 24, 2019 20:38
@kubkon
Copy link
Member Author

kubkon commented Jun 24, 2019

I've now filed #30.

Nice one, thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants