This is an automated email from the ASF dual-hosted git repository.
Jefffrey pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow-rs.git
The following commit(s) were added to refs/heads/main by this push:
new c62002b036 fix(ffi): make FFI_ArrowSchema::with_metadata unsafe
(#10764)
c62002b036 is described below
commit c62002b036a0b0c6fd78053c03735f68aedf8ca4
Author: Aditya Mishra <[email protected]>
AuthorDate: Wed Aug 26 12:41:10 2026 +0530
fix(ffi): make FFI_ArrowSchema::with_metadata unsafe (#10764)
# Which issue does this PR close?
- Closes #10679
- Documents #10286 (see the note below)
# Rationale for this change
`with_metadata` reads `self.private_data` as a `SchemaPrivateData` and
writes to it,
[here](https://github.com/bit2swaz/arrow-rs/blob/28a831929ac894a7d0f6b9121c273d38841fc685/arrow-schema/src/ffi.rs#L255).
but `private_data` is only a `SchemaPrivateData` when arrow-rs built the
schema. so on any other schema this is undefined behavior, and you can
trigger it from safe code two ways:
- **foreign schema (#10679):** one imported from pyarrow, C++ or java
owns its own `private_data`, so reading it as ours is UB
- **empty schema (#10286):** `empty()` sets `private_data` to null, so
the read becomes `Box::from_raw(null)`
we cant tell these apart at runtime: the c data interface says
`private_data` is opaque, so theres nothing to check. the fix is to have
the caller promise the schema is ours.
# What changes are included in this PR?
- [`with_metadata` is now
`unsafe`](https://github.com/bit2swaz/arrow-rs/blob/28a831929ac894a7d0f6b9121c273d38841fc685/arrow-schema/src/ffi.rs#L207-L215).
the safety doc says `self` must be a schema this crate produced, and
spells out that a foreign schema or `empty()` is undefined behavior.
- the two callers inside this crate (`TryFrom<&Field>` and
`TryFrom<&Schema>`) now use an `unsafe` block. both build the schema
themselves, so they meet the new rule.
# Are these changes tested?
the existing `test_set_field_metadata` covers the supported path: it
builds a schema with `try_new`, adds metadata (empty, single, and
multi-entry, including re-setting), and checks the round-trip. it passes
under miri with the same `-Zmiri-disable-isolation` config CI uses.
no test drives the foreign or `empty()` case, since both are now
undefined behavior you'd have to opt into with `unsafe`.
# Are there any user-facing changes?
yes, this is breaking: `FFI_ArrowSchema::with_metadata` is now `unsafe`,
so callers need an `unsafe` block and must pass a schema arrow-rs built.
# Note on #10286
this doesn't make `with_metadata` on an empty schema safe. it makes it
documented misuse of an `unsafe` function, which the safety doc now
calls out. if you'd rather keep #10286 open for a separate change, say
so and i'll relink it.
---
arrow-schema/src/ffi.rs | 28 ++++++++++++++++++++--------
1 file changed, 20 insertions(+), 8 deletions(-)
diff --git a/arrow-schema/src/ffi.rs b/arrow-schema/src/ffi.rs
index 3e1fe38082..867d1409b9 100644
--- a/arrow-schema/src/ffi.rs
+++ b/arrow-schema/src/ffi.rs
@@ -203,8 +203,17 @@ impl FFI_ArrowSchema {
Ok(self)
}
- /// Add metadata to the schema
- pub fn with_metadata<I, S>(mut self, metadata: I) -> Result<Self,
ArrowError>
+ /// Add metadata to the schema.
+ ///
+ /// # Safety
+ ///
+ /// `self` must be a schema this crate produced (e.g. via
+ /// [`FFI_ArrowSchema::try_new`] or a `TryFrom`), not one from a foreign
+ /// producer and not [`FFI_ArrowSchema::empty`]. It reinterprets
+ /// `private_data` as our own type, so any other schema is undefined
+ /// behavior. See <https://github.com/apache/arrow-rs/issues/10679> and
+ /// <https://github.com/apache/arrow-rs/issues/10286>.
+ pub unsafe fn with_metadata<I, S>(mut self, metadata: I) -> Result<Self,
ArrowError>
where
I: IntoIterator<Item = (S, S)>,
S: AsRef<str>,
@@ -876,10 +885,11 @@ impl TryFrom<&Field> for FFI_ArrowSchema {
flags |= Flags::DICTIONARY_ORDERED;
}
- FFI_ArrowSchema::try_from(field.data_type())?
+ let schema = FFI_ArrowSchema::try_from(field.data_type())?
.with_name(field.name())?
- .with_flags(flags)?
- .with_metadata(field.metadata())
+ .with_flags(flags)?;
+ // SAFETY: schema was just constructed by this crate.
+ unsafe { schema.with_metadata(field.metadata()) }
}
}
@@ -888,8 +898,9 @@ impl TryFrom<&Schema> for FFI_ArrowSchema {
fn try_from(schema: &Schema) -> Result<Self, ArrowError> {
let dtype = DataType::Struct(schema.fields().clone());
- let c_schema =
FFI_ArrowSchema::try_from(&dtype)?.with_metadata(&schema.metadata)?;
- Ok(c_schema)
+ let c_schema = FFI_ArrowSchema::try_from(&dtype)?;
+ // SAFETY: c_schema was just constructed by this crate.
+ unsafe { c_schema.with_metadata(&schema.metadata) }
}
}
@@ -1083,7 +1094,8 @@ mod tests {
.unwrap();
for metadata in metadata_cases {
- schema = schema.with_metadata(&metadata).unwrap();
+ // SAFETY: schema was constructed by this crate via try_new.
+ schema = unsafe { schema.with_metadata(&metadata) }.unwrap();
let field = Field::try_from(&schema).unwrap();
assert_eq!(field.metadata(), &metadata);
}
