Possible unsoundness in rtc_Bytes::as_slice

[lwz23]

Description:
The implementation of rtc_Bytes::as_slice is unsound and may lead to undefined behavior (UB) when the count field exceeds the actual allocated size of the memory pointed to by ptr. This occurs because the method uses std::slice::from_raw_parts without validating whether the pointer and length are valid, potentially causing out-of-bounds memory access.

PoC

use libc::size_t;

pub struct rtc_Bytes<'a> {
    pub ptr: *const u8,
    pub count: size_t,
    phantom: std::marker::PhantomData<&'a u8>,
}

impl<'a> Default for rtc_Bytes<'a> {
    fn default() -> Self {
        Self {
            ptr: std::ptr::null(),
            count: 0,
            phantom: std::marker::PhantomData,
        }
    }
}

impl<'a> rtc_Bytes<'a> {
    pub fn as_slice(&self) -> &[u8] {
        if self.ptr.is_null() {
            return &[];
        }
        unsafe { std::slice::from_raw_parts(self.ptr, self.count) }
    }

    pub fn to_vec(&self) -> Vec<u8> {
        self.as_slice().to_vec()
    }
}
fn main() {
    // Case: Pointer is valid, but count exceeds the allocated memory size
    let mut a = rtc_Bytes::default();
    let data = [1, 2, 3];
    a.ptr = data.as_ptr();
    a.count = 10; // Deliberately set count to exceed the actual size of `data`

    // Directly call `as_slice` to trigger potential UB
    let _ = a.as_slice();
}

run with miri

PS E:\Github\lwz> cargo +nightly miri run
   Compiling libc v0.2.167
   Compiling lwz v0.1.0 (E:\Github\lwz)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.45s
     Running `C:\Users\ROG\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\bin\cargo-miri.exe runner target\miri\x86_64-pc-windows-msvc\debug\lwz.exe`
warning: type `rtc_Bytes` should have an upper camel case name
 --> src\main.rs:3:12
  |
3 | pub struct rtc_Bytes<'a> {
  |            ^^^^^^^^^ help: convert the identifier to upper camel case: `RtcBytes`
  |
  = note: `#[warn(non_camel_case_types)]` on by default

error: Undefined Behavior: out-of-bounds pointer use: expected a pointer to 10 bytes of memory, but got alloc459 which is only 3 bytes from the end of the allocation
   --> C:\Users\ROG\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\slice\raw.rs:138:9
    |
138 |         &*ptr::slice_from_raw_parts(data, len)
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ out-of-bounds pointer use: expected a pointer to 10 bytes of memory, but got alloc459 which is only 3 bytes from the end of the allocation
    |
    = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
    = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
help: alloc459 was allocated here:
   --> src\main.rs:34:9
    |
34  |     let data = [1, 2, 3];
    |         ^^^^
    = note: BACKTRACE (of the first span):
    = note: inside `std::slice::from_raw_parts::<'_, u8>` at C:\Users\ROG\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\slice\raw.rs:138:9: 138:47
note: inside `rtc_Bytes::<'_>::as_slice`
   --> src\main.rs:24:18
    |
24  |         unsafe { std::slice::from_raw_parts(self.ptr, self.count) }
    |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside `main`
   --> src\main.rs:39:13
    |
39  |     let _ = a.as_slice();
    |             ^^^^^^^^^^^^

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

error: aborting due to 1 previous error; 1 warning emitted

error: process didn't exit successfully: `C:\Users\ROG\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\bin\cargo-miri.exe runner target\miri\x86_64-pc-windows-msvc\debug\lwz.exe` (exit code: 1)
PS E:\Github\lwz> 

Suggest fix
To ensure memory safety, validate both the ptr and count fields before calling std::slice::from_raw_parts.

[lwz23]

and maybe same problem for as_str

ringrtc/src/rust/src/lite/ffi.rs

Line 127 in 9eec8cf

let bytes = unsafe { std::slice::from_raw_parts(self.ptr, self.count) };

I'm currently just emulating the program's implementation logic without the clone codebase to actually verify it, so don't mind if this is a false message.

[jim-signal]

@lwz23 Thank you for the feedback! rtc_Bytes (and friends) is part of the FFI with Swift (see this). The Swift code should guarantee that count will correspond to the allocated buffer. It might help if we add # Safety documentation explaining this, that here we are relying on the Swift code to allocate correctly. We can do this in an upcoming RingRTC release. Would that allay your concerns?

[lwz23]

The suggestion to add # Safety documentation to clarify the reliance on Swift code for correct allocation is certainly valuable, as it helps developers understand the method's preconditions. However, documentation alone cannot enforce these safety constraints, which might leave some room for potential misuse. Since rtc_Bytes::as_slice fundamentally relies on the validity of ptr and count—which are not validated within the method itself—there remains a possibility of undefined behavior, especially if used in contexts outside the intended FFI interaction.

From my point of view. Marking as_slice as unsafe and provide proper # Safety documentation could provide a stronger safeguard that aligns more closely with Rust's safety principles. This would make the method's requirements explicitly clear to its users, ensuring that callers are fully aware of and responsible for meeting the necessary conditions. It would also allow Rust's type system to enforce explicit handling of unsafe operations, significantly reducing the chances of unintended misuse. While adding # Safety documentation is undoubtedly helpful, marking the method as unsafe could better ensure that its preconditions are consistently upheld, particularly in cases where the code might evolve or be used in unanticipated ways.

If the team has confidence that the Swift-side guarantees will always hold and that this method will not be exposed in less controlled contexts, documenting these safety assumptions might be sufficient. I just think marking the function as unsafe would offer a more conservative and future-proof solution, aligning with Rust's philosophy of prioritizing soundness and minimizing potential risks. :)