Misaligned reference constructed by ControlMessage::ScmTimestamp

[Ralith]

On macOS (and maybe other systems?) cmsg data is 4-byte aligned. In 64-bit macOS environments, libc::timeval contains 8-byte values, and is presumably 8-byte aligned. ControlMessage directly converts the pointer to the cmsg data into a typed reference, which is therefore apparently unaligned and incurs UB. It seems like the only safe thing to do here would be to use ptr::read_unaligned.

[asomers]

Do you have an example program demonstrating the bug?

[Ralith]

No, I don't have access to a macOS machine to test on, and at any rate you can mostly get away with ignoring alignment on modern x86. The chain of logic is straightforward, though; the alignment of libc::timeval is easily observed, as is the alignment of the pointers handled by ControlMessage. See related discussion at http://austingroupbugs.net/view.php?id=978.

[asomers]

This is hard to fix in a backwards-compatible way. I see three options:

1. Make ControlMessage take its data by value rather than reference. This would be easy, but backwards-incompatible. Virtually all sendmsg users would have to update their code. Plus, it would add an extra data copy to sendmsg and an extra allocation for ScmRights (because the file descriptors would have to be stored in a Vec instead of an array. Some recvmsg code may have to change, but most would compile just fine.

2. Change RecvMsg's iterator's output type to a new struct that would own its contents and would have a method that returns a ControlMessage which references the new struct's owned value. This would require no changes to sendmsg, but all recvmsg users would have to add the extra call. Performance impact would be minor.

3. Add a new non-public ControlMessageOwned type. CmsgIterator would have a Vec of these things. Every call to CmsgIterator::next would create a new one and shove it into the Vec for storage, then return a ControlMessage that references the stored data. This would require no changes to user code whatsoever, but requires allocating the extra Vec. It also requires holding all of those stored types in the iterator until the iterator is dropped.

4. Get clever about alignment. Some control messages, like ControlMessage::ScmRights, probably have sufficient alignment on every platform. So in combination with one of the above approaches, we could optimize some control messages to skip the extra copy in decode_from and/or the extra storage in the iterator.

What do you think @Susurrus ? I'm inclined to go with option 3 because it's simplest for users, even though it's complicated for us.

[Ralith]

I've been imagining something like:

pub enum ControlMessage<'a> {
    ScmRights(ScmRights<'a>),
    Timestamp(Timestamp<'a>),
    // ...
}
pub struct ScmRights<'a>(&'a [u8]);
impl ScmRights {
    pub fn iter(&self) -> impl Iterator<Item=c_int> {
        self.0.chunks(mem::size_of::<c_int>())
              .map(|x| NativeEndian::read_uint(x, mem::size_of::<c_int>()) as _)
    }
}
pub struct Timestamp<'a>(&'a [u8]);
impl Timestamp {
    pub fn get(&self) -> timeval { unsafe { ptr::read_unaligned(self.0.as_ptr() as _) } }
}

This avoids any extra allocation or copying, in effect just forcing people to use unaligned reads where needed. It's a breaking change, but any change other than silently allocating a suitably-aligned buffer and copying into it behind the scenes would be breaking.