fix(ffi): make FFI_ArrowSchema::with_metadata unsafe and guard null private_data - #10764
Open
bit2swaz wants to merge 2 commits into
Open
fix(ffi): make FFI_ArrowSchema::with_metadata unsafe and guard null private_data#10764bit2swaz wants to merge 2 commits into
bit2swaz wants to merge 2 commits into
Conversation
Jefffrey
reviewed
Aug 20, 2026
| S: AsRef<str>, | ||
| { | ||
| // empty() leaves private_data null; error instead of deref-ing it (#10286). | ||
| if self.private_data.is_null() { |
Contributor
There was a problem hiding this comment.
im on similar mind to this comment from @alamb
Perhaps it would be more future proof to just set
private_datafor the result of FFI_ArrowSchema::empty()?It seems like it is not unreasonable to set metadata on an empty schema, and I worry that other existing (or newly added) code paths will assume private_data is non null
that or set it here if we're adding metadata to an empty schema 🤔
though we can check the other ffi structs to see how they deal with this
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
FFI_ArrowSchema::with_metadatawhen used with empty schema #10286Rationale for this change
with_metadatareadsself.private_dataas aSchemaPrivateDataand writes to it, here. butprivate_datais only aSchemaPrivateDatawhen arrow-rs built the schema. so on any other schema this is undefined behavior, and you can trigger it from safe code two ways:private_data, so reading it as ours is UBFFI_ArrowSchema::with_metadatawhen used with empty schema #10286):empty()setsprivate_datato null, so the read becomesBox::from_raw(null)we cant tell these apart at runtime: the c data interface says
private_datais opaque, so theres nothing to check. the fix is to have the caller promise the schema is ours, which is what #10679 landed onWhat changes are included in this PR?
with_metadatais nowunsafe, with a safety doc saying the caller must pass a schema arrow-rs built. this covers the foreign case (with_metadata is UB on FFI-imported schemas #10679).private_datainstead of reading it. this covers the empty case (Null pointer dereference inFFI_ArrowSchema::with_metadatawhen used with empty schema #10286).TryFrom<&Field>andTryFrom<&Schema>) now use anunsafeblock. both build the schema themselves, so they meet the new rule.Are these changes tested?
added
test_with_metadata_on_empty_schema_errors: it callsempty().with_metadata(...)and checks forErr.ran it under miri with the same
-Zmiri-disable-isolationconfig CI uses. on the old code miri reports UB, after the fix it passes.the foreign case has no test, you cant call an
unsafefn on a foreign schema in a passing test.Are there any user-facing changes?
yes, this is breaking:
FFI_ArrowSchema::with_metadatais nowunsafe, so callers need anunsafeblock.private_datait returns an error instead of hitting UB.