Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use dynamically sized slices to prevent UB from reading an array out of bounds #7

Merged

Conversation

asquared31415
Copy link
Contributor

This is a breaking change due to changing the type of publicly accessible fields. Bumps version to 0.3.0

Using a [T; 0] to access more than 0 elements (that is, to access any data) at that location is undefined behavior. This PR moves to a dynamically sized slice approach to not cause undefined behavior in that way and better mirrors the exact data layout of the data being accessed.

@Andy-Python-Programmer
Copy link
Owner

This is a breaking change due to changing the type of publicly accessible fields. Bumps version to 0.3.0

Using a [T; 0] to access more than 0 elements (that is, to access any data) at that location is undefined behavior. This PR moves to a dynamically sized slice approach to not cause undefined behavior in that way and better mirrors the exact data layout of the data being accessed.

According to the rust nomicon, it states that the following is only if the struct is following the Rust ABI. Since all of the structs in the crate use the C ABI, its perfectly fine to create zero sized arrays.

image

As you can see the following example code from the rust nomicon describing the undefined behaviour you described above, the structs follow the Rust ABI.

If the following was true for if the structure was using the C ABI aswell then it will cause an ABI breakage since the GCC specification states the following Declaring zero-length arrays is allowed in GNU C as an extension. A zero-length array can be useful as the last element of a structure that is really a header for a variable-length object (https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html)

@Andy-Python-Programmer Andy-Python-Programmer self-assigned this Mar 9, 2022
@repnop
Copy link

repnop commented Mar 9, 2022

The layout isn't the point here -- pointers have what's called provenance, and that provenance is what is being affected here. When you create a pointer to a zero-length array, you can access exactly 0 bytes with that pointer, any other accesses are beyond the allocation scope of that pointer, and thus its UB. For example, if you run the following code with MIRIFLAGS="-Zmiri-tag-raw-pointers" cargo +nightly miri run:

#[repr(C)]
struct Impl {
    len: u32,
    c: [u32; 4],
}

#[repr(C)]
struct Flex {
    len: u32,
    c: [u32; 0],
}

impl Flex {
    fn slice(&self) -> &[u32] {
        unsafe { core::slice::from_raw_parts(self.c.as_ptr(), self.len as usize) }
    }
}

fn main() {
    let impl_ = Impl {
        len: 4,
        c: [3, 4, 5, 6],
    };
    let flex = unsafe { &*(&impl_ as *const _ as *const Flex) };
    println!("{:?}", flex.slice());
}

we get confirmation that this is UB:

error: Undefined Behavior: trying to reborrow for SharedReadOnly at alloc959+0x4, but parent tag <2000> does not have an appropriate item in the borrow stack
  --> /home/<snip>/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/slice/raw.rs:92:14
   |
92 |     unsafe { &*ptr::slice_from_raw_parts(data, len) }
   |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ trying to reborrow for SharedReadOnly at alloc959+0x4, but parent tag <2000> does not have an appropriate item in the borrow stack
   |
   = help: this indicates a potential bug in the program: it performed an invalid operation, but the rules it violated are still experimental
   = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
           
   = note: inside `std::slice::from_raw_parts::<u32>` at /home/<snip>/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/slice/raw.rs:92:14
note: inside `Flex::slice` at src/main.rs:15:18
  --> src/main.rs:15:18
   |
15 |         unsafe { core::slice::from_raw_parts(self.c.as_ptr(), self.len as usize) }
   |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside `main` at src/main.rs:25:22
  --> src/main.rs:25:22
   |
25 |     println!("{:?}", flex.slice());
   |                      ^^^^^^^^^^^^

@Andy-Python-Programmer Andy-Python-Programmer merged commit 8b0ce9a into Andy-Python-Programmer:master Mar 10, 2022
@Andy-Python-Programmer
Copy link
Owner

Thanks @asquared31415!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants