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

sendmsg: Fix padding for ControlMessages, and zero it rather than leave it uninitialised #874

Closed
wants to merge 1 commit into from

Conversation

alecmocatta
Copy link
Contributor

@alecmocatta alecmocatta commented Mar 19, 2018

I hit a valgrind complaint sending a single fd over a single ControlMessage. Nix was handing a length including padding to the sendmsg syscall, but the padding bytes were unaddressable (i.e. not just uninitialised, but after the end of the Vec, which is not good).

This fixes that. It possibly fixes/affects #756, as the old behaviour of not padding between ControlMessages despite the padded length being what's passed to the syscall seemed wrong. That however is yet to be tested.

Feel free to not accept until I/someone else gets a chance to more thoroughly test, I'm making it so that if someone else bumps into this problem they have something to go on in the meantime.

@asomers
Copy link
Member

asomers commented Mar 21, 2018

Thanks for the bug report. Could you also please add a test case that demonstrates the problem? Perhaps a unit test for encode_into?

@przygienda
Copy link

Sigh, the encoding seems to be broken beyond a single cmsg. When several cmsg are given only first is encoded but the size blows up to the point kernel starts to throw back excessive length. On top copy seems to be weird as well, it seems to copy @ the end. In short, it shows there are not unit tests for that stuff ... Yes, unit-test for encode_into needed ...

@przygienda
Copy link

Did more work & IMO there's a clear bug when you add stuff where cmsg.len() is not = cmsg.size(). Here's the necessary diff ... After I fixed that one multiple CMESG started to work so it seems the padding alignment code is actually working ...

pub fn sendmsg<'a>(fd: RawFd, iov: &[IoVec<&'a [u8]>], cmsgs: &[ControlMessage<'a>], flags: MsgFlags, addr: Option<&'a SockAddr>) -> Result<usize> {
    let mut capacity = 0;
    for cmsg in cmsgs {
        capacity += cmsg.space();
    }
    // Note that the resulting vector claims to have length == capacity,
    // so it's presently uninitialized.
    let mut cmsg_buffer = unsafe {
        let mut vec = Vec::<u8>::with_capacity(capacity);
        vec.set_len(capacity);
        vec
    };
    {
        let mut ofs = 0;
        for cmsg in cmsgs {
            let mut ptr = &mut cmsg_buffer[ofs ..];
            unsafe { cmsg.encode_into(&mut ptr) };
            ofs += cmsg.space();
        }
    }

@asomers
Copy link
Member

asomers commented May 19, 2018

Thanks for tracking this down! Could you please submit your change in PR form, along with a test case?

bors bot added a commit that referenced this pull request Jul 5, 2018
918: Fix passing multiple file descriptors / control messages via sendmsg r=asomers a=jonas-schievink

Fixes #464
Closes #874 because it's incorporated here
Closes #756 because it adds the test from that issue (with fixes)

Co-authored-by: alecmocatta <alec@mocatta.net>
bors bot added a commit that referenced this pull request Jul 5, 2018
918: Fix passing multiple file descriptors / control messages via sendmsg r=asomers a=jonas-schievink

Fixes #464
Closes #874 because it's incorporated here
Closes #756 because it adds the test from that issue (with fixes)

Co-authored-by: alecmocatta <alec@mocatta.net>
@bors bors bot closed this in #918 Jul 5, 2018
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 this pull request may close these issues.

None yet

3 participants