Manishearth opened a new issue, #10284: URL: https://github.com/apache/arrow-rs/issues/10284
> [!NOTE] > This finding was identified during an agentic unsafe Rust code review performed by Gemini AI, followed by human review and verification. ### The Issue `b64_encode` is safe to call and generic over any caller-provided `E: Engine`. https://github.com/apache/arrow-rs/blob/ae8e6c631abf6587ebffae7f87174f60af621855/arrow-cast/src/base64.rs#L31-L56 The `base64::Engine` trait is a safe trait from the external `base64` crate, meaning any type can implement it in 100% safe Rust. Inside `b64_encode`, the function encodes binary data using `engine.encode_slice(...)` and sums the lengths, verifies `assert_eq!(offset, buffer_len)`, and then constructs a `GenericStringArray` via `unsafe { GenericStringArray::new_unchecked(offsets, Buffer::from_vec(buffer), array.nulls().cloned()) }`. Because `base64::Engine` is a safe trait, a custom implementation in safe Rust can write invalid UTF-8 bytes into the output slice while returning a byte count matching the calculated length. When the resulting `GenericStringArray` is accessed in safe Rust (such as calling `.value(i)` to obtain a `&str`), it causes UB by violating Rust's core `&str` UTF-8 validity guarantee. <details><summary>Minimal Reproduction (Miri)</summary> ```rust use arrow_array::BinaryArray; use arrow_cast::base64::b64_encode; use base64::{ engine::{general_purpose::STANDARD, Config, DecodeEstimate, DecodeMetadata}, DecodeSliceError, EncodeSliceError, Engine, }; struct EvilEngine; impl Engine for EvilEngine { type Config = <base64::engine::GeneralPurpose as Engine>::Config; type DecodeEstimate = <base64::engine::GeneralPurpose as Engine>::DecodeEstimate; fn internal_encode(&self, input: &[u8], output: &mut [u8]) -> usize { STANDARD.internal_encode(input, output) } fn internal_decoded_len_estimate(&self, input_len: usize) -> Self::DecodeEstimate { STANDARD.internal_decoded_len_estimate(input_len) } fn internal_decode( &self, input: &[u8], output: &mut [u8], estimate: Self::DecodeEstimate, ) -> Result<DecodeMetadata, DecodeSliceError> { STANDARD.internal_decode(input, output, estimate) } fn config(&self) -> &Self::Config { STANDARD.config() } fn encode_slice<T: AsRef<[u8]>>( &self, input: T, output_buf: &mut [u8], ) -> Result<usize, EncodeSliceError> { let len = STANDARD.encode_slice(input, output_buf)?; // Overwrite standard valid base64 output with invalid UTF-8 (0xFF) for b in output_buf[..len].iter_mut() { *b = 0xFF; } Ok(len) } } fn main() { let data: BinaryArray = vec![Some(b"hello".to_vec())].into_iter().collect(); let encoded = b64_encode(&EvilEngine, &data); // Safe Rust access returning a &str pointing to invalid UTF-8 (0xFF) let s: &str = encoded.value(0); println!("s = {:?}", std::hint::black_box(s)); } ``` ```text error: Undefined Behavior: constructing invalid value of type char: encountered 0x001fffff, but expected a valid unicode scalar value (in `0..=0x10FFFF` but not in `0xD800..=0xDFFF`) --> /usr/local/google/home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/char/methods.rs:239:18 | 239 | unsafe { super::convert::from_u32_unchecked(i) } | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 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::char::methods::<impl char>::from_u32_unchecked at /usr/local/google/home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/char/methods.rs:239:18: 239:55 1: <std::str::Chars<'_> as std::iter::Iterator>::next::{closure#0} at /usr/local/google/home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/str/iter.rs:42:59: 42:87 2: std::option::Option::<u32>::map::<char, {closure@<std::str::Chars<'_> as std::iter::Iterator>::next::{closure#0}}> at /usr/local/google/home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/option.rs:1165:29: 1165:33 3: <std::str::Chars<'_> as std::iter::Iterator>::next at /usr/local/google/home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/str/iter.rs:42:18: 42:88 4: <str as std::fmt::Debug>::fmt at /usr/local/google/home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/fmt/mod.rs:2942:30: 2942:42 5: <&str as std::fmt::Debug>::fmt at /usr/local/google/home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/fmt/mod.rs:2872:62: 2872:82 6: core::fmt::rt::Argument::<'_>::fmt at /usr/local/google/home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/fmt/rt.rs:152:76: 152:95 7: std::fmt::write at /usr/local/google/home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/fmt/mod.rs:1685:17: 1687:80 8: std::io::default_write_fmt::<std::io::StdoutLock<'_>> at /usr/local/google/home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/io/mod.rs:621:11: 621:40 9: <std::io::StdoutLock<'_> as std::io::Write>::write_fmt at /usr/local/google/home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/io/mod.rs:1976:13: 1976:42 10: <&std::io::Stdout as std::io::Write>::write_fmt at /usr/local/google/home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/io/stdio.rs:834:9: 834:36 11: <std::io::Stdout as std::io::Write>::write_fmt at /usr/local/google/home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/io/stdio.rs:808:9: 808:33 12: std::io::stdio::print_to::<std::io::Stdout> at /usr/local/google/home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/io/stdio.rs:1164:21: 1164:47 13: std::io::_print at /usr/local/google/home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/io/stdio.rs:1275:5: 1275:37 14: main at /usr/local/google/home/manishearth/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/macros.rs:146:9: 146:62 ``` </details> <details><summary>Suggested Fix</summary> Either validate the UTF-8 validity of the buffer before calling `new_unchecked` (e.g. using `std::str::from_utf8`), or mark `b64_encode` and `b64_decode` as `unsafe fn` and document the caller safety pre-conditions requiring the `Engine` implementation to produce valid UTF-8. </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_cast` (`v56`) ## Overall Safety Assessment The `arrow_cast` crate contains multiple unsafe operations, primarily around conversions using `from_trusted_len_iter` and `build_unchecked` from `arrow_array`. We found one critical soundness vulnerability where a public safe API (`base64::b64_encode`) accepts a caller-provided implementation of a safe trait (`base64::Engine`) and uses its output to construct a `StringArray` via `new_unchecked`. Because a malicious or incorrect safe implementation of the trait can write invalid UTF-8, this allows safe Rust to trigger Undefined Behavior. Additionally, several internal unsafe blocks lack safety documentation (`// SAFETY:` comments) or contain inaccurate/misleading safety comments. ## Critical Findings ### Unsound safe API `base64::b64_encode` due to safe trait implementation trust ๐ด ๐งช - **Priority**: ๐ด High - **Threat Vector**: ๐งช Contrived Setup - **Bug Type**: `Unsound Trait Trust` - **Location**: `src/base64.rs` ([base64.rs:L53-L56](file:///google/src/cloud/corpagent-eng-manishearth/unsafe-rust-review-2/google3/third_party/rust/arrow_cast/v56/src/base64.rs#L53-L56)) - **Description**: The public function `pub fn b64_encode<E: Engine, O: OffsetSizeTrait>` is safe to call and generic over any `E: Engine`. The `base64::Engine` trait is a safe trait from the `base64` dependency. In `b64_encode`, the caller-provided `engine` is used to encode binary data: ```rust for i in 0..array.len() { let len = engine .encode_slice(array.value(i), &mut buffer[offset..]) .unwrap(); offset += len; } ``` The function then constructs a `GenericStringArray` without validating the resulting buffer's UTF-8 correctness: ```rust // Safety: Base64 is valid UTF-8 unsafe { GenericStringArray::new_unchecked(offsets, Buffer::from_vec(buffer), array.nulls().cloned()) } ``` Because `base64::Engine` is a safe trait, a user can implement it in safe Rust to write invalid UTF-8 bytes to the slice, or return an incorrect length that exposes uninitialized or invalid UTF-8 bytes in the buffer. When the resulting `GenericStringArray` is used in safe Rust (e.g., via `value(i)` which returns `&str`), it will result in Undefined Behavior (violating the `&str` UTF-8 validity guarantee). **Remediation**: Either: 1. Validate the UTF-8 validity of the buffer before calling `new_unchecked` (e.g. using `std::str::from_utf8`). 2. Mark `b64_encode` as `unsafe fn` and document that the caller must guarantee that the `Engine` produces valid UTF-8. ## Fishy Findings ### Misleading safety comment in `cast_dict_to_byte_view` ๐ก โ ๏ธ - **Priority**: ๐ก Low - **Threat Vector**: โ ๏ธ Accidental Misuse - **Bug Type**: `Misleading Safety Comment` - **Location**: `src/cast/dictionary.rs` ([dictionary.rs:L145-L153](file:///google/src/cloud/corpagent-eng-manishearth/unsafe-rust-review-2/google3/third_party/rust/arrow_cast/v56/src/cast/dictionary.rs#L145-L153)) - **Description**: ```rust // Safety // (1) The index is within bounds as they are offsets // (2) The append_view is safe unsafe { let offset = value_offsets.get_unchecked(idx).as_usize(); let end = value_offsets.get_unchecked(idx + 1).as_usize(); let length = end - offset; builder.append_view_unchecked(0, offset as u32, length as u32) } ``` The comment states: "The index is within bounds as they are offsets". This is misleading: `idx` is a dictionary key, not an offset. It is within bounds of the `value_offsets` array because of the type invariant of `DictionaryArray` (which requires all non-null keys to be within `0..values.len()`, which implies that `idx` is valid and `idx + 1 <= values.len()`). ## Missing Safety Comments ### Missing safety comments for `from_trusted_len_iter` in `src/cast/mod.rs` ๐ก - **Priority**: ๐ก Low - **Bug Type**: `Missing Safety Comment` - **Location**: `src/cast/mod.rs` ([mod.rs:L445-L447](file:///google/src/cloud/corpagent-eng-manishearth/unsafe-rust-review-2/google3/third_party/rust/arrow_cast/v56/src/cast/mod.rs#L445-L447), [mod.rs:L462-L464](file:///google/src/cloud/corpagent-eng-manishearth/unsafe-rust-review-2/google3/third_party/rust/arrow_cast/v56/src/cast/mod.rs#L462-L464), [mod.rs:L498-L500](file:///google/src/cloud/corpagent-eng-manishearth/unsafe-rust-review-2/google3/third_party/rust/arrow_cast/v56/src/cast/mod.rs#L498-L500), [mod.rs:L519-L521](file:///google/src/cloud/corpagent-eng-manishearth/unsafe-rust-review-2/google3/third_party/rust/arrow_cast/v56/src/cast/mod.rs#L519-L521)) - **Description**: Several calls to `from_trusted_len_iter` inside `cast_duration_to_interval` lack safety comments entirely. - **Proposed Safety Comments**: ```rust // SAFETY: // `iter`/`vec.iter()` implements `TrustedLen` because it is derived from standard library collections // or checked adapters on trusted iterators, guaranteeing its size hint matches the actual number of items. ``` ### Missing safety comments for `build_unchecked` in `src/cast/mod.rs` ๐ก - **Priority**: ๐ก Low - **Bug Type**: `Missing Safety Comment` - **Location**: `src/cast/mod.rs` ([mod.rs:L2608](file:///google/src/cloud/corpagent-eng-manishearth/unsafe-rust-review-2/google3/third_party/rust/arrow_cast/v56/src/cast/mod.rs#L2608)) - **Description**: The call to `builder.build_unchecked()` lacks safety comments explaining why the resulting array structure matches the layout requirements of `TO::DATA_TYPE`. - **Proposed Safety Comment**: ```rust // SAFETY: // The layout of `TO::DATA_TYPE` is satisfied by `offset_buffer` and `str_values_buf` because they were // cloned/copied from a valid array of type `FROM`, preserving the length, offset, nulls, and data structure, // with only the offset type widened or narrowed (which was checked for overflow). ``` </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]
