bit2swaz opened a new pull request, #10431:
URL: https://github.com/apache/arrow-rs/pull/10431
# Which issue does this PR close?
Closes #10429.
# Rationale for this change
`FFI_ArrowArray`, `FFI_ArrowSchema`, and `FFI_ArrowArrayStream` all have
public fields, including the `release` fn pointer that each struct's `Drop`
impl calls. Safe Rust can construct any of them with a garbage `release`
callback and trigger UB on drop, with no `unsafe` block anywhere in sight.
`Drop::drop` can't be `unsafe fn`, so there's no way to put the safety
obligation on the caller at the drop site. The only fix is making the fields
private, so garbage instances can't be constructed from safe code in the first
place.
This came up in #10253. @Jefffrey pointed out that the Drop impl has the
same problem and that `FFI_ArrowArray` is prone to it too, so the fix needs to
cover all three structs.
# What changes are included in this PR?
Stripped `pub` from all fields on `FFI_ArrowArray` (arrow-data),
`FFI_ArrowSchema` (arrow-schema), and `FFI_ArrowArrayStream` (arrow-array). No
new logic, no new public API.
One test in `arrow-data/src/ffi.rs` was accessing `n_buffers` and
`private_data` directly as a child module. Replaced with
`ffi_array.num_buffers()`, which is the existing public getter for the same
information.
`#[repr(C)]` layout is determined by field types and order, not visibility,
so the ABI is unchanged. External C producers fill these structs as raw memory
and hand a pointer to Rust via `from_raw`, which is already `unsafe`.
# Are these changes tested?
Yes. All existing tests pass. The soundness hole is now a compile error:
constructing any of the three structs with struct literal syntax from outside
the defining module fails with `fields ... are private`.
# Are there any user-facing changes?
Breaking change for any downstream Rust code constructing these structs with
struct literal syntax (e.g. `FFI_ArrowArray { release: Some(f), ... }`). That
pattern was only valid in safe code for the struct fields that had no raw
pointer invariants, but it was the exact vector for the soundness hole, so
closing it is the point. Code using the public constructors (`new`, `empty`,
`from_raw`) is unaffected.
--
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]
