alamb opened a new pull request, #11782: URL: https://github.com/apache/arrow-rs/pull/11782
# Which issue does this PR close? - Relates to #11174 (review discussion: https://github.com/apache/arrow-rs/pull/11174#issuecomment-6305286024) # Rationale for this change `arrow-rs` follows the [Rust Soundness Pledge](https://github.com/apache/arrow-rs/blob/main/arrow/README.md#safety): code must not be able to trigger undefined behavior using safe APIs. The specialized `InProgressArray` implementations added in #9755 have several safe functions whose memory safety depends on their callers: - `InProgressArray::copy_rows` is a safe trait method with no documented precondition on `offset`/`len`, but the primitive and byte view implementations index the source values with `get_unchecked(offset..offset + len)` behind a `SAFETY` comment that relies on the caller. - `append_values_by_indices`, `append_values_by_slices` and `append_views_by_filter` are safe helpers that write through raw pointers using indices taken from a `FilterPredicate`, relying on the predicate matching the length of the source array. A caller passing an out of range `offset`/`len`, or a filter longer than the source array, would read out of bounds through a safe API. This came up while reviewing #11174, where the `copy_rows` pattern was cited as precedent. This PR removes the precedent. # What changes are included in this PR? 1. Replace the two `get_unchecked` range slices in `copy_rows` with ordinary bounds-checked slicing. The cost is a single bounds check per `copy_rows` call (not per element), so I do not expect a measurable performance difference. 2. Mark `append_values_by_indices`, `append_values_by_slices` and `append_views_by_filter` as `unsafe fn` with documented `# Safety` preconditions. 3. Add a `check_filter_len` helper and call it from the safe `copy_rows_by_filter` / `copy_rows_by_filter_from` implementations before invoking the unsafe helpers, returning an error if the filter is longer than the source array. This is one comparison per call. The `filter.count()` invariant the helpers also rely on is already guaranteed by `FilterPredicate` (its only override, `FilterBuilder::with_count`, is `unsafe`). # Are these changes tested? Yes. Existing `coalesce` tests cover the valid-input paths (no behavior change). New tests verify that `copy_rows_by_filter` and `copy_rows_by_filter_from` return an error, rather than reading out of bounds, when the filter is longer than the source array, for both the index and slice iteration strategies. # Are there any significant user-facing changes? No. -- 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]
