fix(ffi): make FFI struct fields private to close Drop soundness hole

[bit2swaz]

Which issue does this PR close?

Closes #10429.

Rationale for this change

FFI_ArrowArray, FFI_ArrowSchema, and FFI_ArrowArrayStream had all of their fields pub. Several of those fields carry invariants that the Drop impl and the import path rely on, so safe code could set them to values that trigger undefined behavior with no unsafe block:

• Setting FFI_ArrowSchema::format or name to a pointer that did not come from CString::into_raw causes UB when the release callback frees it with CString::from_raw.
• Setting FFI_ArrowArray::buffers to an invalid pointer causes UB when it is dereferenced with ptr::read_unaligned on import.
• Setting the release fn pointer (or the stream's get_schema / get_next / get_last_error fn pointers) to a bogus function causes UB when it is invoked on drop or import.

An earlier revision of this PR made only release and private_data private and left the data fields pub, on the assumption that the data fields were harmless. That was wrong: as noted in review, the pointer-carrying data fields are UB vectors too. Any write to these fields has to be treated as unsafe.

What changes are included in this PR?

• All fields on FFI_ArrowArray, FFI_ArrowSchema, and FFI_ArrowArrayStream are now private.
• Reads are unchanged: every field already has a typed getter, so consumers keep full read access.
• The wrap-release use case from Expose FFI data structures fields #9771 keeps working through the existing unsafe setters set_release and set_private_data, which swap in a new callback and private data and return the old values so the caller can chain into the original release. No other field needs a write path (the only fields any external consumer writes are release and private_data).
• #[repr(C)] is unchanged, so the C Data Interface layout and ABI are identical.

A separate soundness issue with FFI_ArrowSchema::with_metadata / with_name on foreign schemas was raised in review. It has a different root cause and changes the safe/unsafe surface of those methods, so it will be handled in its own issue and PR rather than bundled here.

Are these changes tested?

Yes. Each struct has a test_wrap_release_callback test covering the swap-and-restore path, and all three pass under Miri with no leak, use-after-free, or double-free. The existing FFI roundtrip tests also pass under Miri. The full workspace builds and tests with --features ffi.

Are there any user-facing changes?

Yes, this is a breaking change. Code that read these fields directly must switch to the getters. Code that constructed these structs with a struct literal, or wrote fields directly, must use the safe constructors (try_from, new, empty) or, for release / private_data, the unsafe setters. The #[repr(C)] layout is unchanged.

[alamb]

I believe you fixed the MIRI error on #10433

Merging up to retrigger and get a clean run

[Jefffrey]

looks like we had them (or at least 2/3) private before

• feat: make FFI structs fields pub #9772
• Expose FFI data structures fields #9771

wonder if @ashdnazg could weigh in here?

[bit2swaz]

right, thanks for bringing that up. i can see that #9772 made these pub on purpose for @ashdnazg's cross-thread use case in #9771 so this PR as is reverts that

i think both goals can coexist tho. the only soundness relevant fields are release and private_data since those are what Drop calls. the plain data fields being pub is harmless. and the thing #9771 actually needs, wrapping release + private_data with your own, is an unsafe operation anyway since youre taking over memory ownership

one direction i think would suit this would be to make the fields private and give back that one capability through a small unsafe API in the same spirit as from_raw. exact shape depends on how @ashdnazg actually uses it

@ashdnazg do you mutate the struct in place (swap release + private_data on an existing one) or do you take it apart and rebuild with your own release? and do you need the originals read back out? that would decide whether this wants an unsafe swap method or an unsafe "from parts" constructor. happy to build whichever fits :)

[ashdnazg]

Thanks for pinging me!

Since I don't want to assume anything about the existing implementation and where the structs came from, I'm accessing the fields directly and I store the original release and private_data in my new private_data:

        let private_data = Box::new(PrivateData {
            original_private_data: schema.private_data,
            original_release: schema.release,
            // other stuff
        });
        schema.private_data = Box::into_raw(private_data) as *mut c_void;
        schema.release = Some(release_schema);

My release function does what it needs with my private data, then restores the original release and private data, and finally calls the original release function:

        let private_data = unsafe { Box::from_raw(schema.private_data as *mut PrivateData) };
        schema.release = private_data.original_release;
        schema.private_data = private_data.original_private_data;
        // call release if not null

So in my opinion getters and unsafe setters are the simplest approach that provides the most flexibility while keeping soundness of safe code.

[ashdnazg]

There is another problem though - the function with_metadata relies on private_data being a SchemaPrivateData, which wouldn't be the case if we got this schema from e.g., Java and didn't construct it locally (which should be a valid use for an FFI object).

with_name currently avoids this by leaking the old name, but if we fixed it not to leak, it would have a similar issue.

We might want to make these functions unsafe and not public.

[bit2swaz]

ah nice, that settles it then. getters + unsafe setters it is. maps right onto your flow too; read both out, overwrite both, restore on release, and you never have to guess where the struct came from. ill add release() / private_data() getters and unsafe set_release() / set_private_data() setters (just plain overwrites since you pull the originals out first and own them from there). same on FFI_ArrowArray

also, good one pointing out the with_metadata. its already live on the safe api and a foreign schema's private_data ismt a SchemaPrivateData so that cast is straight up UB. it is its own thing tho and fixing it changes the safe/unsafe surface of with_metadata + with_name so i dont wanna bundle it in here and hold up the field change.
gonna spin off a separate issue and link back to this thread. that alright with you?

anyways, appreciate you hopping in on this :)

[ashdnazg]

Sounds good, thanks!

[bit2swaz]

alright, made the changes. went with getters + unsafe setters like we talked about on all three structs (FFI_ArrowArray, FFI_ArrowSchema, FFI_ArrowArrayStream) since they all had the same pub release hole. only release + private_data are private now, the rest of the fields stay pub. also updated the PR description to match the new approach

the with_metadata thing ill spin off into its own issue like i mentioned. lmk what you think :)

[ashdnazg]

I don't think you can leave any fields pub.

For instance, changing schema.name or schema.format to anything which isn't a CString will result in UB.

[ashdnazg]

In addition, you can't know what an externally received release function might do, so any alteration of the inside fields must be treated as unsafe.

[alamb]

Thanks for this @bit2swaz

[alamb]

If the goal is to avoid potentially unsafe access do we have to change the other fields to so they aren't pub? For example if I set the buffers field to something invalid that would cause a crash / undefined behavior without requiring an unsafe block 🤔

[bit2swaz]

kept just the per-field unsafe setters and skipped new_from_parts for now. lmk if youd rather have it. only fields anyone writes are release + private_data anyway (thats what @ashdnazg's wrap-release does, read both out, swap both in, restore on release). the setters do that in place on a struct from anywhere. new_from_parts builds a fresh one instead, wrong shape for wrapping an existing foreign struct and its signature has to change every time a field gets added, the churn you flagged the setters dodge.

its additive and non-breaking too so easy to add later if a real from-scratch case shows up. didnt wanna ship unsafe api with no caller yet so yeah

[bit2swaz]

yeah youre both right, ill make all the fields private. i was wrong that the data fields were harmless, my bad on that

@ashdnazg's point on format/name is valid (setting them to anything that didnt come from CString::into_raw blows up in release) and @alamb's buffers example is the same thing on the array side. buffer() does a read_unaligned on self.buffers, so a bad pointer there is UB with no unsafe at the call site. so any write to the inner fields has to be treated as unsafe. going all-fields-private

reads are already covered, all three structs have typed getters for every field, so nothing new needed there

on the write side, ill keep the per-field unsafe setters (set_release / set_private_data). those are the only fields the #9771 wrap-release flow actually writes, and like you said @alamb, setters don't force an api change when a field gets added later. the unsafe fn new_from_parts(...) idea is nice for building a struct from scratch but the safe try_from / new constructors already cover that path and nobodys asked to hand-build these field by field yet, so id rather not add it speculatively. easy to tack on later if a use case shows up. lmk if youd rather have it now tho

the with_metadata thing ill spin off into its own issue later like i mentioned

[bit2swaz]

made those changes. all fields private on all three structs now. you were both right, the data fields are UB vectors too so leaving them pub was a hole. thats my bad on the earlier "harmless" take. reads didnt need anything, every field already has a getter

kept the unsafe setters for release + private_data since thats all the wrap-release flow writes. left a note on @alamb's new_from_parts thread on why i skipped it for now, happy to add it if you want tho

#[repr(C)] is untouched so the layout/abi is identical. lmk if anything else is needed from my side :)