Skip to content

Commit f30b8e8

Browse files
andygroveandygrove
committed
refactor(common): return an error from encode_bytes_to_slice on a length mismatch
A debug_assert let release builds silently encode only part of the input when the output buffer was the wrong size. Check unconditionally and return an internal error instead. Also trims the write_digits doc comment to the borrow-checker constraint it needs to explain.
1 parent 5e7e211 commit f30b8e8

2 files changed

Lines changed: 53 additions & 25 deletions

File tree

‎datafusion/common/src/utils/hex.rs‎

Lines changed: 52 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,9 @@
2323
//! integer, trimming leading zeros. All four take a [`HexCase`] to choose
2424
//! between lowercase and uppercase digits.
2525
26+
use crate::Result;
27+
use crate::error::_internal_err;
28+
2629
/// Case of the emitted hex digits.
2730
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
2831
pub enum HexCase {
@@ -83,34 +86,37 @@ pub fn encode_bytes_into(bytes: &[u8], case: HexCase, out: &mut Vec<u8>) {
8386

8487
/// Writes the hex encoding of `bytes` into `out`.
8588
///
86-
/// `out` must be exactly `2 * bytes.len()` bytes long. This is for callers
87-
/// that already own a pre-sized buffer (for example a slice of a larger,
88-
/// pre-allocated output array) and want to write directly into it rather
89-
/// than appending to a `Vec`.
89+
/// This is for callers that already own a pre-sized buffer (for example a
90+
/// slice of a larger, pre-allocated output array) and want to write directly
91+
/// into it rather than appending to a `Vec`.
9092
///
91-
/// The length requirement is only checked in debug builds, via
92-
/// `debug_assert_eq!`. In release builds a mismatched `out` length is not
93-
/// an error: only `min(bytes.len(), out.len() / 2)` bytes are encoded, so
94-
/// an `out` that is too short silently drops the remaining input, and an
95-
/// `out` that is too long is left with unwritten, stale bytes at the end.
96-
/// Callers are responsible for sizing `out` correctly.
93+
/// Returns an internal error if `out` is not exactly `2 * bytes.len()` bytes
94+
/// long, rather than silently encoding only part of the input.
9795
///
9896
/// # Example
9997
///
10098
/// ```
10199
/// use datafusion_common::utils::hex::{HexCase, encode_bytes_to_slice};
102100
///
103101
/// let mut out = [0u8; 8];
104-
/// encode_bytes_to_slice(&[0xde, 0xad, 0xbe, 0xef], HexCase::Lower, &mut out);
102+
/// encode_bytes_to_slice(&[0xde, 0xad, 0xbe, 0xef], HexCase::Lower, &mut out)?;
105103
/// assert_eq!(&out, b"deadbeef");
104+
/// # Ok::<(), datafusion_common::DataFusionError>(())
106105
/// ```
107106
#[inline(always)]
108-
pub fn encode_bytes_to_slice(bytes: &[u8], case: HexCase, out: &mut [u8]) {
109-
debug_assert_eq!(out.len(), bytes.len() * 2);
107+
pub fn encode_bytes_to_slice(bytes: &[u8], case: HexCase, out: &mut [u8]) -> Result<()> {
108+
let expected = bytes.len() * 2;
109+
if out.len() != expected {
110+
return _internal_err!(
111+
"hex output buffer is {} bytes, expected {expected}",
112+
out.len()
113+
);
114+
}
110115
let lookup = case.lookup();
111116
for (&b, chunk) in bytes.iter().zip(out.chunks_exact_mut(2)) {
112117
chunk.copy_from_slice(&lookup[b as usize]);
113118
}
119+
Ok(())
114120
}
115121

116122
/// Returns the hex encoding of `bytes` as an owned `String`.
@@ -162,10 +168,7 @@ pub fn encode_u64(v: u64, case: HexCase, buf: &mut [u8; 16]) -> &[u8] {
162168
/// first digit.
163169
///
164170
/// Split out from [`encode_u64`] so the mutable borrow of `buf` ends before the
165-
/// returned slice reborrows it. The `v == 0` case returns early; if that early
166-
/// return instead reborrowed `buf` as `&buf[..]` inline in [`encode_u64`], the
167-
/// borrow checker would extend that reborrow over the rest of the function
168-
/// body, conflicting with the mutable writes on the non-zero path below.
171+
/// returned slice reborrows it.
169172
#[inline(always)]
170173
fn write_digits(v: u64, case: HexCase, buf: &mut [u8; 16]) -> usize {
171174
if v == 0 {
@@ -300,30 +303,55 @@ mod tests {
300303
}
301304

302305
#[test]
303-
fn encode_bytes_to_slice_empty() {
306+
fn encode_bytes_to_slice_empty() -> Result<()> {
304307
let mut out: [u8; 0] = [];
305-
encode_bytes_to_slice(&[], HexCase::Lower, &mut out);
308+
encode_bytes_to_slice(&[], HexCase::Lower, &mut out)?;
306309
assert_eq!(out, [] as [u8; 0]);
310+
Ok(())
307311
}
308312

309313
#[test]
310-
fn encode_bytes_to_slice_examples() {
314+
fn encode_bytes_to_slice_examples() -> Result<()> {
311315
let mut out = [0u8; 8];
312-
encode_bytes_to_slice(&[0xde, 0xad, 0xbe, 0xef], HexCase::Lower, &mut out);
316+
encode_bytes_to_slice(&[0xde, 0xad, 0xbe, 0xef], HexCase::Lower, &mut out)?;
313317
assert_eq!(&out, b"deadbeef");
314318

315319
let mut out = [0u8; 8];
316-
encode_bytes_to_slice(&[0xde, 0xad, 0xbe, 0xef], HexCase::Upper, &mut out);
320+
encode_bytes_to_slice(&[0xde, 0xad, 0xbe, 0xef], HexCase::Upper, &mut out)?;
317321
assert_eq!(&out, b"DEADBEEF");
322+
Ok(())
318323
}
319324

320325
#[test]
321-
fn encode_bytes_to_slice_agrees_with_encode_bytes() {
326+
fn encode_bytes_to_slice_agrees_with_encode_bytes() -> Result<()> {
322327
let bytes: Vec<u8> = (0..=255u8).collect();
323328
for case in [HexCase::Lower, HexCase::Upper] {
324329
let mut out = vec![0u8; bytes.len() * 2];
325-
encode_bytes_to_slice(&bytes, case, &mut out);
330+
encode_bytes_to_slice(&bytes, case, &mut out)?;
326331
assert_eq!(String::from_utf8(out).unwrap(), encode_bytes(&bytes, case));
327332
}
333+
Ok(())
334+
}
335+
336+
#[test]
337+
fn encode_bytes_to_slice_rejects_wrong_length() {
338+
// Too short: the old `debug_assert` let release builds silently drop
339+
// the remaining input.
340+
let mut short = [0u8; 6];
341+
let err =
342+
encode_bytes_to_slice(&[0xde, 0xad, 0xbe, 0xef], HexCase::Lower, &mut short)
343+
.unwrap_err();
344+
assert!(
345+
err.message()
346+
.contains("hex output buffer is 6 bytes, expected 8"),
347+
"unexpected message: {err}"
348+
);
349+
350+
// Too long: would have left stale bytes at the tail.
351+
let mut long = [0u8; 10];
352+
assert!(
353+
encode_bytes_to_slice(&[0xde, 0xad, 0xbe, 0xef], HexCase::Lower, &mut long)
354+
.is_err()
355+
);
328356
}
329357
}

‎datafusion/functions/src/encoding/inner.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -479,7 +479,7 @@ where
479479
for v in array.iter() {
480480
if let Some(v) = v {
481481
let out_len = v.len() * 2;
482-
encode_bytes_to_slice(v, HexCase::Lower, &mut values[pos..pos + out_len]);
482+
encode_bytes_to_slice(v, HexCase::Lower, &mut values[pos..pos + out_len])?;
483483
pos += out_len;
484484
}
485485
offsets.push(OutputOffset::usize_as(pos));

0 commit comments

Comments
 (0)