Bug report: Unsoundness in FfiSlice::as_slice

[lwz23]

Description
The current implementation of the FfiSlice struct introduces potential undefined behavior (UB) due to the lack of safety guarantees when public fields are directly passed to an unsafe API (std::slice::from_raw_parts).

swift-bridge/src/lib.rs

Line 48 in 225380e

unsafe { std::slice::from_raw_parts(self.start, self.len) }

pub struct FfiSlice<T> {
    pub start: *const T,
    pub len: usize,
}

impl<T> FfiSlice<T> {
    /// Create an FfiSlice from a slice.
    pub fn from_slice(slice: &[T]) -> Self {
        FfiSlice {
            start: slice.as_ptr(),
            len: slice.len(),
        }
    }

    /// Get a reference to the slice that this FfiSlice points to.
    pub fn as_slice(&self) -> &'static [T] {
        unsafe { std::slice::from_raw_parts(self.start, self.len) }
    }
}

My PoC:
Cargo.toml:

[package]
name = "lwz"
version = "0.1.0"
authors = ["lwz"]
edition = "2018"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

# In your Cargo.toml

[build-dependencies]
swift-bridge-build = "0.1"

[dependencies]
swift-bridge = "0.1"

main.rs:

extern crate swift_bridge;

use swift_bridge::FfiSlice;

fn main() {
    let invalid_ffi_slice = FfiSlice {
        start: 0xdeadbeef as *const u8, 
        len: 10,                       
    };

    let slice = invalid_ffi_slice.as_slice();

    for byte in slice {
        println!("{}", byte);
    }
}

Result:

PS E:\Github\lwz> cargo run
   Compiling lwz v0.1.0 (E:\Github\lwz)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.21s
     Running `target\debug\lwz.exe`
error: process didn't exit successfully: `target\debug\lwz.exe` (exit code: 0xc0000005, STATUS_ACCESS_VIOLATION)

Root Cause
The root cause of the issue is that the FfiSlice struct exposes its fields (start and len) as pub, allowing external code to manipulate them without enforcing the invariants required by std::slice::from_raw_parts. Since as_slice directly uses these fields in an unsafe block, any incorrect or invalid input will propagate into the unsafe API, leading to UB.

Proposed Solution
To address this issue, I propose the following changes:

1. Make Fields Private:
   Change start and len to private fields (pub(crate) or fully private) to prevent arbitrary external modifications.
2. Add Validation:
   Introduce a constructor function that validates the pointer and length before creating an instance of FfiSlice.

for example:

pub struct FfiSlice<T> {
    start: *const T,
    len: usize,
}

impl<T> FfiSlice<T> {
    pub fn new(slice: &[T]) -> Self {
        FfiSlice {
            start: slice.as_ptr(),
            len: slice.len(),
        }
    }

    pub fn as_slice(&self) -> Option<&[T]> {
        if self.start.is_null() || self.len == 0 {
            None
        } else {
            unsafe { Some(std::slice::from_raw_parts(self.start, self.len)) }
        }
    }
}

[lwz23]

Considering this is a Unsound problem and published to crates.io, I suggest reprot it to RustSec.

[chinedufn]

FfiSlice is used in the code generated by our code generators and is not meant to be used directly by users.
It is not part of our publicly supported API. This is why it has a #[doc(hidden)] attribute.

swift-bridge/src/lib.rs

Lines 21 to 26 in e5a0c89

	#[doc(hidden)]
	#[repr(C)]
	pub struct FfiSlice<T> {
	pub start: *const T,
	pub len: usize,
	}

Feel free to re-open this issue if you are able to demonstrate unsoundness using swift-bridge's public APIs.

[lwz23]

If it isn't a public api, maybe mark it as pub(crate) would be more appropriate. Currently, I'm scanning unsoundness problem the pub api of crates.io. this is just my suggestion for reference only.

[chinedufn]

We can't make it as pub(crate) since it is used by code generated by a macro that users call.

[lwz23]

Same problem for RustStr::eq and RustStr::to_str but I guess it also isn't a public api?

swift-bridge/src/std_bridge/string.rs

Line 29 in 225380e

pub struct RustStr {

[lwz23]

FYI: Using #[doc(hidden)] alone is not sufficient to completely eliminate the risks associated with exposing a potentially unsafe API like FfiSlice. While hiding the documentation reduces the likelihood of casual misuse, it does not prevent advanced users or malicious actors from directly accessing and misusing the struct. Since #[doc(hidden)] only affects documentation visibility, the public nature of the struct fields (pub start and pub len) still allows users to directly manipulate these fields, potentially introducing undefined behavior. This is particularly problematic when unsafe blocks are internally relying on the invariants that these fields are supposed to maintain.

A better solution would involve restricting direct access to the struct fields and enforcing stricter encapsulation. The fields start and len should be made private, and all interactions with them should be mediated through safe, well-defined APIs. For instance, providing a constructor that performs validation before creating an instance of FfiSlice ensures that invalid pointers or lengths cannot be passed. Additionally, functions like as_slice should include runtime checks to confirm that the fields are in a valid state before proceeding. For example, the function could return an Option or Result to gracefully handle cases where the pointer is null or the length is zero.

Another robust approach would be to mark FfiSlice as an unsafe struct, explicitly signaling that its use requires careful handling and a deep understanding of the safety guarantees. This would place the responsibility on the developer to ensure that the struct is used correctly, but it would also discourage casual misuse by requiring any operations involving FfiSlice to be enclosed in an unsafe block. This strategy makes it clear that the onus of maintaining safety lies with the user of the struct.

Thank you for your contribution to the maintenance of this project. These are just my personal opinions for security concern :)

[chinedufn]

There is no reason for a user to use the FfiSlice. It is a private API.
We do not need to protect users from our private APIs.

If a user finds the undocumented FfiSlice and decides to use it, despite our clear attempts to prevent them, then I trust that they have very unusual needs.

We cannot prevent an advanced user from creating an FfiSlice.
They can do so using unsafe code.

let null_slice = (std::ptr::null::<u32>(), 100);
let null_slice: FfiSlice<u32> = unsafe { std::mem::transmute(null_slice) };

If a user is willing to use our undocumented APIs then we will trust that they'd be just as willing to construct the type using std::mem::transmute and access the fields using std::ptr::addr_of_mut.

#[doc(hidden)] is enough to protect casual users.
We will not add runtime checks or more complicated APIs (return Option, Result, etc).
This extra code means a small increase in maintenance costs, in exchange for zero benefits to users of our public API.

Instead, we will continue to rely on our test suite to ensure that FfiSlice is used reliably.

---

Keeping the FfiSlice's fields public better communicates that the generated Swift / C code makes use of the FfiSlice's fields.
The FfiSlice is not designed for users.
Neither is the RustStr, or any other type that is marked #[doc(hidden)].

[eric-seppanen]

If there is a pub fn that could be used to cause undefined behavior, then I think it should be marked unsafe.

This would satisfy the need for macros to access it, as well as document the hazards in a way that nobody can claim unsoundness.