[Variant] Fix several overflow panic risks for 32-bit arch (#7752)

# Which issue does this PR close?

Part of
* #6736

# Rationale for this change

The variant spec makes extensive use of 4-byte offsets. On a 32-bit
system, this can lead to integer overflow panics when doing offset
arithmetic on malicious or malformed variant data, because `usize` is 32
bits. That is bad.

# What changes are included in this PR?

Use checked arithmetic in code that operates on offsets extracted from
(untrusted) variant byte buffers.

Of particular interest, we define and use a new
`slice_from_slice_at_offset` helper, which safely adds an offset to a
range.

# Are there any user-facing changes?

No.

Author: scovich

parquet-variant/src/decoder.rs

@@ -14,7 +14,7 @@
 // KIND, either express or implied.  See the License for the
 // specific language governing permissions and limitations
 // under the License.
-use crate::utils::{array_from_slice, slice_from_slice, string_from_slice};
+use crate::utils::{array_from_slice, slice_from_slice_at_offset, string_from_slice};
 use crate::ShortString;
 
 use arrow_schema::ArrowError;
@@ -23,6 +23,9 @@ use chrono::{DateTime, Duration, NaiveDate, NaiveDateTime, Utc};
 use std::array::TryFromSliceError;
 use std::num::TryFromIntError;
 
+// Makes the code a bit more readable
+pub(crate) const VARIANT_VALUE_HEADER_BYTES: usize = 1;
+
 #[derive(Debug, Clone, Copy, PartialEq)]
 pub enum VariantBasicType {
     Primitive = 0,
@@ -262,21 +265,19 @@ pub(crate) fn decode_timestampntz_micros(data: &[u8]) -> Result<NaiveDateTime, A
 /// Decodes a Binary from the value section of a variant.
 pub(crate) fn decode_binary(data: &[u8]) -> Result<&[u8], ArrowError> {
     let len = u32::from_le_bytes(array_from_slice(data, 0)?) as usize;
-    let value = slice_from_slice(data, 4..4 + len)?;
-    Ok(value)
+    slice_from_slice_at_offset(data, 4, 0..len)
 }
 
 /// Decodes a long string from the value section of a variant.
 pub(crate) fn decode_long_string(data: &[u8]) -> Result<&str, ArrowError> {
     let len = u32::from_le_bytes(array_from_slice(data, 0)?) as usize;
-    let string = string_from_slice(data, 4..4 + len)?;
-    Ok(string)
+    string_from_slice(data, 4, 0..len)
 }
 
 /// Decodes a short string from the value section of a variant.
 pub(crate) fn decode_short_string(metadata: u8, data: &[u8]) -> Result<ShortString, ArrowError> {
     let len = (metadata >> 2) as usize;
-    let string = string_from_slice(data, 0..len)?;
+    let string = string_from_slice(data, 0, 0..len)?;
     ShortString::try_new(string)
 }
 
@@ -518,10 +519,11 @@ mod tests {
 
         let width = OffsetSizeBytes::Two;
 
-        // dictionary_size starts immediately after the header
+        // dictionary_size starts immediately after the header byte
         let dict_size = width.unpack_usize(&buf, 1, 0).unwrap();
         assert_eq!(dict_size, 2);
 
+        // offset array immediately follows the dictionary size
         let first = width.unpack_usize(&buf, 1, 1).unwrap();
         assert_eq!(first, 0);
 

parquet-variant/src/utils.rs

@@ -21,6 +21,11 @@ use arrow_schema::ArrowError;
 use std::fmt::Debug;
 use std::slice::SliceIndex;
 
+/// Helper for reporting integer overflow errors in a consistent way.
+pub(crate) fn overflow_error(msg: &str) -> ArrowError {
+    ArrowError::InvalidArgumentError(format!("Integer overflow computing {msg}"))
+}
+
 #[inline]
 pub(crate) fn slice_from_slice<I: SliceIndex<[u8]> + Clone + Debug>(
     bytes: &[u8],
@@ -33,17 +38,33 @@ pub(crate) fn slice_from_slice<I: SliceIndex<[u8]> + Clone + Debug>(
         ))
     })
 }
+
+/// Helper to safely slice bytes with offset calculations.
+///
+/// Equivalent to `slice_from_slice(bytes, (base_offset + range.start)..(base_offset + range.end))`
+/// but using checked addition to prevent integer overflow panics on 32-bit systems.
+#[inline]
+pub(crate) fn slice_from_slice_at_offset(
+    bytes: &[u8],
+    base_offset: usize,
+    range: Range<usize>,
+) -> Result<&[u8], ArrowError> {
+    let start_byte = base_offset
+        .checked_add(range.start)
+        .ok_or_else(|| overflow_error("slice start"))?;
+    let end_byte = base_offset
+        .checked_add(range.end)
+        .ok_or_else(|| overflow_error("slice end"))?;
+    slice_from_slice(bytes, start_byte..end_byte)
+}
+
 pub(crate) fn array_from_slice<const N: usize>(
     bytes: &[u8],
     offset: usize,
 ) -> Result<[u8; N], ArrowError> {
-    let bytes = slice_from_slice(bytes, offset..offset + N)?;
-    bytes.try_into().map_err(map_try_from_slice_error)
-}
-
-/// To be used in `map_err` when unpacking an integer from a slice of bytes.
-pub(crate) fn map_try_from_slice_error(e: TryFromSliceError) -> ArrowError {
-    ArrowError::InvalidArgumentError(e.to_string())
+    slice_from_slice_at_offset(bytes, offset, 0..N)?
+        .try_into()
+        .map_err(|e: TryFromSliceError| ArrowError::InvalidArgumentError(e.to_string()))
 }
 
 pub(crate) fn first_byte_from_slice(slice: &[u8]) -> Result<u8, ArrowError> {
@@ -53,9 +74,13 @@ pub(crate) fn first_byte_from_slice(slice: &[u8]) -> Result<u8, ArrowError> {
         .ok_or_else(|| ArrowError::InvalidArgumentError("Received empty bytes".to_string()))
 }
 
-/// Helper to get a &str from a slice based on range, if it's valid or an error otherwise
-pub(crate) fn string_from_slice(slice: &[u8], range: Range<usize>) -> Result<&str, ArrowError> {
-    str::from_utf8(slice_from_slice(slice, range)?)
+/// Helper to get a &str from a slice at the given offset and range, or an error if invalid.
+pub(crate) fn string_from_slice(
+    slice: &[u8],
+    offset: usize,
+    range: Range<usize>,
+) -> Result<&str, ArrowError> {
+    str::from_utf8(slice_from_slice_at_offset(slice, offset, range)?)
         .map_err(|_| ArrowError::InvalidArgumentError("invalid UTF-8 string".to_string()))
 }
 

parquet-variant/src/variant.rs

@@ -168,13 +168,13 @@ impl<'a> TryFrom<&'a str> for ShortString<'a> {
     }
 }
 
-impl<'a> AsRef<str> for ShortString<'a> {
+impl AsRef<str> for ShortString<'_> {
     fn as_ref(&self) -> &str {
         self.0
     }
 }
 
-impl<'a> Deref for ShortString<'a> {
+impl Deref for ShortString<'_> {
     type Target = str;
 
     fn deref(&self) -> &Self::Target {

parquet-variant/src/variant/list.rs

@@ -15,11 +15,16 @@
 // specific language governing permissions and limitations
 // under the License.
 use crate::decoder::OffsetSizeBytes;
-use crate::utils::{first_byte_from_slice, slice_from_slice, validate_fallible_iterator};
+use crate::utils::{
+    first_byte_from_slice, overflow_error, slice_from_slice_at_offset, validate_fallible_iterator,
+};
 use crate::variant::{Variant, VariantMetadata};
 
 use arrow_schema::ArrowError;
 
+// The value header occupies one byte; use a named constant for readability
+const NUM_HEADER_BYTES: usize = 1;
+
 /// A parsed version of the variant array value header byte.
 #[derive(Clone, Debug, PartialEq)]
 pub(crate) struct VariantListHeader {
@@ -78,25 +83,16 @@ impl<'m, 'v> VariantList<'m, 'v> {
             false => OffsetSizeBytes::One,
         };
 
-        // Skip the header byte to read the num_elements
-        let num_elements = num_elements_size.unpack_usize(value, 1, 0)?;
-        let first_offset_byte = 1 + num_elements_size as usize;
-
-        let overflow =
-            || ArrowError::InvalidArgumentError("Variant value_byte_length overflow".into());
-
-        // 1.  num_elements + 1
-        let n_offsets = num_elements.checked_add(1).ok_or_else(overflow)?;
-
-        // 2.  (num_elements + 1) * offset_size
-        let value_bytes = n_offsets
-            .checked_mul(header.offset_size as usize)
-            .ok_or_else(overflow)?;
+        // Skip the header byte to read the num_elements; the offset array immediately follows
+        let num_elements = num_elements_size.unpack_usize(value, NUM_HEADER_BYTES, 0)?;
+        let first_offset_byte = NUM_HEADER_BYTES + num_elements_size as usize;
 
-        // 3.  first_offset_byte + ...
-        let first_value_byte = first_offset_byte
-            .checked_add(value_bytes)
-            .ok_or_else(overflow)?;
+        // (num_elements + 1) * offset_size + first_offset_byte
+        let first_value_byte = num_elements
+            .checked_add(1)
+            .and_then(|n| n.checked_mul(header.offset_size as usize))
+            .and_then(|n| n.checked_add(first_offset_byte))
+            .ok_or_else(|| overflow_error("offset of variant list values"))?;
 
         let new_self = Self {
             metadata,
@@ -139,9 +135,10 @@ impl<'m, 'v> VariantList<'m, 'v> {
         };
 
         // Read the value bytes from the offsets
-        let variant_value_bytes = slice_from_slice(
+        let variant_value_bytes = slice_from_slice_at_offset(
             self.value,
-            self.first_value_byte + unpack(index)?..self.first_value_byte + unpack(index + 1)?,
+            self.first_value_byte,
+            unpack(index)?..unpack(index + 1)?,
         )?;
         let variant = Variant::try_new_with_metadata(self.metadata, variant_value_bytes)?;
         Ok(variant)

parquet-variant/src/variant/metadata.rs

@@ -17,7 +17,8 @@
 
 use crate::decoder::OffsetSizeBytes;
 use crate::utils::{
-    first_byte_from_slice, slice_from_slice, string_from_slice, validate_fallible_iterator,
+    first_byte_from_slice, overflow_error, slice_from_slice, string_from_slice,
+    validate_fallible_iterator,
 };
 
 use arrow_schema::ArrowError;
@@ -35,6 +36,9 @@ pub(crate) struct VariantMetadataHeader {
 // purposes and to make that visible.
 const CORRECT_VERSION_VALUE: u8 = 1;
 
+// The metadata header occupies one byte; use a named constant for readability
+const NUM_HEADER_BYTES: usize = 1;
+
 impl VariantMetadataHeader {
     /// Tries to construct the variant metadata header, which has the form
     ///
@@ -102,20 +106,22 @@ impl<'m> VariantMetadata<'m> {
         let header_byte = first_byte_from_slice(bytes)?;
         let header = VariantMetadataHeader::try_new(header_byte)?;
 
-        // Offset 1, index 0 because first element after header is dictionary size
-        let dict_size = header.offset_size.unpack_usize(bytes, 1, 0)?;
+        // First element after header is dictionary size
+        let dict_size = header
+            .offset_size
+            .unpack_usize(bytes, NUM_HEADER_BYTES, 0)?;
 
         // Calculate the starting offset of the dictionary string bytes.
         //
         // Value header, dict_size (offset_size bytes), and dict_size+1 offsets
-        // = 1 + offset_size + (dict_size + 1) * offset_size
-        // = (dict_size + 2) * offset_size + 1
+        // = NUM_HEADER_BYTES + offset_size + (dict_size + 1) * offset_size
+        // = (dict_size + 2) * offset_size + NUM_HEADER_BYTES
         let dictionary_key_start_byte = dict_size
             .checked_add(2)
             .and_then(|n| n.checked_mul(header.offset_size as usize))
-            .and_then(|n| n.checked_add(1))
-            .ok_or_else(|| ArrowError::InvalidArgumentError("metadata length overflow".into()))?;
-        println!("dictionary_key_start_byte: {dictionary_key_start_byte}");
+            .and_then(|n| n.checked_add(NUM_HEADER_BYTES))
+            .ok_or_else(|| overflow_error("offset of variant metadata dictionary"))?;
+
         let new_self = Self {
             bytes,
             header,
@@ -149,16 +155,17 @@ impl<'m> VariantMetadata<'m> {
     /// This offset is an index into the dictionary, at the boundary between string `i-1` and string
     /// `i`. See [`Self::get`] to retrieve a specific dictionary entry.
     fn get_offset(&self, i: usize) -> Result<usize, ArrowError> {
-        // Skipping the header byte (setting byte_offset = 1) and the dictionary_size (setting offset_index +1)
+        // Skip the header byte and the dictionary_size entry (by offset_index + 1)
         let bytes = slice_from_slice(self.bytes, ..self.dictionary_key_start_byte)?;
-        self.header.offset_size.unpack_usize(bytes, 1, i + 1)
+        self.header
+            .offset_size
+            .unpack_usize(bytes, NUM_HEADER_BYTES, i + 1)
     }
 
     /// Gets a dictionary entry by index
     pub fn get(&self, i: usize) -> Result<&'m str, ArrowError> {
-        let dictionary_keys_bytes = slice_from_slice(self.bytes, self.dictionary_key_start_byte..)?;
         let byte_range = self.get_offset(i)?..self.get_offset(i + 1)?;
-        string_from_slice(dictionary_keys_bytes, byte_range)
+        string_from_slice(self.bytes, self.dictionary_key_start_byte, byte_range)
     }
 
     /// Get all dictionary entries as an Iterator of strings

parquet-variant/src/variant/object.rs

@@ -16,12 +16,16 @@
 // under the License.
 use crate::decoder::OffsetSizeBytes;
 use crate::utils::{
-    first_byte_from_slice, slice_from_slice, try_binary_search_range_by, validate_fallible_iterator,
+    first_byte_from_slice, overflow_error, slice_from_slice, try_binary_search_range_by,
+    validate_fallible_iterator,
 };
 use crate::variant::{Variant, VariantMetadata};
 
 use arrow_schema::ArrowError;
 
+// The value header occupies one byte; use a named constant for readability
+const NUM_HEADER_BYTES: usize = 1;
+
 /// Header structure for [`VariantObject`]
 #[derive(Clone, Debug, PartialEq)]
 pub(crate) struct VariantObjectHeader {
@@ -72,36 +76,43 @@ impl<'m, 'v> VariantObject<'m, 'v> {
         let header_byte = first_byte_from_slice(value)?;
         let header = VariantObjectHeader::try_new(header_byte)?;
 
-        // Determine num_elements size based on is_large flag
+        // Determine num_elements size based on is_large flag and fetch the value
         let num_elements_size = if header.is_large {
             OffsetSizeBytes::Four
         } else {
             OffsetSizeBytes::One
         };
+        let num_elements = num_elements_size.unpack_usize(value, NUM_HEADER_BYTES, 0)?;
+
+        // Calculate byte offsets for different sections with overflow protection
+        let field_ids_start_byte = NUM_HEADER_BYTES
+            .checked_add(num_elements_size as usize)
+            .ok_or_else(|| overflow_error("offset of variant object field ids"))?;
 
-        // Parse num_elements
-        let num_elements = num_elements_size.unpack_usize(value, 1, 0)?;
+        let field_offsets_start_byte = num_elements
+            .checked_mul(header.field_id_size as usize)
+            .and_then(|n| n.checked_add(field_ids_start_byte))
+            .ok_or_else(|| overflow_error("offset of variant object field offsets"))?;
 
-        // Calculate byte offsets for different sections
-        let field_ids_start_byte = 1 + num_elements_size as usize;
-        let field_offsets_start_byte =
-            field_ids_start_byte + num_elements * header.field_id_size as usize;
-        let values_start_byte =
-            field_offsets_start_byte + (num_elements + 1) * header.field_offset_size as usize;
+        let values_start_byte = num_elements
+            .checked_add(1)
+            .and_then(|n| n.checked_mul(header.field_offset_size as usize))
+            .and_then(|n| n.checked_add(field_offsets_start_byte))
+            .ok_or_else(|| overflow_error("offset of variant object field values"))?;
 
         // Spec says: "The last field_offset points to the byte after the end of the last value"
         //
         // Use the last offset as a bounds check. The iterator check below doesn't use it -- offsets
         // are not monotonic -- so we have to check separately here.
-        let last_field_offset =
-            header
-                .field_offset_size
-                .unpack_usize(value, field_offsets_start_byte, num_elements)?;
-        if values_start_byte + last_field_offset > value.len() {
+        let end_offset = header
+            .field_offset_size
+            .unpack_usize(value, field_offsets_start_byte, num_elements)?
+            .checked_add(values_start_byte)
+            .ok_or_else(|| overflow_error("end of variant object field values"))?;
+        if end_offset > value.len() {
             return Err(ArrowError::InvalidArgumentError(format!(
-                "Last field offset value {} at offset {} is outside the value slice of length {}",
-                last_field_offset,
-                values_start_byte,
+                "Last field offset value {} is outside the value slice of length {}",
+                end_offset,
                 value.len()
             )));
         }
@@ -140,7 +151,11 @@ impl<'m, 'v> VariantObject<'m, 'v> {
             self.field_offsets_start_byte,
             i,
         )?;
-        let value_bytes = slice_from_slice(self.value, self.values_start_byte + start_offset..)?;
+        let value_start = self
+            .values_start_byte
+            .checked_add(start_offset)
+            .ok_or_else(|| overflow_error("offset of variant object field"))?;
+        let value_bytes = slice_from_slice(self.value, value_start..)?;
         Variant::try_new_with_metadata(self.metadata, value_bytes)
     }