RPC alignment and size calculation #159

New Issue

pca006132 · 2022-01-04T17:18:14+08:00

pca006132 commented

2022-01-04 17:18:14 +08:00

I am trying to fix https://github.com/m-labs/artiq/issues/1810 , and is currently reviewing our RPC code. There are a couple of potential problems in our code. I am unable to devise an example that can cause memory corruption with these, but I think we should probably get them fixed and make sure that they are correct.

https://git.m-labs.hk/M-Labs/artiq-zynq/src/branch/master/src/runtime/src/rpc.rs#L76-L108

        Tag::List(it) => {
            #[repr(C)]
            struct List { elements: *mut (), length: u32 }
            consume_value!(List, |ptr| {
                let length = proto_async::read_i32(stream).await? as usize;
                (*ptr).length  = length as u32;
                let tag = it.clone().next().expect("truncated tag");
                let mut data = alloc(tag.size() * length as usize).await;
                (*ptr).elements = data;
                match tag {
                    // ...
                    Tag::Int64 | Tag::Float64 => {
                        let ptr = align_ptr_mut::<u64>(data);
                        let dest = core::slice::from_raw_parts_mut(ptr as *mut u8, length * 8);
                        proto_async::read_chunk(stream, dest).await?;
                        drop(dest);
                        let dest = core::slice::from_raw_parts_mut(ptr, length);
                        NativeEndian::from_slice_u64(dest);
                    },

There are two problems here.

The (*ptr).elements may be different from the slice pointer (ptr = align_ptr_mut::<u64>(data)), as the latter one is aligned to 8 bytes while the alloc is only guaranteed to align to 4 bytes boundary.
This also means that it is possible for us to write over our allocated memory by 4 bytes, as the align_ptr_mut may cause ptr to be 4 bytes after data.

The fix I'm thinking about is to

Allocate 4 more bytes for i64/f64, to avoid writing past the allocated buffer. (we will at most need 4 more bytes, and it will not affect other types)
Align the data pointer before setting (*ptr).elements.

Another way would be to make alloca align to 8 bytes boundary, but this will waste memory for other types.

I think the code for array is also problematic for the same reason.

Apart from the above bugs, I'm also thinking about what is the best way to fix the tuple alignment and size calculation bug. We can write a function to calculate the alignment in our RPC code, or embed the alignment and size requirement for tuples in the tag which can be emitted by the compiler.

I am trying to fix https://github.com/m-labs/artiq/issues/1810 , and is currently reviewing our RPC code. There are a couple of potential problems in our code. I am unable to devise an example that can cause memory corruption with these, but I think we should probably get them fixed and make sure that they are correct. https://git.m-labs.hk/M-Labs/artiq-zynq/src/branch/master/src/runtime/src/rpc.rs#L76-L108 ```rust Tag::List(it) => { #[repr(C)] struct List { elements: *mut (), length: u32 } consume_value!(List, |ptr| { let length = proto_async::read_i32(stream).await? as usize; (*ptr).length = length as u32; let tag = it.clone().next().expect("truncated tag"); let mut data = alloc(tag.size() * length as usize).await; (*ptr).elements = data; match tag { // ... Tag::Int64 | Tag::Float64 => { let ptr = align_ptr_mut::<u64>(data); let dest = core::slice::from_raw_parts_mut(ptr as *mut u8, length * 8); proto_async::read_chunk(stream, dest).await?; drop(dest); let dest = core::slice::from_raw_parts_mut(ptr, length); NativeEndian::from_slice_u64(dest); }, ``` There are two problems here. 1. The `(*ptr).elements` may be different from the slice pointer (`ptr = align_ptr_mut::<u64>(data)`), as the latter one is aligned to 8 bytes while the alloc is only guaranteed to align to 4 bytes boundary. 2. This also means that it is possible for us to write over our allocated memory by 4 bytes, as the `align_ptr_mut` may cause `ptr` to be 4 bytes after `data`. The fix I'm thinking about is to 1. Allocate 4 more bytes for i64/f64, to avoid writing past the allocated buffer. (we will at most need 4 more bytes, and it will not affect other types) 2. Align the data pointer before setting `(*ptr).elements`. Another way would be to make alloca align to 8 bytes boundary, but this will waste memory for other types. I think the code for array is also problematic for the same reason. --- Apart from the above bugs, I'm also thinking about what is the best way to fix the tuple alignment and size calculation bug. We can write a function to calculate the alignment in our RPC code, or embed the alignment and size requirement for tuples in the tag which can be emitted by the compiler.

pca006132 referenced a pull request that will close this issue

2022-01-04 18:28:02 +08:00

runtime/rpc: fixes alignment and size problem #160

sb10q closed this issue

2022-01-05 08:24:03 +08:00

Sign in to join this conversation.