Hi,

I've coded something and asked chatgpt that told me it's wrong to do the following.
In the modular_bitfield crate, the underlying storage is [u8;N], with N the size of the struct.

use modular_bitfield::prelude::*;

        #[bitfield]
        #[derive(Debug, Default)]

        struct MyFrame {
                length:     B24,
                typed:      B8,
                unused:     B8,
                reserved:   B1,
                identifier: B31,
                reserved2:  B1,
                other:      B31
        }

        #[test]

        fn t0() {

                let mut large_vec = vec![0_u8; 1024];

                let my_struct = MyFrame::new()
                        .with_length(65879)
                        .with_typed(45)
                        .with_identifier(589764)
                        .with_other(6);

                let bytes = my_struct.into_bytes();

                let dbg = &large_vec[5..5 + bytes.len()];

                assert_eq!(dbg.len(), bytes.len());

                large_vec[5..5 + bytes.len()].copy_from_slice(&bytes);

                large_vec[51..51 + bytes.len()].copy_from_slice(&bytes);


                let s0 = unsafe {

                        let ptr = large_vec[5..].as_ptr() as *const MyFrame;

                        ptr.as_ref_unchecked()
                };


                assert_eq!(s0.length() as u32, 65879);

                assert_eq!(s0.typed() as u32, 45);

                assert_eq!(s0.identifier() as u32, 589764);

                assert_eq!(s0.other() as u32, 6);


                let s0 = unsafe {

                        let ptr = large_vec[51..].as_ptr() as *const MyFrame;

                        ptr.as_ref_unchecked()
                };

                assert_eq!(s0.length() as u32, 65879);

                assert_eq!(s0.typed() as u32, 45);

                assert_eq!(s0.identifier() as u32, 589764);

                assert_eq!(s0.other() as u32, 6);
        }

This is the struct definition expanded macro:

#[derive(Default)]

struct MyFrame {
        bytes: [::core::primitive::u8;
                (((<B24 as ::modular_bitfield::Specifier>::BITS
                        + <B8 as ::modular_bitfield::Specifier>::BITS
                        + <B8 as ::modular_bitfield::Specifier>::BITS
                        + <B1 as ::modular_bitfield::Specifier>::BITS
                        + <B31 as ::modular_bitfield::Specifier>::BITS
                        + <B1 as ::modular_bitfield::Specifier>::BITS
                        + <B31 as ::modular_bitfield::Specifier>::BITS
                        - 1)
                        / 8)
                        + 1)
                        * 8
                        / 8]
}

const _: () = {

        impl ::modular_bitfield::private::checks::CheckTotalSizeMultipleOf8 for MyFrame {
                type Size = ::modular_bitfield::private::checks::TotalSize<
                        ::modular_bitfield::private::checks::BitCount<
                                {

                                        (<B24 as ::modular_bitfield::Specifier>::BITS
                                                + <B8 as ::modular_bitfield::Specifier>::BITS
                                                + <B8 as ::modular_bitfield::Specifier>::BITS
                                                + <B1 as ::modular_bitfield::Specifier>::BITS
                                                + <B31 as ::modular_bitfield::Specifier>::BITS
                                                + <B1 as ::modular_bitfield::Specifier>::BITS
                                                + <B31 as ::modular_bitfield::Specifier>::BITS)
                                                % 8
                                }
                        >
                >;
        }
};

The code is to be run on x86_64 arches only.

So far, the results are consistent and the test completes without issue. Chat tells me that it's very dangerous and INCORRECT and that I should use the from_bytes method, which incurs a copy. But that fact that the underlying storage is a [u8;N] tells me that the crate handles unaligned reads perfectly.

So I went ahead and found that all the paths are selected at compile time and that there is no risk.

pub fn read_specifier<T>(bytes: &[u8], offset: usize) -> <T as Specifier>::Bytes
where
    T: Specifier,
    PushBuffer<T::Bytes>: Default + PushBits,
{
    let end = offset + <T as Specifier>::BITS;
    let ls_byte = offset / 8; // compile-time
    let ms_byte = (end - 1) / 8; // compile-time

    // Truncation is always valid due to mod 8 value range
    #[allow(clippy::cast_possible_truncation)]
    let lsb_offset = (offset % 8) as u32; // compile-time
    #[allow(clippy::cast_possible_truncation)]
    let msb_offset = (end % 8) as u32; // compile-time
    let msb_offset = if msb_offset == 0 { 8 } else { msb_offset };

    let mut buffer = push_buffer::<T>();

    if lsb_offset == 0 && msb_offset == 8 {
        // Edge-case for whole bytes manipulation.
        for byte in bytes[ls_byte..=ms_byte].iter().rev() {
            buffer.push_bits(8, *byte);
        }
    } else {
        if ls_byte != ms_byte {
            // Most-significant byte
            buffer.push_bits(msb_offset, bytes[ms_byte]);
        }
        if ms_byte - ls_byte >= 2 {
            // Middle bytes
            for byte in bytes[(ls_byte + 1)..ms_byte].iter().rev() {
                buffer.push_bits(8, *byte);
            }
        }
        if ls_byte == ms_byte {
            buffer.push_bits(
                u32::try_from(<T as Specifier>::BITS).unwrap(),
                bytes[ls_byte] >> lsb_offset,
            );
        } else {
            buffer.push_bits(8 - lsb_offset, bytes[ls_byte] >> lsb_offset);
        }
    }
    buffer.into_bytes()
}

Look, this code is to be used in a very performance sensitive context, we are deep into unsafe territory here. Is my understanding ok? I think Chat is overly conservative here.

1 post - 1 participant

Read full topic