bit2swaz commented on PR #10324:
URL: https://github.com/apache/arrow-rs/pull/10324#issuecomment-4983181607
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 :)
--
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]
