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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
