Skip to content

soundness: FFI structs have public fields, making Drop unsound in safe code #10429

Description

@bit2swaz

Describe the bug

FFI_ArrowArray, FFI_ArrowSchema, and FFI_ArrowArrayStream all have fully public fields, including the release fn pointer that each struct's Drop impl calls. Safe Rust code can construct any of them with a garbage release callback and trigger UB on drop, no unsafe block required.

All three Drop impls are the same shape:

fn drop(&mut self) {
    match self.release {
        None => (),
        Some(release) => unsafe { release(self) },
    };
}

Drop::drop can't be marked unsafe fn, so there's no way to put the safety obligation on the caller at the drop site. The only fix that closes this is making the fields private, so garbage instances can't be constructed from safe code in the first place.

This came up in #10253 for ArrowArrayStreamReader::try_new specifically. @Jefffrey noted that Drop has the same problem and that FFI_ArrowArray is prone to it too, so this needs a broader fix across all three structs.

To Reproduce

use arrow_data::ffi::FFI_ArrowArray;

// defining an extern "C" fn is not an unsafe block, just an ABI declaration
extern "C" fn bad_release(_: *mut FFI_ArrowArray) {
    panic!("garbage callback");
}

fn main() {
    // no `unsafe` block anywhere in this function
    let _array = FFI_ArrowArray {
        length: 0,
        null_count: 0,
        offset: 0,
        n_buffers: 0,
        n_children: 0,
        buffers: std::ptr::null_mut(),
        children: std::ptr::null_mut(),
        dictionary: std::ptr::null_mut(),
        release: Some(bad_release),
        private_data: std::ptr::null_mut(),
    };
    // _array drops here -> calls bad_release -> UB
}

Same applies to FFI_ArrowSchema and FFI_ArrowArrayStream

Expected behavior

Private fields. The only ways to obtain one of these structs would then be the safe constructors (new, try_new, empty) which set valid callbacks, or unsafe fn from_raw which already puts the safety obligation on the caller. As a side effect, ArrowArrayStreamReader::try_new can stay safe fn too, since you can no longer construct a garbage stream in safe Rust

#[repr(C)] layout is unaffected by field visibility. External C producers fill these structs as raw memory anyway and hand a pointer to Rust via from_raw, which is already unsafe. Private Rust fields don't break any legitimate C interop

Additional context

Raised in #10253. Draft PR #10419 addressed only try_new on FFI_ArrowArrayStream and is now closed in favor of this fix

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions