bit2swaz opened a new pull request, #10324:
URL: https://github.com/apache/arrow-rs/pull/10324
## Which issue does this PR close?
Fixes #10284
## Rationale for this change
`b64_encode` builds a `GenericStringArray` from the output of a
caller-supplied `base64::Engine` via `new_unchecked` guarded by a `// Safety:
Base64 is valid UTF-8` comment. That comment only holds for a *correct* engine.
`base64::Engine` is a safe trait so a caller can implement it in 100% safe Rust
to write non-UTF-8 bytes into the buffer. The resulting `StringArray` then
hands out an invalid `&str` which is UB reached from entirely safe code
A safe fn has to be sound for all safe inputs so this is a soundness hole.
It is not a remotely exploitable vuln: the trigger is a caller-supplied
`Engine`, not untrusted data
## What changes are included in this PR?
- `b64_encode` now returns `Result<GenericStringArray<O>, ArrowError>` and
builds the array through the checked `GenericStringArray::try_new` constructor
instead of `unsafe { new_unchecked(...) }`. A misbehaving engine now surfaces
as an `Err` rather than UB. This matches `b64_decode`, which already returns
`Result`
- The `unsafe` block is gone
- Updated the one in-tree caller (the `arrow-json` doc example)
- Added a regression test with a safe-Rust `EvilEngine` (the issue's repro)
that asserts `b64_encode` returns `Err`
`b64_decode` is untouched. It returns `GenericBinaryArray`, which has no
UTF-8 invariant, so this bug does not apply to it
## Verification
- `cargo test -p arrow-cast -p arrow-json` passes.
- Checked under Miri both ways: the `EvilEngine` repro aborts with UB
(`char::from_u32_unchecked`) against the current `new_unchecked` code, and
returns `Err` with no UB after this fix.
## Are there any user-facing changes?
Yes, this is a breaking API change. `b64_encode` now returns `Result`, so
callers have to handle the error (with `?` or `.unwrap()`).
There is an alternative: mark `b64_encode` as `unsafe fn` and document the
engine precondition. That has zero runtime cost but pushes `unsafe` onto every
honest caller. I went with the `Result` route to keep the safe API safe, but
I'm happy to switch to the `unsafe fn` shape if maintainers prefer it.
Credit to the reporter (@Manishearth) for the Miri repro
--
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]
