Skip to content

fix(arrow-array): make ArrowArrayStreamReader::try_new unsafe#10419

Closed
bit2swaz wants to merge 1 commit into
apache:mainfrom
bit2swaz:fix/ffi-stream-try-new-unsafe
Closed

fix(arrow-array): make ArrowArrayStreamReader::try_new unsafe#10419
bit2swaz wants to merge 1 commit into
apache:mainfrom
bit2swaz:fix/ffi-stream-try-new-unsafe

Conversation

@bit2swaz

@bit2swaz bit2swaz commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Closes #10253

Rationale for this change

ArrowArrayStreamReader::try_new is a safe fn, but it calls get_stream_schema, which derefs and calls the caller-supplied get_schema fn pointer. every field of FFI_ArrowArrayStream is public, so safe code can construct a stream with garbage callbacks and hit UB without writing a single unsafe block. the release.is_none() check only catches an already-released stream, not a garbage-populated one

both from_raw constructors in the same file are already unsafe for this exact reason. try_new was the one missing it.

What changes are included in this PR?

  • ArrowArrayStreamReader::try_new is now unsafe fn with a # Safety section documenting that the stream's callbacks and private_data must be valid per the C stream interface
  • from_raw body and the two in-module tests wrap their try_new calls in unsafe {}.
  • both try_new call sites in arrow-pyarrow wrap in unsafe {} with a // SAFETY: note
  • dropped the stale #[allow(dead_code)] on try_new

the other option from #10253 (private fields on FFI_ArrowArrayStream) would break the #[repr(C)] ABI that external FFI producers fill field by field, without gaining anything; the callbacks still have to be trusted at call time regardless of visibility.

Are these changes tested?

yes. a compile_fail doctest on try_new verifies that calling it without an unsafe block is a compile error. the existing round-trip and error-import tests cover the happy path and still pass

Are there any user-facing changes?

yes, this is a breaking change. ArrowArrayStreamReader::try_new is now unsafe. callers that obtain a stream from a legitimate FFI source (e.g. from_raw, _export_to_c) just need to wrap the call in unsafe {}

@bit2swaz
bit2swaz force-pushed the fix/ffi-stream-try-new-unsafe branch from 57a5821 to 26daa74 Compare July 23, 2026 19:03
@github-actions github-actions Bot added the arrow Changes to the arrow crate label Jul 23, 2026
@bit2swaz

bit2swaz commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

closing this in favor of a broader fix. @/Jefffrey pointed out that the Drop impl has the same soundness hole and theres no way to slap unsafe on drop. private fields on all three FFI structs (FFI_ArrowSchema, FFI_ArrowArray, FFI_ArrowArrayStream) is the right fix. kills both the try_new and the Drop vector at the root, for all of them at once

tracking in: #10429

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

arrow Changes to the arrow crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

FFI_ArrowArrayStream + ArrowArrayStreamReader::try_new is technically unsound

1 participant