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

recvmsg: incorrect lifetime returned #2083

Closed
yuval-k opened this issue Aug 2, 2023 · 4 comments · Fixed by #2095
Closed

recvmsg: incorrect lifetime returned #2083

yuval-k opened this issue Aug 2, 2023 · 4 comments · Fixed by #2095

Comments

@yuval-k
Copy link

yuval-k commented Aug 2, 2023

the current signature of recvmsg is:

pub fn recvmsg<'a, 'outer, 'inner, S>(fd: RawFd, iov: &'outer mut [IoSliceMut<'inner>],
                   mut cmsg_buffer: Option<&'a mut Vec<u8>>,
                   flags: MsgFlags) -> Result<RecvMsg<'a, 'inner, S>>

note the iov param: iov: &'outer mut [IoSliceMut<'inner>]

now in the resulting RecvMsg we can access the iovs using a method named iovs(). This method iterates the iov array provided. But the lifetime of the iov array is 'outer and not 'inner.

So the signature of the result should be changed to Result<RecvMsg<'a, 'outer, S>>.

If all sounds correct, i can open a PR to fix this.

example fail scenario with tokio (note the move closure):

            let tokio_stream = /* get a stream somehow... */;
            let mut buffer = vec![0u8; 10];

            let mut iov = [IoSliceMut::new(&mut buffer)];
            
            let res: Result<nix::sys::socket::RecvMsg<'_, '_, ()>, std::io::Error> =
                tokio_stream.try_io(tokio::io::Interest::READABLE, move || {
                    recvmsg::<()>(
                        raw_fd,
                        &mut iov,
                        None,
                        MsgFlags::empty(),
                    )
                    .map_err(|e| std::io::Error::from_raw_os_error(e as i32))
                });
            match res {
                Ok(res) => {
// BUG HAPPENS HERE
// NOTE: here res.iovs() uses the iov array that was moved in the try_io closure
// res.iovs() may point to invalid memory, as iov may have been destroyed already.
                }
                Err(e) => { /* ... */  }
            };
    }
@asomers
Copy link
Member

asomers commented Aug 6, 2023

Yes, I think your analysis is correct. Would you submit a PR?

@yuval-k
Copy link
Author

yuval-k commented Aug 7, 2023

I can but I'm not available to do it this week. I can submit one next week. Please feel free to fix if you require this sooner.

asomers added a commit to asomers/nix that referenced this issue Aug 11, 2023
asomers added a commit to asomers/nix that referenced this issue Aug 11, 2023
cgwalters added a commit to cgwalters/containers-image-proxy-rs that referenced this issue Aug 28, 2023
An updated nix fixes an incorrect lifetime.

cc nix-rust/nix#2083
cgwalters added a commit to cgwalters/containers-image-proxy-rs that referenced this issue Aug 28, 2023
An updated nix fixes an incorrect lifetime.

cc nix-rust/nix#2083

Signed-off-by: Colin Walters <walters@verbum.org>
@cgwalters
Copy link

Good catch on this! Looks like one of my projects was affected containers/containers-image-proxy-rs#52

I guess any potentially problematic code will now fail to compile, which is good. I wonder if this could use a bit more visibility though with e.g. a rustsec advisory?

@cgwalters
Copy link

cgwalters commented Aug 28, 2023

Using a GH code search I'm seeing a few other repositories likely affected too.

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

Successfully merging a pull request may close this issue.

3 participants