interprocess icon indicating copy to clipboard operation
interprocess copied to clipboard

Error in decoding length of AncillaryData (forget to remove the header offset to the data length)

Open ETKNeil opened this issue 4 years ago • 0 comments

If you look at your encoding you will see that you encode the length of the payload as header + data length https://github.com/kotauskas/interprocess/blob/master/src/os/unix/udsocket/ancillary.rs#L94

            let mut cmsg_len = size_of::<cmsghdr>();  // here 16 or 12
            let cmsg_level_bytes = SOL_SOCKET.to_ne_bytes();
            let cmsg_type_bytes;

            match i {
                AncillaryData::FileDescriptors(fds) => {
                    cmsg_type_bytes = SCM_RIGHTS.to_ne_bytes();
                    cmsg_len += fds.len() * 4; // plus data size
                    // #[cfg(target_pointer_width = "64")]
                    // this was here, I don't even remember why, but that
                    // wouldn't compile on a 32-bit machine
                    let cmsg_len_bytes = cmsg_len.to_ne_bytes(); 
                    buffer.extend_from_slice(&cmsg_len_bytes); // header size + data size

But when you actually decode it you are forgetting that part https://github.com/kotauskas/interprocess/blob/master/src/os/unix/udsocket/ancillary.rs#L303

        // The first field is the length, which is a size_t -> full size as header size + data size
        #[cfg(target_pointer_width = "64")]
        let element_size = {
            if bytes.len() - self.i < 8 {
                self.i = end;
                return None;
            }
            u64_from_slice(&bytes[self.i..self.i + 8]) as usize
        };
        #[cfg(target_pointer_width = "32")]
        let element_size = {
            if bytes.len() - self.i < 4 {
                self.i = end;
                return None;
            }
            u32_from_slice(&bytes[self.i..self.i + 4]) as usize
        };
     // SNIP
        // Update the counter before returning. -> you are adding header size + full size= header_size + data size so its already off by 16 or 12 here
        self.i += element_offset // cmsg_size, cmsg_level and cmsg_type
                + element_size; // data size
      // SNIP
                // We're reading one or multiple descriptors from the ancillary data payload.
                // All descriptors are 4 bytes in size — leftover bytes are discarded thanks
                // to integer division rules
               // here you are dividing header size + data size by 4 which means that in best case you add 3 more descriptors in worse you add 4 more (since 64 bits is 16bits for header size)
                let amount_of_descriptors = element_size / 4;

The fix is relatively simple just add this here https://github.com/kotauskas/interprocess/blob/master/src/os/unix/udsocket/ancillary.rs#L319

        };
        
        // You should never have an empty payload, if it is then return
        let element_size=match element_size.checked_sub(std::mem::size_of::<cmsghdr>()){
            None => {
                self.i = end;
                return None;
            }
            Some(i) if i==0 => {
                self.i = end;
                return None;
            }
            Some(i) => {
                i
            }
        };
        
        // The cmsg_level field is always SOL_SOCKET — we don't need it, let's get the

this remove the header size offset of the element_size (which I would suggest to rename to elements_size) thus getting only the data size here. Commit for reference: https://github.com/ETKNeil/interprocess/commit/b1de6339d6ecdfc6fb0122a7be2d973898739039

ETKNeil avatar Oct 05 '21 14:10 ETKNeil