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

Use safe code for calling readlinkat #519

Closed
sunfishcode opened this issue Jun 24, 2019 · 4 comments
Closed

Use safe code for calling readlinkat #519

sunfishcode opened this issue Jun 24, 2019 · 4 comments
Labels
enhancement wasi:impl Issues pertaining to WASI implementation in Wasmtime

Comments

@sunfishcode
Copy link
Member

On Linux, readlinkat returns EINVAL if given a zero-length buffer. WASI's wasi_path_readlink should follow the POSIX semantics for readlinkat instead. wasi-common implements this, however it uses an unsafe call to the libc readlinkat rather than calling through a safe nix wrapper.

See the discussion here for more details.

@sendilkumarn
Copy link
Contributor

sendilkumarn commented Aug 21, 2019

This looks like a bug in nix.

IIUC, the following line is the problem

https://github.com/nix-rust/nix/blob/414cc86c0af09fd44454b93b6dc738316b16c43c/src/fcntl.rs#L184

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() { // *** This check ***
                Err(Error::Sys(Errno::ENAMETOOLONG))
            } else {
                Ok(OsStr::from_bytes(&buffer[..(len as usize)]))
            }
        }
    }
}

In nix testing, we are manually adding an 1 to increase the buffer size at here. I think this makes it successful.

int main(int argc, const char * argv[]) {
    
    std::string filename = "/tmp/test-dir/target";
    
    size_t bufferSize = 9;
    char* buffer = new char[bufferSize];
    size_t rc = readlink (filename.c_str(), buffer, bufferSize); 
   // Since `readlink's` output depends on the `bufferSize`. If the`bufferSize` is 9 or greater than 
   // 9 then it will return 9 or else it will return `bufferSize` as the return value. 

    return 0;
}

@sendilkumarn
Copy link
Contributor

Created nix-rust/nix#1109 (hopefully that will fix the error).

@kubkon
Copy link
Member

kubkon commented Aug 23, 2019

Nice one, thanks!

marmistrz referenced this issue in marmistrz/wasi-common Aug 29, 2019
Currently, the fixed version of `readlinkat` is provided in upstream
[nix](https://github.com/nix-rust/nix) crate only, therefore, we
need to change `nix` to be a git dependency, which is not ideal.
However, a new release should be out soon-ish, hence, I reckon it's
OK to go with it as-is for now, and change to a semver when a new
version gets released.

Changes:
* uses `nix::fcntl::readlinkat` instead of a direct call to
  `libc::readlinkat`
* if read target path is longer than the buffer provided,
  pads the buffer with 0s
@kubkon kubkon transferred this issue from CraneStation/wasi-common Nov 8, 2019
@kubkon kubkon added enhancement wasi:impl Issues pertaining to WASI implementation in Wasmtime labels Nov 8, 2019
@kubkon
Copy link
Member

kubkon commented Dec 11, 2019

Similarly to the story in #521, given that we've decided to replace nix dependency with our own thin, low-level in-house wrapper crate yanix (see #649), I'm closing this issue as it's no longer relevant.

@kubkon kubkon closed this as completed Dec 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement wasi:impl Issues pertaining to WASI implementation in Wasmtime
Projects
None yet
Development

No branches or pull requests

3 participants