Rich-T-kid commented on code in PR #10324:
URL: https://github.com/apache/arrow-rs/pull/10324#discussion_r3567201102
##########
arrow-cast/src/base64.rs:
##########
@@ -113,4 +115,49 @@ mod tests {
test_engine(&BASE64_STANDARD, &data);
test_engine(&BASE64_STANDARD_NO_PAD, &data);
}
+
+ /// Safe-Rust `Engine` that writes invalid UTF-8 into the encode buffer
+ /// (#10284). `b64_encode` must reject it rather than build an unsound
+ /// `StringArray`.
+ 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 {
+ BASE64_STANDARD.internal_encode(input, output)
+ }
+ fn internal_decoded_len_estimate(&self, input_len: usize) ->
Self::DecodeEstimate {
+ BASE64_STANDARD.internal_decoded_len_estimate(input_len)
+ }
+ fn internal_decode(
+ &self,
+ input: &[u8],
+ output: &mut [u8],
+ estimate: Self::DecodeEstimate,
+ ) -> Result<base64::engine::DecodeMetadata, base64::DecodeSliceError> {
+ BASE64_STANDARD.internal_decode(input, output, estimate)
+ }
+ fn config(&self) -> &Self::Config {
+ BASE64_STANDARD.config()
+ }
+ fn encode_slice<T: AsRef<[u8]>>(
+ &self,
+ input: T,
+ output_buf: &mut [u8],
+ ) -> Result<usize, base64::EncodeSliceError> {
+ let len = BASE64_STANDARD.encode_slice(input, output_buf)?;
+ for b in output_buf[..len].iter_mut() {
+ *b = 0xFF; // invalid UTF-8, but correct length
+ }
+ Ok(len)
+ }
+ }
+
+ #[test]
+ fn test_b64_encode_rejects_invalid_utf8() {
+ let data: BinaryArray =
vec![Some(b"hello".to_vec())].into_iter().collect();
+ assert!(b64_encode(&EvilEngine, &data).is_err());
Review Comment:
I think adding similar test for the `validate_utf8 = false` is a good idea.
##########
arrow-cast/src/base64.rs:
##########
@@ -49,10 +53,8 @@ pub fn b64_encode<E: Engine, O: OffsetSizeTrait>(
}
assert_eq!(offset, buffer_len);
- // Safety: Base64 is valid UTF-8
- unsafe {
- GenericStringArray::new_unchecked(offsets, Buffer::from_vec(buffer),
array.nulls().cloned())
- }
+ // `try_new` validates UTF-8 instead of trusting the (safe-trait) Engine.
+ GenericStringArray::try_new(offsets, Buffer::from_vec(buffer),
array.nulls().cloned())
Review Comment:
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.
```rust
pub fn b64_encode<E: Engine, O: OffsetSizeTrait>(
engine: &E,
array: &GenericBinaryArray<O>,
validate_utf8: boolean,
)
```
and
```rust
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())
}
```
--
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]
