You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "bit2swaz (via GitHub)" <gi...@apache.org> on 2026/08/13 17:31:29 UTC
[I] with_metadata is UB on FFI-imported schemas [arrow-rs]
bit2swaz opened a new issue, #10679:
URL: https://github.com/apache/arrow-rs/issues/10679
### Describe the bug
`FFI_ArrowSchema::with_metadata` casts `self.private_data` to `*mut SchemaPrivateData` and writes through it, without checking that arrow-rs built the schema: [ffi.rs#L245](https://github.com/apache/arrow-rs/blob/c87638f4a977c289395725bb329cf33fb4e8fc44/arrow-schema/src/ffi.rs#L245)
for a schema imported over FFI (from Java, C++, pyarrow, etc.) `private_data` is the producer's own struct, not a `SchemaPrivateData`. so that cast reinterprets foreign bytes as a Rust type and writes to them, which is UB. `with_metadata` is safe public api, so this is reachable from safe code.
[with_name](https://github.com/apache/arrow-rs/blob/c87638f4a977c289395725bb329cf33fb4e8fc44/arrow-schema/src/ffi.rs#L178) is fine today since it only overwrites `self.name`, but it leaks the old name pointer to avoid touching `private_data`. fixing that leak would hit the same problem.
came out of the #10431 review thread, credits to @ashdnazg for spotting it
### To Reproduce
import any schema over the C Data Interface (so `private_data` isnt a `SchemaPrivateData`), then call `.with_metadata(...)` on it
### Expected behavior
calling `with_metadata` on a foreign schema shouldnt be UB. two ways to close it:
- A: make `with_metadata` (and `with_name`) `unsafe`. it's honest about the precondition and costs nothing at runtime, but it breaks the safe api.
- B: detect foreign schemas and return `Err` instead of casting, keeping the methods safe. a local schema always has `release == Some(release_schema)` (set in `try_new`), and a foreign one carries the producer's release, so that comparison tells them apart with no new field or abi change.
i lean B since it keeps the API safe and non breaking
### Additional context
follow-up to #10431. happy to put up the PR once the direction is picked :)
--
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: github-unsubscribe@arrow.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] with_metadata is UB on FFI-imported schemas [arrow-rs]
Posted by "bit2swaz (via GitHub)" <gi...@apache.org>.
bit2swaz commented on issue #10679:
URL: https://github.com/apache/arrow-rs/issues/10679#issuecomment-5334574651
> some sort of reproducer (ad hoc) that shows this is actually a bug
import a schema from pyarrow but keep it as an `FFI_ArrowSchema` instead of converting to a `Schema`, then call `with_metadata`:
```rust
let mut c_schema = FFI_ArrowSchema::empty();
py_schema.call_method1("_export_to_c", (&raw mut c_schema as usize,))?;
// c_schema.private_data now belongs to pyarrow, not a SchemaPrivateData
let c_schema = c_schema.with_metadata([("k", "v")])?; // safe api, no unsafe
```
`with_metadata` runs the [cast at ffi.rs#L246](https://github.com/apache/arrow-rs/blob/cb20cc4bfd0fd29bf31d4c4c5c783174d9fcbff6/arrow-schema/src/ffi.rs#L246), treating pyarrow's `private_data` as a `SchemaPrivateData` and writing thru it. thats UB. `_export_to_c` is stable pyarrow api. arrow-rs converts an imported schema to a native `Schema` right away so this only bites a downstream user who keeps the raw `FFI_ArrowSchema` around
one snag on B tho: the `release == release_schema` check doesnt hold up. `std::ptr::fn_addr_eq` can return false for the same function, and it makes 6 ffi tests fail under miri (which CI runs) so it can wrongly reject a schema arrow-rs built
B needs a different detector then, like a magic tag as the first field of `SchemaPrivateData`. do you want me to sort that out, or just make the methods `unsafe` (A)?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] with_metadata is UB on FFI-imported schemas [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on issue #10679:
URL: https://github.com/apache/arrow-rs/issues/10679#issuecomment-5336719664
correct me if im wrong, but wouldnt that mean any method which dereferences an inner field would be unsound if safe construction that fills with garbage is possible?
for example here:
https://github.com/apache/arrow-rs/blob/505afcbfd2c27897f747916e782d41e1593b7007/arrow-schema/src/ffi.rs#L403-L465
what if `self.metadata` is non-null but doesnt point to enough data? i.e. we read beyond the allocation its supposed to point to
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] with_metadata is UB on FFI-imported schemas [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on issue #10679:
URL: https://github.com/apache/arrow-rs/issues/10679#issuecomment-5343368787
yeah we can do that since the other PR for that issue looks to be stale
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] with_metadata is UB on FFI-imported schemas [arrow-rs]
Posted by "bit2swaz (via GitHub)" <gi...@apache.org>.
bit2swaz commented on issue #10679:
URL: https://github.com/apache/arrow-rs/issues/10679#issuecomment-5342852424
> it does seem a bit clunky, and has a downside in that it technically keeps the old metadata around
fair, thats a real downside of C. A is cleaner and im happy to go with it, the 60.0.0 breaking window makes it a good time.
one thing to watch with A tho is that making it `unsafe` fixes the foreign-schema case but it doesnt close #10286 on its own https://github.com/apache/arrow-rs/blob/505afcbfd2c27897f747916e782d41e1593b7007/arrow-schema/src/ffi.rs#L280-L281 `empty()` leaves `private_data` null, and the `Box::from_raw` at L246
https://github.com/apache/arrow-rs/blob/505afcbfd2c27897f747916e782d41e1593b7007/arrow-schema/src/ffi.rs#L246 runs unconditionally, so `empty().with_metadata(...)` is still UB even for a caller doing everything right on an arrow-rs schema. no safety contract can rule that out since `empty()` is our own constructor
so A needs a null guard on `private_data` too, then it closes both. want me to fold #10286 into the same PR, or should i keep it separate?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] with_metadata is UB on FFI-imported schemas [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #10679:
URL: https://github.com/apache/arrow-rs/issues/10679#issuecomment-5334189322
B sounds good to me
Cna you also please provide an test case that shows that this is actually a problem? That the schema field can be set from C++ or Java for example?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] with_metadata is UB on FFI-imported schemas [arrow-rs]
Posted by "bit2swaz (via GitHub)" <gi...@apache.org>.
bit2swaz commented on issue #10679:
URL: https://github.com/apache/arrow-rs/issues/10679#issuecomment-5338296121
the difference is what the spec says about each field.
[`metadata`](https://arrow.apache.org/docs/format/CDataInterface.html#c.ArrowSchema.metadata) has a defined byte layout the producer must write, so `metadata()`https://github.com/apache/arrow-rs/blob/505afcbfd2c27897f747916e782d41e1593b7007/arrow-schema/src/ffi.rs#L403-L465 reading it is honoring the contract. it only breaks if the producer already broke the spec, like any ffi pointer
[`private_data`](https://arrow.apache.org/docs/format/CDataInterface.html#c.ArrowSchema.private_data) is spec-defined as opaque to the consumer. `with_metadata` casts it to `SchemaPrivateData` and writes thru it which only holds for schemas arrow-rs built. thats UB on a well formed foreign schema with no bad producer needed
bounding the length reads in `metadata()` is still worth doing, just separate from this
do you see `metadata()` as the same bucket or does that line hold?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] with_metadata is UB on FFI-imported schemas [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on issue #10679:
URL: https://github.com/apache/arrow-rs/issues/10679#issuecomment-5342333968
it does seem a bit clunky, and has a downside in that it technically keeps the old metadata around if we were replacing it. im of the mind to go with option A since we already have some other breaking changes to the ffi structs in the coming 60.0.0 release 🤔
though the PR that adds this `with_metadata` is pretty old so more likely for it to be used downstream
- https://github.com/apache/arrow-rs/pull/3944
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] with_metadata is UB on FFI-imported schemas [arrow-rs]
Posted by "bit2swaz (via GitHub)" <gi...@apache.org>.
bit2swaz commented on issue #10679:
URL: https://github.com/apache/arrow-rs/issues/10679#issuecomment-5336394451
> is this saying we can construct a `FFI_ArrowSchema` that has a `private_data` which violates safety guarantees without using any unsafe API?
yes. `with_metadata` is a safe method and it does `Box::from_raw(self.private_data.cast::<SchemaPrivateData>())`
https://github.com/apache/arrow-rs/blob/505afcbfd2c27897f747916e782d41e1593b7007/arrow-schema/src/ffi.rs#L246
if that `private_data` came from anywhere other than arrow-rs (pyarrow, c++, java via the c data interface), it isnt a `SchemaPrivateData`, so reinterpreting it as one and writing thru it is UB. no unsafe needed on the caller side, `_export_to_c` fills the struct for you
> https://github.com/apache/arrow-rs/issues/10286
same method, same root cause. `empty()` leaves `private_data` null, so `with_metadata` hits `Box::from_raw(null)` there instead of a foreign pointer. the fix here (dont touch the incoming `private_data`, put the new metadata in a fresh box) covers #10286 too
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] with_metadata is UB on FFI-imported schemas [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #10679:
URL: https://github.com/apache/arrow-rs/issues/10679#issuecomment-5334435413
I don't think we need to add a regression test in the PR / repro for this (your idea of "foreign-shaped FFI_ArrowSchema" sounds good to me for a test)
However, I would like to see some sort of reproducer (ad hoc) that shows this is actually a bug that can be hit in real datasets
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] with_metadata is UB on FFI-imported schemas [arrow-rs]
Posted by "ashdnazg (via GitHub)" <gi...@apache.org>.
ashdnazg commented on issue #10679:
URL: https://github.com/apache/arrow-rs/issues/10679#issuecomment-5334739337
IMO a magic tag doesn't sound robust. The private data could be anything.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] with_metadata is UB on FFI-imported schemas [arrow-rs]
Posted by "bit2swaz (via GitHub)" <gi...@apache.org>.
bit2swaz commented on issue #10679:
URL: https://github.com/apache/arrow-rs/issues/10679#issuecomment-5284210084
take
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] with_metadata is UB on FFI-imported schemas [arrow-rs]
Posted by "bit2swaz (via GitHub)" <gi...@apache.org>.
bit2swaz commented on issue #10679:
URL: https://github.com/apache/arrow-rs/issues/10679#issuecomment-5340988205
yep, thats basically it. we never touch the incoming `private_data`, we just box a wrapper around it:
```rust
struct WithMetadata {
metadata: Vec<u8>, // our new serialized metadata, kept alive
inner_release: Option<unsafe extern "C" fn(*mut FFI_ArrowSchema)>,
inner_private_data: *mut c_void, // pyarrow's, untouched
}
```
then point `self.metadata` at the new bytes, set `self.private_data` to the boxed wrapper, and install our own `release`. on drop, our `release` frees the wrapper (our metadata) then calls `inner_release` on a temp schema carrying `inner_private_data`, so pyarrow still cleans up its own state
works the same for an arrow-rs schema: the inner release is just `release_schema` and it chains fine. `set_release`/`set_private_data` already exist for exactly this. it also fixes #10286, since a null inner is just a no-op chain
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] with_metadata is UB on FFI-imported schemas [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on issue #10679:
URL: https://github.com/apache/arrow-rs/issues/10679#issuecomment-5336228631
i might need some help with understanding something: is this saying we can construct a `FFI_ArrowSchema` that has a `private_data` which violates safety guarantees without using any unsafe API?
also a related issue (though not exactly the same):
- https://github.com/apache/arrow-rs/issues/10286
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] with_metadata is UB on FFI-imported schemas [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey closed issue #10679: with_metadata is UB on FFI-imported schemas
URL: https://github.com/apache/arrow-rs/issues/10679
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] with_metadata is UB on FFI-imported schemas [arrow-rs]
Posted by "bit2swaz (via GitHub)" <gi...@apache.org>.
bit2swaz commented on issue #10679:
URL: https://github.com/apache/arrow-rs/issues/10679#issuecomment-5334169556
@alamb @Jefffrey friendly ping on this one. id like to fix it but the two options change the API differently so i figured its best to ask before proceeding
a lil recap of the fork:
> A: make `with_metadata` (and `with_name`) `unsafe`
> B: detect foreign schemas and return `Err` instead of casting
one thing thats changed since i filed this: both methods already return `Result` on current main ([with_metadata at ffi.rs#L198](https://github.com/apache/arrow-rs/blob/cb20cc4bfd0fd29bf31d4c4c5c783174d9fcbff6/arrow-schema/src/ffi.rs#L198)). so option B adds no new signature and no breakage, it just returns an `Err` on the case that is currently UB, the [cast at ffi.rs#L246](https://github.com/apache/arrow-rs/blob/cb20cc4bfd0fd29bf31d4c4c5c783174d9fcbff6/arrow-schema/src/ffi.rs#L246).
the check for B is cheap: a schema arrow-rs built always has `release == Some(release_schema)` (set in `try_new`, [ffi.rs#L155](https://github.com/apache/arrow-rs/blob/cb20cc4bfd0fd29bf31d4c4c5c783174d9fcbff6/arrow-schema/src/ffi.rs#L155)), and a foreign one carries the producer's release, so that tells them apart with no new field
and thats why im biased towards B. whats your guys' take on this tho?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] with_metadata is UB on FFI-imported schemas [arrow-rs]
Posted by "bit2swaz (via GitHub)" <gi...@apache.org>.
bit2swaz commented on issue #10679:
URL: https://github.com/apache/arrow-rs/issues/10679#issuecomment-5334860205
> a magic tag doesn't sound robust. The private data could be anything.
agreed, and that kills detection in general: `private_data` is opaque and no other field only arrow-rs would set. so option B is out
that leaves three fixes that dont need detection:
- A: make `with_metadata` `unsafe`. simplest, but breaking
- C: put the new metadata in a fresh box and chain the existing release on drop (the #9771 wrap pattern). non breaking, works for foreign and local schemas
- D: read the fields via the safe getters and rebuild a fresh schema with the extra metadata. non breaking too, but it rebuilds the whole child/dict tree and drops metadata key order, so id rank it last
(only `with_metadata` is affected, `with_name` never touches `private_data`)
i lean C. which do you prefer?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] with_metadata is UB on FFI-imported schemas [arrow-rs]
Posted by "bit2swaz (via GitHub)" <gi...@apache.org>.
bit2swaz commented on issue #10679:
URL: https://github.com/apache/arrow-rs/issues/10679#issuecomment-5334351316
> B sounds good to me
great, ill go with B then
> Can you also please provide an test case that shows that this is actually a problem?
i can build a foreign-shaped `FFI_ArrowSchema` in the test without needing an actual cpp or java producer: set `private_data` to a box that isnt a `SchemaPrivateData` with its own release callback, same shape the c data interface hands us. calling `with_metadata` on that today hits the [cast at ffi.rs#L246](https://github.com/apache/arrow-rs/blob/cb20cc4bfd0fd29bf31d4c4c5c783174d9fcbff6/arrow-schema/src/ffi.rs#L246) and reinterprets those bytes as our type. the test will show that returns `Err` after the fix instead of being UB
ill also add a pyarrow roundtrip in arrow-pyarrow-integration-testing so theres a real cross-language case on top of the unit test. if thats alright, then ill get a PR otw :)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] with_metadata is UB on FFI-imported schemas [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on issue #10679:
URL: https://github.com/apache/arrow-rs/issues/10679#issuecomment-5338804416
i think i understand the issue now, thanks for explaining. could you elaborate on point C a bit more? is it saying we replace `release` & `private_data` (which in this scenario contains pyarrow specific data) with our own wrapper struct that holds both our new metadata + release and the old metadata + release?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org