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 each have a pub release field holding a producer-supplied function pointer, and their Drop impl calls it. Because the field is public, safe code can build one of these structs with a bogus release and dropping it runs that pointer as a function. That is undefined behavior reachable from safe code.

private_data has the same problem: the release callback owns it, so a mismatched pointer is UB on drop too.

The fix has to keep the cross-thread wrap-release use case from #9771 working, where a consumer swaps in its own release + private_data and chains back to the original.

What changes are included in this PR?

• Make only release and private_data private on all three structs. The other fields stay pub, since they can't cause UB and are part of the existing public API.
• Add safe getters release() and private_data().
• Add unsafe setters set_release() and set_private_data() that swap the value and return the old one without freeing it, so a consumer can save the originals and chain back on drop. This is the Expose FFI data structures fields #9771 pattern, now behind unsafe where it belongs.
• #[repr(C)] is unchanged, so the FFI ABI is the same.

Are these changes tested?

Yes. Each struct gets a test that installs a wrapping release, drops the struct, and checks the wrapper ran and chained to the original. The existing FFI round-trip tests still pass, and the new tests pass under Miri with no leaks or use-after-free.

Are there any user-facing changes?

Yes. release and private_data are no longer public fields on the three structs. Code that read or wrote them directly should use the new getters and unsafe setters instead. Everything else 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]

Another potential API could be

unsafe fn new_from_parts(...) -> Self {
...
}

To allow someone to set the fields as they want, but acknoledging it is not safe. The benefit of the API in this PR is that we won't have to modify the existing API if a new field is added to FFI_Stream in the future

[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]

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