fix(arrow-cast): make b64_encode reject invalid UTF-8 from misbehaving Engine impls

[bit2swaz]

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

[Rich-T-kid]

Similar to what @Jefffrey mentioned here

It feels weird that we should defend in our downstream use here when we should be able to rely on the base64 engine producing correct base64 encoding (and thus valid utf8)

I don't think it's arrow-rs's responsibility to validate this. I think it's a good idea to give users the option to validate it themselves, without causing a performance regression for users who don't need that validation.

[Rich-T-kid]

I think adding similar test for the validate_utf8 = false is a good idea.

[Rich-T-kid]

I think it makes sense to have either an alternate function or passing in a boolean for utf8 validation.
I don't think its should be arrow-rs responsibility to validate the engine by default, especially when it comes with performance penalty.

maybe something similar to

pub fn b64_encode<E: Engine, O: OffsetSizeTrait>(
    engine: &E,
    array: &GenericBinaryArray<O>,
    validate_utf8: boolean,
) 

and

switch validate_utf8 {
true  => GenericStringArray::try_new(offsets, Buffer::from_vec(buffer), array.nulls().cloned())
false => GenericStringArray::new_unchecked(offsets, Buffer::from_vec(buffer), array.nulls().cloned())
}

to avoid introducing an breaking API change it may make sense to make a separate function that handles the validation.

pub fn b64_encode_validate<E: Engine, O: OffsetSizeTrait>(
    engine: &E,
    array: &GenericBinaryArray<O>,
) 

[Jefffrey]

i'll need to think on this a little; i dont think having a separate validate_utf8 flag would solve this as there would still be the loophole of exhibiting UB via completely safe APIs. marking as unsafe is the "easiest" fix but it seems unfortunate to do this when its for quite an edge case (indeed the docs for the base64 crate states that almost no one should need to implement Engine 🤔)

another solution could be to change the return type to be a binary array instead of a string array, since it then pushes the utf8 validation onto the user, though it seems also a clunky solution 🙁

[bit2swaz]

the core thing id push back on: a validate_utf8: bool (or a _validate variant) that keeps new_unchecked on the false path doesnt fix the soundness hole, it barely renames it. any safe fn that can reach new_unchecked is really just the original bug, validate_utf8=false becomes UB-on-demand from 100% safe rust. @Jefffrey already spotted this ("still be the loophole of exhibiting UB via completely safe APIs"). so a bool doesnt buy back the perf without also making the api unsound

to keep new_unchecked (zero-cost) the fast path has to be unsafe fn so the caller takes on the utf-8 proof obligation. thats @Manishearth's point on the issue: a library has to opt in to having its output relied on in unsafe code and "behaves right for a correct engine" != "sound from all safe callers"

so there are really only two sound shapes:

1. safe fn returning Result, validates always (this PR)
2. unsafe fn keeping new_unchecked, zero-cost, precondition documented

on the perf worry: try_new's check is str::from_utf8 over the buffer, a simd linear scan, cheap next to the base64 encode that already touched every byte. i can benchmark it if that would settle it. also worth noting the honest common case is BASE64_STANDARD which is always valid utf-8, so the validation never fails in practice, it just closes the door on the contrived engine.

re @Jefffrey's "return a binary array" idea: its sound but just relocates the same from_utf8 onto every caller and breaks the api harder than a Result so honestly it seems strictly worse than option 1 to me

happy to go either way tho. this PR is option 1. if youd rather keep it zero-cost then i'll switch it to the unsafe fn shape (option 2) instead, just lmk which you prefer :)

[Jefffrey]

the only other ways i can think of is either a reflection based approach, e.g.

    use std::any::TypeId;
    if TypeId::of::<E>() == TypeId::of::<base64::engine::GeneralPurpose>() {
        // Safety: Base64 is valid UTF-8
        unsafe {
            GenericStringArray::new_unchecked(
                offsets,
                Buffer::from_vec(buffer),
                array.nulls().cloned(),
            )
        }
    } else {
        GenericStringArray::try_new(offsets, Buffer::from_vec(buffer), array.nulls().cloned())
    }

or for something more flexible, a trait based approach like

/// Implement this on an engine that guarantees it produces valid utf8
pub unsafe trait SafeBase64 {}

unsafe impl SafeBase64 for base64::engine::GeneralPurpose {}

/// Bas64 encode each element of `array` with the provided [`Engine`]
pub fn b64_encode<E: Engine + SafeBase64, O: OffsetSizeTrait>(

frankly i dont know how common this base64 code is used downstream anyway (i cant find any usages in datafusion) so maybe we dont need to be overkill and can just force validation 🤔

edit: or we could remove it being generic over an engine and just make it take only GeneralPurpose; maybe the intersection of people who use this function and implement a custom engine (for a valid usecase) is zero?

[bit2swaz]

agreed, id just force validation. this PR already is that so its ready if youre happy with it :)

the thing with the other two is that TypeId needs E: 'static and only fast-paths the one concrete GeneralPurpose, every other honest engine (GeneralPurposeNoPad, custom alphabets) still hits try_new, so its unsafe + a footgun to speed up exactly one type. doesnt seem worth it

SafeBase64 is the correct zero-cost shape if we ever want it, the caller proves the invariant via unsafe impl and a dishonest engine just wont compile. but its a new public unsafe trait for an edge case with no known downstream user (like you said you couldnt find one in datafusion). easy to add later if someone shows up needing it. dont think theres a need to ship it speculatively

dropping the generic to GeneralPurpose-only works too but thats a bigger breaking change and kills the Engine abstraction entirely and feels orthogonal to the soundness fix. happy to do it in a separate PR if youd prefer that direction tho