cakeni commented on code in PR #10813:
URL: https://github.com/apache/arrow-rs/pull/10813#discussion_r3848087512
##########
arrow-select/src/take.rs:
##########
@@ -462,37 +462,169 @@ fn take_bits<I: ArrowPrimitiveType>(
indices: &PrimitiveArray<I>,
) -> BooleanBuffer {
let len = indices.len();
+ let src_offset = values.offset();
+ let src_ptr = values.values().as_ptr();
+ let out_bytes = len.div_ceil(8);
match indices.nulls().filter(|n| n.null_count() > 0) {
Some(nulls) => {
- let mut output_buffer = MutableBuffer::new_null(len);
- let output_slice = output_buffer.as_slice_mut();
- nulls.valid_indices().for_each(|idx| {
- // SAFETY: idx is a valid index in indices.nulls() -->
idx<indices.len()
- if values.value(unsafe {
indices.value_unchecked(idx).as_usize() }) {
- // SAFETY: MutableBuffer was created with space for
indices.len() bit, and idx < indices.len()
- unsafe { bit_util::set_bit_raw(output_slice.as_mut_ptr(),
idx) };
+ let mut output = MutableBuffer::new_null(len);
+ let out_ptr = output.as_mut_ptr();
+ nulls.valid_indices().for_each(|i| {
+ // SAFETY: i < len from the validity bitmap
+ let src_idx = unsafe { indices.value_unchecked(i) }.as_usize()
+ src_offset;
+ // SAFETY: src_idx bounded by take's prior bounds check
+ unsafe {
+ if bit_util::get_bit_raw(src_ptr, src_idx) {
Review Comment:
I think this makes the unchecked `take` path unsound. `take` only calls
`check_bounds` when `TakeOptions::check_bounds` is enabled, but this now
dereferences `src_ptr` assuming the index was already checked. With the default
`check_bounds = false`, an out-of-range index is documented to panic; here it
can instead read past the buffer. The same issue seems to apply to
`take_bits_with_validity`. Could we preserve the panic behavior before doing
the raw reads?
--
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]
