Manishearth opened a new issue, #10286: URL: https://github.com/apache/arrow-rs/issues/10286
> [!NOTE] > This finding was identified during an agentic unsafe Rust code review performed by Gemini AI, followed by human review and verification. ### The Issue `FFI_ArrowSchema::empty()` initializes `private_data` to null. https://github.com/apache/arrow-rs/blob/1583c8e6563dca80f1380c93089a7c1052776943/arrow-schema/src/ffi.rs#L268 `with_metadata` calls `Box::from_raw` on this value (unconditionally, I don't see an early return here) https://github.com/apache/arrow-rs/blob/1583c8e6563dca80f1380c93089a7c1052776943/arrow-schema/src/ffi.rs#L232-L236 <details><summary>Minimal Reproduction</summary> ```rust use arrow_schema::ffi::FFI_ArrowSchema; fn main() { let schema = FFI_ArrowSchema::empty(); // calling with_metadata on empty schema invokes Box::from_raw(null) let _ = schema.with_metadata([("key", "value")]); } ``` ```text error: Undefined Behavior: constructing invalid value of type std::ptr::NonNull<arrow_schema::ffi::SchemaPrivateData>: at .pointer, encountered 0, but expected something greater or equal to 1 --> /usr/local/google/home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/unique.rs:88:36 | 88 | ...er: NonNull::new_unchecked(ptr), _marker: PhantomData } } | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ Undefined Behavior occurred here | = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information = note: stack backtrace: 0: std::ptr::Unique::<arrow_schema::ffi::SchemaPrivateData>::new_unchecked at /usr/local/google/home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/unique.rs:88:36: 88:63 1: std::boxed::Box::<arrow_schema::ffi::SchemaPrivateData>::from_raw_in at /usr/local/google/home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/boxed.rs:1560:22: 1560:48 2: std::boxed::Box::<arrow_schema::ffi::SchemaPrivateData>::from_raw at /usr/local/google/home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/boxed.rs:1323:18: 1323:48 3: arrow_schema::ffi::FFI_ArrowSchema::with_metadata::<[(&str, &str); 1], &str> at /usr/local/google/home/manishearth/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/arrow-schema-57.3.1/src/ffi.rs:224:36: 224:94 4: main at src/bin/repro1.rs:6:13: 6:53 ``` </details> <details><summary>Suggested Fix</summary> Apply necessary runtime bounds checks or formalize the `unsafe fn` safety contract pre-conditions. </details> > [!NOTE] > The full audit report below also contains additional minor findings (such as missing safety comments or undocumented FFI assumptions) that are probably worth fixing as well but not the primary goal of this issue. The audit report has not been human-reviewed, it may contain misleading claims. <details><summary>Full Gemini Codebase Audit Report Appendix</summary> # Unsafe Rust Review: `arrow_schema` (`v57`) ## Overall Safety Assessment The `arrow_schema` crate contains a small amount of unsafe Rust code, entirely located in `src/ffi.rs` to support the Arrow C Data Interface. The density of unsafe code is low, but the risk is high because several safe public APIs wrap unsafe operations without adequate validation, leading to critical soundness bugs. Furthermore, safety documentation is almost entirely absent: there are virtually no `# Safety` sections for unsafe functions, and the few `// SAFETY:` comments are informal, lacking the logical rigor required by the review guidelines. ## Critical Findings ### Undefined Behavior / Memory Corruption in `FFI_ArrowSchema::with_metadata` 🔴 ⚠️ - **Priority**: 🔴 High - **Threat Vector**: ⚠️ Accidental Misuse - **Bug Type**: Null Pointer Dereference The public, safe builder method `FFI_ArrowSchema::with_metadata` is unsound and can be used to trigger immediate undefined behavior in safe Rust code. #### Explanation The method is defined as: ```rust pub fn with_metadata<I, S>(mut self, metadata: I) -> Result<Self, ArrowError> where I: IntoIterator<Item = (S, S)>, S: AsRef<str>, { ... unsafe { let mut private_data = Box::from_raw(self.private_data as *mut SchemaPrivateData); private_data.metadata = new_metadata; self.private_data = Box::into_raw(private_data) as *mut c_void; } Ok(self) } ``` If this method is called on a schema constructed via `FFI_ArrowSchema::empty()`, `self.private_data` is a null pointer. The code passes this null pointer directly to `Box::from_raw`, which violates the standard library safety precondition (the pointer must be non-null and allocated by the global allocator with the appropriate layout). This triggers undefined behavior (manifesting as a abort/panic in debug mode with standard library checks). Furthermore, if the schema was imported from C via `from_raw`, `self.private_data` contains an opaque pointer provided by the foreign C producer. Calling `with_metadata` on such a schema will cast this opaque pointer to a `SchemaPrivateData` and attempt to drop it or reallocate it, causing type confusion and memory corruption. #### Reproduction ```rust #[test] fn test_empty_with_metadata_ub() { let schema = FFI_ArrowSchema::empty(); // This will call Box::from_raw(null) inside `with_metadata`, triggering UB. let _ = schema.with_metadata([("key", "value")]); } ``` #### Recommended Fix Make the method validate that the schema was constructed locally and is initialized: ```rust if self.private_data.is_null() { return Err(ArrowError::CDataInterface( "Cannot set metadata on an empty or uninitialized schema".to_string(), )); } if self.release != Some(release_schema) { return Err(ArrowError::CDataInterface( "Cannot set metadata on schema with external release callback".to_string(), )); } unsafe { let mut private_data = Box::from_raw(self.private_data as *mut SchemaPrivateData); private_data.metadata = new_metadata; self.private_data = Box::into_raw(private_data) as *mut c_void; } ``` --- ## Fishy Findings None. --- ## Missing Safety Comments The following unsafe blocks and functions lack safety comments or have comments that are not structured as proofs: ### 1. `FFI_ArrowSchema::from_raw` 🟡 - **Priority**: 🟡 Low - **Bug Type**: Missing Safety Documentation ```rust pub unsafe fn from_raw(schema: *mut FFI_ArrowSchema) -> Self { unsafe { std::ptr::replace(schema, Self::empty()) } } ``` * **Missing safety documentation:** The function has `# Safety` documentation for the caller, but the unsafe block inside `from_raw` lacks a `// SAFETY:` comment explaining why `std::ptr::replace` is sound given those preconditions. * **Proposed safety comment:** ```rust // SAFETY: // - By caller precondition, `schema` is valid for reads and writes and properly aligned. // - Replacing the value with `Self::empty()` is safe as both types match. // - By replacing the schema, we set its release callback to `None` in the source location, // safely moving ownership of the resource to the returned `Self`. ``` ### 2. `FFI_ArrowSchema::format` & `name` 🟡 - **Priority**: 🟡 Low - **Bug Type**: Missing Safety Documentation ```rust pub fn format(&self) -> &str { assert!(!self.format.is_null()); // safe because the lifetime of `self.format` equals `self` unsafe { CStr::from_ptr(self.format) } .to_str() .expect("The external API has a non-utf8 as format") } ``` * **Missing safety comment:** The comments are brief and do not explain why it is safe to dereference the raw pointer (i.e. validity, null-termination). * **Proposed safety comment for `format`:** ```rust // SAFETY: // - `self.format` is checked to be non-null. // - The provider of `FFI_ArrowSchema` guarantees that `format` points to a valid, // null-terminated UTF-8 string that remains valid for the lifetime of `self`. ``` ### 3. `FFI_ArrowSchema::child` 🟡 - **Priority**: 🟡 Low - **Bug Type**: Missing Safety Documentation ```rust pub fn child(&self, index: usize) -> &Self { assert!(index < self.n_children as usize); unsafe { self.children.add(index).as_ref().unwrap().as_ref().unwrap() } } ``` * **Missing safety comment:** No safety comment justifies the unsafe operations (pointer arithmetic and double dereferencing). * **Proposed safety comment:** ```rust // SAFETY: // - By FFI contract or local construction invariant, `self.children` is a valid pointer // pointing to an allocated array containing at least `self.n_children` elements. // - Since `index < self.n_children`, `self.children.add(index)` is within the bounds of // the allocated array. // - The pointers contained in the array are non-null and point to valid, initialized // `FFI_ArrowSchema` instances that remain valid as long as `self` is live. ``` ### 4. `FFI_ArrowSchema::dictionary` 🟡 - **Priority**: 🟡 Low - **Bug Type**: Missing Safety Documentation ```rust pub fn dictionary(&self) -> Option<&Self> { unsafe { self.dictionary.as_ref() } } ``` * **Missing safety comment:** No safety comment justifies the pointer dereference. * **Proposed safety comment:** ```rust // SAFETY: // - By FFI contract, if `self.dictionary` is non-null, it points to a valid, // initialized `FFI_ArrowSchema` that remains valid as long as `self` is live. ``` ### 5. `FFI_ArrowSchema::metadata` (helper functions) 🟡 - **Priority**: 🟡 Low - **Bug Type**: Missing Safety Documentation ```rust fn next_four_bytes(buffer: *const u8, pos: &mut isize) -> [u8; 4] { let out = unsafe { [ *buffer.offset(*pos), *buffer.offset(*pos + 1), *buffer.offset(*pos + 2), *buffer.offset(*pos + 3), ] }; *pos += 4; out } ``` * **Missing safety comment:** No safety comment justifies the pointer arithmetic and dereferences. * **Proposed safety comment:** ```rust // SAFETY: // - The caller is responsible for ensuring `buffer` points to a valid Arrow metadata // allocation. Under Arrow metadata format, the buffer must contain at least the // number of bytes required by the parsed schema structure. // - This reads bytes sequentially within the bounds guaranteed by the structure. ``` ### 6. `Drop for FFI_ArrowSchema` 🟡 - **Priority**: 🟡 Low - **Bug Type**: Missing Safety Documentation ```rust impl Drop for FFI_ArrowSchema { fn drop(&mut self) { match self.release { None => (), Some(release) => unsafe { release(self) }, }; } } ``` * **Missing safety comment:** No safety comment justifying the invocation of the C function pointer. * **Proposed safety comment:** ```rust // SAFETY: // - If `self.release` is `Some`, it points to a valid release callback function. // - The callback expects a mutable pointer to the schema and takes ownership of releasing // its internal allocations. // - By dropping `self`, we will not access the schema again, so transferring ownership to // the release callback is sound. ``` ### 7. `unsafe impl Send for FFI_ArrowSchema` 🟡 - **Priority**: 🟡 Low - **Bug Type**: Missing Safety Documentation ```rust unsafe impl Send for FFI_ArrowSchema {} ``` * **Missing safety comment:** No comment explaining why thread safety holds. * **Proposed safety comment:** ```rust // SAFETY: // - `FFI_ArrowSchema` owns all its heap-allocated buffers (via `private_data` or FFI contract). // - It does not expose shared mutability or reference-counted pointer structures that lack // synchronization. // - Therefore, transferring ownership of an `FFI_ArrowSchema` to another thread is sound. ``` </details> -- 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]
