fix(ffi): make FFI_ArrowSchema::with_metadata unsafe

[bit2swaz]

Which issue does this PR close?

• Closes with_metadata is UB on FFI-imported schemas #10679
• Documents Null pointer dereference in FFI_ArrowSchema::with_metadata when used with empty schema #10286 (see the note below)

Rationale for this change

with_metadata reads self.private_data as a SchemaPrivateData and writes to it, here. 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 (with_metadata is UB on FFI-imported schemas #10679): one imported from pyarrow, C++ or java owns its own private_data, so reading it as ours is UB
• empty schema (Null pointer dereference in FFI_ArrowSchema::with_metadata when used with 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. 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.

[alamb]

I thought a bunch more about this

I think the real usecase for the with_metadata is for Arrow-rs itself to add metadata to the schema. I can't really think of a usecase where someone could/should be modifying the metadata on a schema that came from another implementation (e.g py arrow)

Therefore I suggest:

1. mark with_metadata as unsafe (as you have done)
2. Document that the metadata must come from arrow and that if it already had metadata, the old metadata will be leaked
3. Leave the behavior the same

I think there are too many risks to changing the existing behavior and not much upside. Simply getting this function marked unsafe I think is the "safest" approach (no pun intended)

Thank you for all your work on this @bit2swaz and @Jefffrey

[alamb]

e can also document we shouldn't use with_schema with an empty schema

[bit2swaz]

agreed, ill revert that commit then. a schema with format = "" drops fine but its a half built C schema that could get exported to a foreign consumer, not worth it

Leave the behavior the same

the original on an empty schema was Box::from_raw(null), the UB we're fixing so "the same" doesnt make sense to me. did you mean keep the early Err on null private_data or drop the empty handling entirely and let the unsafe contract + docs cover it? your note about documenting "dont use on an empty schema" reads like the second, but i figured id just be sure

lead with self must be a Schema created in arrow-rs

will do

one correction: re setting metadata doesnt leak. the old bytes are in private_data.metadata: Option<Vec<u8>> and get dropped on reassign. the leaker is with_name which overwrites self.name without freeing the old CString:

arrow-rs/arrow-schema/src/ffi.rs

Lines 179 to 188 in dcc801a

	pub fn with_name(mut self, name: &str) -> Result<Self, ArrowError> {
	self.name = CString::new(name)
	.map_err(|e| {
	ArrowError::CDataInterface(format!(
	"Null byte at position {} not allowed in name",
	e.nul_position()
	))
	})?
	.into_raw();
	Ok(self)

note it there instead, or leave for later?

[bit2swaz]

done and pushed. dropped the empty handling, with_metadata is unsafe with the safety doc leading on "must be a schema this crate produced" and calling out foreign + empty() as UB

since it no longer makes the empty case safe, i switched the PR to "documents #10286" instead of "closes"

[Lstarsky0]

The hole is real — try_new sets release and private_data together at ffi.rs:155 and :173, and nothing else in the type ties them — but I don't think "there's nothing to check" holds. The release callback is the discriminator. Any conforming producer installs its own, arrow-rs installs release_schema, and empty() leaves it None, so self.release() — already public at ffi.rs:286 — answers exactly the question with_metadata needs, via ptr::fn_addr_eq against release_schema.

It fails in the safe direction. A schema this crate did not build cannot carry that address, so the worst case is a spurious CDataInterface error rather than UB. That would let with_metadata stay a safe fn returning Err, which also sidesteps the part I'd want spelled out below.

The one case it gets wrong is the wrapping pattern the comment at ffi.rs:297 describes: a consumer that saves the old callback and installs its own still owns arrow-rs private_data but no longer matches, so it would get an error where today it works. Whether that is acceptable is a judgement call, but it is a much smaller cost than the current one and it is worth weighing explicitly rather than ruling the check out.

Two smaller things.

The title says "guard null private_data" and the diff has no guard — the # Safety clause forbids empty(), which is a contract, not a check. The null case is the one that is trivially checkable even without the release comparison, so it reads like something got dropped.

Turning a safe pub fn into unsafe fn breaks every downstream caller at compile time. That may well be the right call, but the description doesn't say which release this is aimed at, and the safe-fn-plus-error shape above would not need one.

[bit2swaz]

the release-callback check was option B on #10679 and i did try it. ptr::fn_addr_eq can return false for the same function so it rejects schemas arrow-rs actually built, it broke 6 ffi tests under miri (which CI runs). thats a spurious Err on a real schema, which imo is the wrong way to fail

your wrapping case is a second false reject: a schema that wrapped its release via set_release still owns our private_data but won't match release_schema:

arrow-rs/arrow-schema/src/ffi.rs

Line 315 in 06edc8f

/// Lets a consumer wrap release: save the old callback, install its own, and

thats why @alamb, @Jefffrey and i landed on unsafe (full thread on #10679)

on the smaller two: title is stale from an earlier null-guard version, ill fix it. and yeah its breaking, goes in the next major