Skip to content

fix: validate patch indices read from a file instead of panicking #9741

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jackylee-ch wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

fix: validate patch indices read from a file instead of panicking #9741

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions encodings/alp/src/alp/decompress.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,15 +38,15 @@ pub fn decompress_into_array(
let patches_chunk_offsets = chunk_offsets.clone().execute::<PrimitiveArray>(ctx)?;
let patches_indices = p.indices().clone().execute::<PrimitiveArray>(ctx)?;
let patches_values = p.values().clone().execute::<PrimitiveArray>(ctx)?;
Ok(decompress_chunked_core(
decompress_chunked_core(
prim_encoded,
exponents,
&patches_indices,
&patches_values,
&patches_chunk_offsets,
p,
dtype,
))
)
} else {
let encoded_prim = encoded.execute::<PrimitiveArray>(ctx)?;
decompress_unchunked_core(encoded_prim, exponents, patches, dtype, ctx)
Expand All @@ -72,15 +72,15 @@ pub fn execute_decompress(array: ALPArray, ctx: &mut ExecutionCtx) -> VortexResu
let patches_chunk_offsets = chunk_offsets.clone().execute::<PrimitiveArray>(ctx)?;
let patches_indices = p.indices().clone().execute::<PrimitiveArray>(ctx)?;
let patches_values = p.values().clone().execute::<PrimitiveArray>(ctx)?;
Ok(decompress_chunked_core(
decompress_chunked_core(
encoded,
exponents,
&patches_indices,
&patches_values,
&patches_chunk_offsets,
p,
dtype,
))
)
} else {
let encoded = encoded.execute::<PrimitiveArray>(ctx)?;
decompress_unchunked_core(encoded, exponents, patches, dtype, ctx)
Expand All @@ -103,7 +103,7 @@ fn decompress_chunked_core(
patches_chunk_offsets: &PrimitiveArray,
patches: &Patches,
dtype: DType,
) -> PrimitiveArray {
) -> VortexResult<PrimitiveArray> {
let validity = encoded
.validity()
.vortex_expect("ALP validity should be derivable");
Expand Down Expand Up @@ -135,11 +135,11 @@ fn decompress_chunked_core(
patches_chunk_offsets,
chunk_idx,
offset_within_chunk,
);
)?;
}

let decoded_buffer: BufferMut<T> = unsafe { transmute(alp_buffer) };
PrimitiveArray::new::<T>(decoded_buffer.freeze(), validity)
Ok(PrimitiveArray::new::<T>(decoded_buffer.freeze(), validity))
})
})
})
Expand Down
63 changes: 58 additions & 5 deletions vortex-array/src/arrays/bool/patch.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use crate::arrays::PrimitiveArray;
use crate::arrays::bool::BoolArrayExt;
use crate::match_each_unsigned_integer_ptype;
use crate::patches::Patches;
use crate::validity::check_patch_indices;

impl BoolArray {
pub fn patch(self, patches: &Patches, ctx: &mut ExecutionCtx) -> VortexResult<Self> {
Expand All @@ -28,11 +29,11 @@ impl BoolArray {
.try_into_mut()
.unwrap_or_else(|bb| BitBufferMut::copy_from(&bb));
match_each_unsigned_integer_ptype!(indices.ptype(), |I| {
for (idx, value) in indices
.as_slice::<I>()
.iter()
.zip_eq(values.bit_buffer_view().iter())
{
let indices = indices.as_slice::<I>();
// Checked up front so `set_to` below cannot exceed the buffer; see
// `check_patch_indices` for why construction is not enough.
check_patch_indices(indices, offset, len)?;
for (idx, value) in indices.iter().zip_eq(values.bit_buffer_view().iter()) {
#[allow(clippy::cast_possible_truncation)]
own_values.set_to(*idx as usize - offset, value);
}
Expand All @@ -45,12 +46,64 @@ impl BoolArray {
#[cfg(test)]
mod tests {
use vortex_buffer::BitBuffer;
use vortex_buffer::buffer;

use crate::IntoArray;
use crate::VortexSessionExecute;
use crate::array_session;
use crate::arrays::BoolArray;
use crate::assert_arrays_eq;
use crate::patches::Patches;

/// The reported crash: `BitBufferMut::set` panicked with
/// "index 402653634 exceeds len 1024" while patching bool validity read from a
/// file. The indices are unsorted so the last-element maximum check in
/// `Patches::new` does not see the out-of-range value, and sortedness is only
/// asserted under `debug_assertions`.
#[test]
fn patch_rejects_out_of_range_index() {
let mut ctx = array_session().create_execution_ctx();
let array = BoolArray::from(BitBuffer::new_set(8));
let patches = unsafe {
Patches::new_unchecked(
8,
0,
buffer![1u64, 402_653_634, 2].into_array(),
BoolArray::from_iter([false, false, false]).into_array(),
None,
None,
)
};

let err = array
.patch(&patches, &mut ctx)
.expect_err("out-of-range patch index must be rejected");
assert!(
err.to_string().contains("402653634"),
"unexpected error: {err}"
);
}

/// A valid patch set must still be applied — the check must not over-reject.
#[test]
fn patch_accepts_in_range_indices() {
let mut ctx = array_session().create_execution_ctx();
let array = BoolArray::from(BitBuffer::new_set(4));
let patches = unsafe {
Patches::new_unchecked(
4,
0,
buffer![1u64, 3].into_array(),
BoolArray::from_iter([false, false]).into_array(),
None,
None,
)
};

let patched = array.patch(&patches, &mut ctx).unwrap();
let expected = BoolArray::from_iter([true, false, true, false]);
assert_arrays_eq!(patched, expected, &mut ctx);
}

#[test]
fn patch_sliced_bools() {
Expand Down
141 changes: 133 additions & 8 deletions vortex-array/src/arrays/primitive/array/patch.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
use std::ops::Range;

use vortex_error::VortexResult;
use vortex_error::vortex_err;

use crate::ExecutionCtx;
use crate::IntoArray;
Expand All @@ -16,6 +17,7 @@ use crate::match_each_native_ptype;
use crate::patches::PATCH_CHUNK_SIZE;
use crate::patches::Patches;
use crate::validity::Validity;
use crate::validity::check_patch_indices;

impl PrimitiveArray {
pub fn patch(self, patches: &Patches, ctx: &mut ExecutionCtx) -> VortexResult<Self> {
Expand All @@ -30,7 +32,7 @@ impl PrimitiveArray {
&patch_validity,
ctx,
)?;
Ok(match_each_integer_ptype!(patch_indices.ptype(), |I| {
match_each_integer_ptype!(patch_indices.ptype(), |I| {
match_each_native_ptype!(self.ptype(), |T| {
self.patch_typed::<T, I>(
patch_indices,
Expand All @@ -39,7 +41,7 @@ impl PrimitiveArray {
patched_validity,
)
})
}))
})
}

fn patch_typed<T, I>(
Expand All @@ -48,19 +50,23 @@ impl PrimitiveArray {
patch_indices_offset: usize,
patch_values: PrimitiveArray,
patched_validity: Validity,
) -> Self
) -> VortexResult<Self>
where
T: NativePType,
I: IntegerPType,
{
let len = self.len();
let mut own_values = self.into_buffer_mut::<T>();

let patch_indices = patch_indices.as_slice::<I>();
let patch_values = patch_values.as_slice::<T>();
// Checked up front so the write loop below cannot index out of range; see
// `check_patch_indices` for why construction is not enough.
check_patch_indices(patch_indices, patch_indices_offset, len)?;
for (idx, value) in itertools::zip_eq(patch_indices, patch_values) {
own_values[idx.as_() - patch_indices_offset] = *value;
}
Self::new(own_values, patched_validity)
Ok(Self::new(own_values, patched_validity))
}
}

Expand Down Expand Up @@ -100,7 +106,8 @@ pub fn patch_chunk<T, I, C>(
chunk_offsets_slice: &[C],
chunk_idx: usize,
offset_within_chunk: usize,
) where
) -> VortexResult<()>
where
T: NativePType,
I: UnsignedPType,
C: UnsignedPType,
Expand All @@ -124,22 +131,139 @@ pub fn patch_chunk<T, I, C>(
let chunk_start = chunk_range(chunk_idx, patches_offset, /* ignore */ usize::MAX).start;

for patches_idx in patches_start_idx..patches_end_idx {
let chunk_relative_index =
(patches_indices[patches_idx].as_() - patches_offset) - chunk_start;
// A patch index deserialized from a file need not lie inside this chunk, and
// neither subtraction is guaranteed not to wrap; reject instead of indexing
// blind. See `check_patch_indices` for why construction is not enough.
let absolute = patches_indices[patches_idx].as_();
let chunk_relative_index = absolute
.checked_sub(patches_offset)
.and_then(|i| i.checked_sub(chunk_start))
.filter(|i| *i < decoded_values.len())
.ok_or_else(|| {
vortex_err!(
"patch index {absolute} is out of bounds for chunk {chunk_idx} \
(offset {patches_offset}, chunk start {chunk_start}, chunk length {})",
decoded_values.len()
)
})?;
decoded_values[chunk_relative_index] = patches_values[patches_idx];
}
Ok(())
}

#[cfg(test)]
mod tests {
use vortex_buffer::buffer;

use super::*;
use crate::IntoArray;
use crate::VortexSessionExecute;
use crate::array_session;
use crate::assert_arrays_eq;
use crate::validity::Validity;

/// The primitive counterpart of the reported crash: `own_values[idx - offset]`
/// indexed blind, so an out-of-range index read from a file panicked.
#[test]
fn patch_rejects_out_of_range_index() {
let mut ctx = array_session().create_execution_ctx();
let array = PrimitiveArray::new::<i32>(buffer![1i32, 2, 3, 4], Validity::NonNullable);
let patches = unsafe {
Patches::new_unchecked(
4,
0,
buffer![1u64, 288_230_376_151_712_349, 2].into_array(),
buffer![10i32, 20, 30].into_array(),
None,
None,
)
};

let err = array
.patch(&patches, &mut ctx)
.expect_err("out-of-range patch index must be rejected");
assert!(
err.to_string().contains("288230376151712349"),
"unexpected error: {err}"
);
}

/// A valid patch set must still be applied — the check must not over-reject.
#[test]
fn patch_accepts_in_range_indices() {
let mut ctx = array_session().create_execution_ctx();
let array = PrimitiveArray::new::<i32>(buffer![1i32, 2, 3, 4], Validity::NonNullable);
let patches = unsafe {
Patches::new_unchecked(
4,
0,
buffer![1u64, 3].into_array(),
buffer![20i32, 40].into_array(),
None,
None,
)
};

let patched = array.patch(&patches, &mut ctx).unwrap();
let expected = PrimitiveArray::new::<i32>(buffer![1i32, 20, 3, 40], Validity::NonNullable);
assert_arrays_eq!(patched, expected, &mut ctx);
}

/// A patch index below the offset must not wrap into a huge usize.
#[test]
fn patch_rejects_index_below_offset() {
let mut ctx = array_session().create_execution_ctx();
let array = PrimitiveArray::new::<i32>(buffer![1i32, 2], Validity::NonNullable);
let patches = unsafe {
Patches::new_unchecked(
2,
100,
buffer![3u64].into_array(),
buffer![10i32].into_array(),
None,
None,
)
};

array
.patch(&patches, &mut ctx)
.expect_err("index below the offset must be rejected");
}

/// A patch index past the end of the chunk must be an error, not an OOB write.
/// `Patches::new` cannot catch this: it derives the maximum from the last index,
/// which is only the maximum when the indices are sorted.
#[test]
fn patch_chunk_rejects_out_of_range_index() {
let mut decoded_values = vec![0.0f64; 8];
let patches_indices: Vec<u64> = vec![1, 402_653_634, 2];
let patches_values: Vec<f64> = vec![1.0, 2.0, 3.0];
let chunk_offsets: Vec<u32> = vec![0];

let err = patch_chunk(
&mut decoded_values,
&patches_indices,
&patches_values,
0,
&chunk_offsets,
0,
0,
)
.expect_err("out-of-range patch index must be rejected");
assert!(
err.to_string().contains("402653634"),
"unexpected error: {err}"
);
}

/// An index below the patches offset must not wrap.
#[test]
fn patch_chunk_rejects_index_below_offset() {
let mut decoded_values = vec![0.0f64; 8];
patch_chunk(&mut decoded_values, &[3u64], &[1.0f64], 100, &[0u32], 0, 0)
.expect_err("index below the offset must be rejected");
}

/// Regression: patch_chunk must not OOB when chunk_offsets (chunk granularity)
/// reference more patches than patches_indices (element granularity) contains.
#[test]
Expand All @@ -162,7 +286,8 @@ mod tests {
&chunk_offsets,
1,
3,
);
)
.unwrap();

// Spot-check: patch index 4 (first in range) should be applied.
assert_ne!(
Expand Down
Loading