[I] soundness: FFI structs have public fields, making Drop unsound in safe code [arrow-rs]

["bit2swaz (via GitHub)" <gi...@apache.org>]

bit2swaz opened a new issue, #10429:
URL: https://github.com/apache/arrow-rs/issues/10429

   ### 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:
   
   ```rust
   fn drop(&mut self) {
       match self.release {
           None => (),
           Some(release) => unsafe { release(self) },
       };
   }
   ```
   
   - [`FFI_ArrowArray` drop (arrow-data/src/ffi.rs:67-74)](https://github.com/apache/arrow-rs/blob/f7dfcd25aabeb01641fe4b6c35ab964fdf0b24aa/arrow-data/src/ffi.rs#L67-L74)
   - [`FFI_ArrowSchema` drop (arrow-schema/src/ffi.rs:418-425)](https://github.com/apache/arrow-rs/blob/f7dfcd25aabeb01641fe4b6c35ab964fdf0b24aa/arrow-schema/src/ffi.rs#L418-L425)
   - [`FFI_ArrowArrayStream` drop (arrow-array/src/ffi_stream.rs:164-171)](https://github.com/apache/arrow-rs/blob/f7dfcd25aabeb01641fe4b6c35ab964fdf0b24aa/arrow-array/src/ffi_stream.rs#L164-L171)
   
   `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
   
   ```rust
   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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] soundness: FFI structs have public fields, making Drop unsound in safe code [arrow-rs]

["bit2swaz (via GitHub)" <gi...@apache.org>]

bit2swaz commented on issue #10429:
URL: https://github.com/apache/arrow-rs/issues/10429#issuecomment-5269867620

   take


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] soundness: FFI structs have public fields, making Drop unsound in safe code [arrow-rs]

["Jefffrey (via GitHub)" <gi...@apache.org>]

Jefffrey closed issue #10429: soundness: FFI structs have public fields, making Drop unsound in safe code
URL: https://github.com/apache/arrow-rs/issues/10429


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org