bit2swaz opened a new pull request, #10764: URL: https://github.com/apache/arrow-rs/pull/10764
# Which issue does this PR close? - Closes #10679 - Closes #10286 # 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/0691b2c38add7ce94b8778950ae4f2179cb71391/arrow-schema/src/ffi.rs#L259). 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, which is what #10679 landed on # What changes are included in this PR? - [`with_metadata` is now `unsafe`](https://github.com/bit2swaz/arrow-rs/blob/0691b2c38add7ce94b8778950ae4f2179cb71391/arrow-schema/src/ffi.rs#L204), with a safety doc saying the caller must pass a schema arrow-rs built. this covers the foreign case (#10679). - it now [returns an error on null `private_data`](https://github.com/bit2swaz/arrow-rs/blob/0691b2c38add7ce94b8778950ae4f2179cb71391/arrow-schema/src/ffi.rs#L210) instead of reading it. this covers the empty case (#10286). - 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? added `test_with_metadata_on_empty_schema_errors`: it calls `empty().with_metadata(...)` and checks for `Err`. ran it under miri with the same `-Zmiri-disable-isolation` config CI uses. on the old code miri reports UB, after the fix it passes. the foreign case has no test, you cant call an `unsafe` fn on a foreign schema in a passing test. # Are there any user-facing changes? yes, this is breaking: - `FFI_ArrowSchema::with_metadata` is now `unsafe`, so callers need an `unsafe` block. - on a schema with null `private_data` it returns an error instead of hitting UB. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
