Skip to content

Check drain range bounds without overflowing - #104

Open
RaphaelFakhri wants to merge 1 commit into
mozilla:mainfrom
RaphaelFakhri:fix-drain-range-overflow
Open

RaphaelFakhri wants to merge 1 commit into
mozilla:mainfrom
RaphaelFakhri:fix-drain-range-overflow

Conversation

@RaphaelFakhri

Copy link
Copy Markdown

Fixes #102.

Problem

ThinVec::drain (and splice, which calls it) adds 1 to an excluded start bound or an included end bound without an overflow check. In release builds the addition wraps:

  • drain((Bound::Excluded(usize::MAX), Bound::Unbounded)) drains every element.
  • drain(..=usize::MAX) drains nothing.

Vec::drain and ThinVec::extract_if panic for both ranges. Debug builds panic with attempt to add with overflow.

Change

  • Move the slice_range and slice_index_fail helpers out of extract_if to module scope, unchanged.
  • drain now calls slice_range(range, ..len), the same way extract_if does. Invalid ranges panic with the same messages as Vec.
  • Remove the Bound import, which is now unused.

Tests

Adds test_drain_excluded_usize_max_start and test_drain_inclusive_usize_max_end. Both fail before the change, in debug and release builds, and pass after it. cargo fmt --check, cargo clippy --workspace --tests --examples, and the CI test commands (default, serde, gecko-ffi, --no-default-features) all pass.

`drain` and `splice` added 1 to an excluded start bound or an included
end bound without checking for overflow. In release builds,
`drain((Excluded(usize::MAX), Unbounded))` drained the whole vector and
`drain(..=usize::MAX)` drained nothing, instead of panicking like
`Vec::drain`.

Move the `slice_range` helper out of `extract_if` and use it in
`drain` too.

Fixes mozilla#102
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

drain with Excluded(usize::MAX) start or ..=usize::MAX end overflows and drains the wrong range in release

1 participant