Closed ogerlach closed 1 year ago
Hi,
Your fix is correct. What I don't understand is why this panic didn't show up before!
FYI, here is an example of (generated) code using serialize
.
wire_buf
is offset by the value returned by the previous serialize
.
/// Construct a new [Pictforminfo].
#[allow(unused_assignments, unused_unsafe)]
pub fn new(
id: Pictformat,
r#type: PictType,
depth: u8,
direct: Directformat,
colormap: xproto::Colormap,
) -> Pictforminfo {
unsafe {
let mut wire_buf = [0u8; 28];
let mut wire_off = 0usize;
wire_off += id.serialize(&mut wire_buf[wire_off..]);
wire_off +=
(std::mem::transmute::<_, u32>(r#type) as u8).serialize(&mut wire_buf[wire_off..]);
wire_off += depth.serialize(&mut wire_buf[wire_off..]);
wire_off += 2; // pad
wire_off += direct.serialize(&mut wire_buf[wire_off..]);
wire_off += colormap.serialize(&mut wire_buf[wire_off..]);
Pictforminfo { data: wire_buf }
}
}
I was wondering about the same thing. It initially caused me to believe that I am wrong about the bug. Luckily the comment clearly stated the intended behavior. The last days I looked deeper into the issue. I am not fully sure (would.need more investigation) but I think that overall it comes down to this: Most types (e.g. primitive types) are not affected as they have another implementation of WiredOut
. It is only structs inside structs. As one can see from the diff, the change affects quite some types but not this many central ones. And even for these types, the slices would be of same length if a struct to be serialized is the last type to be serialized. In this case, there would be no panic as well.
So I see a possible explanation why this problem is not as pervasive as looks at first glance. However, this strongly depends what features are actually used by users of this crate. I cannot say much about other extentions but for render
there is a problem when working with pictures. The core
protocol can be used without trouble. So if most users are fine with not affected features of core
and some extensions, the error would not manifest. This is quite likely imho.
Keep in mind that this mostly guesswork and not a throughout investigation. But it is enough for me at this point.
When creating a new PictformInfo() I received the following error:
thread 'test::test_pictforminfo' panicked at 'source slice length (16) does not match destination slice length (20)', /home/oliver/Dokumente/dev/reMenu/target/debug/build/xcb-22e5b0ca74ae1e36/out/render.rs:639:18
This snippet can trigger the error:
After some testing, I found that WireOut::serialize does not truncate data passed to it but consumes the whole chunk. From the documentation and its use in the code base, I concluded that this is not the intended behaviour. In case my conclusion was right, this PR fixes it.
As requested, here is the diff of the generated code: