From 88eaad298cec475c0c4787899c328a5046576a56 Mon Sep 17 00:00:00 2001 From: Giacomo Stevanato Date: Tue, 19 Mar 2024 16:50:36 +0100 Subject: [PATCH 1/4] Fix UB in IntoOnes --- src/lib.rs | 56 ++++++++++++++++++++++-------------------------------- 1 file changed, 23 insertions(+), 33 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 80a0fe3..60679bf 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -17,10 +17,7 @@ #![deny(clippy::undocumented_unsafe_blocks)] extern crate alloc; -use alloc::{ - vec, - vec::{IntoIter, Vec}, -}; +use alloc::{vec, vec::Vec}; mod block; mod range; @@ -30,8 +27,8 @@ extern crate serde; #[cfg(feature = "serde")] mod serde_impl; +use core::fmt::Write; use core::fmt::{Binary, Display, Error, Formatter}; -use core::{fmt::Write, mem::ManuallyDrop}; use core::cmp::{Ord, Ordering}; use core::iter::{Chain, FusedIterator}; @@ -545,33 +542,24 @@ impl FixedBitSet { /// Iterator element is the index of the `1` bit, type `usize`. /// Unlike `ones`, this function consumes the `FixedBitset`. pub fn into_ones(self) -> IntoOnes { - // SAFETY: This is using the exact same allocation pattern, size, and capacity - // making this reconstruction of the Vec safe. - let mut data = unsafe { - let mut data = ManuallyDrop::new(self.data); - let ptr = data.as_mut_ptr().cast(); - let len = data.len() * SimdBlock::USIZE_COUNT; - let capacity = data.capacity() * SimdBlock::USIZE_COUNT; - Vec::from_raw_parts(ptr, len, capacity) - }; - if data.is_empty() { - IntoOnes { - bitset_front: 0, - bitset_back: 0, - block_idx_front: 0, - block_idx_back: 0, - remaining_blocks: data.into_iter(), - } - } else { - let first_block = data.remove(0); - let last_block = data.pop().unwrap_or(0); - IntoOnes { - bitset_front: first_block, - bitset_back: last_block, - block_idx_front: 0, - block_idx_back: (1 + data.len()) * BITS, - remaining_blocks: data.into_iter(), - } + let ptr = self.data.as_ptr().cast(); + let len = self.data.len() * SimdBlock::USIZE_COUNT; + // SAFETY: + // - ptr comes from self.data, so it is valid; + // - self.data is valid for self.data.len() SimdBlocks, + // which is exactly self.data.len() * SimdBlock::USIZE_COUNT usizes; + // - we will keep this slice around only as long as self.data is, + // so it won't become dangling. + let slice = unsafe { core::slice::from_raw_parts(ptr, len) }; + let mut iter = slice.iter().copied(); + + IntoOnes { + bitset_front: iter.next().unwrap_or(0), + bitset_back: iter.next_back().unwrap_or(0), + block_idx_front: 0, + block_idx_back: len.saturating_sub(1) * BITS, + remaining_blocks: iter, + _buf: self.data, } } @@ -1151,7 +1139,9 @@ pub struct IntoOnes { bitset_back: Block, block_idx_front: usize, block_idx_back: usize, - remaining_blocks: IntoIter, + remaining_blocks: core::iter::Copied>, + // Keep buf along so that `remaining_blocks` remains valid. + _buf: Vec, } impl IntoOnes { From bbee2751085d7f052880ce0689dcef3ac9acb811 Mon Sep 17 00:00:00 2001 From: Giacomo Stevanato Date: Tue, 19 Mar 2024 16:53:28 +0100 Subject: [PATCH 2/4] Make tests faster on MIRI --- src/lib.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 60679bf..8b87ff3 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1595,7 +1595,8 @@ mod tests { #[test] fn size_hint() { - for s in 0..1000 { + let iters = if cfg!(miri) { 250 } else { 1000 }; + for s in 0..iters { let mut bitset = FixedBitSet::with_capacity(s); bitset.insert_range(..); let mut t = s; @@ -1617,7 +1618,8 @@ mod tests { #[test] fn size_hint_alternate() { - for s in 0..1000 { + let iters = if cfg!(miri) { 250 } else { 1000 }; + for s in 0..iters { let mut bitset = FixedBitSet::with_capacity(s); bitset.insert_range(..); let mut t = s; @@ -1678,7 +1680,8 @@ mod tests { #[test] fn count_ones_panic() { - for i in 1..128 { + let iters = if cfg!(miri) { 48 } else { 128 }; + for i in 1..iters { let fb = FixedBitSet::with_capacity(i); for j in 0..fb.len() + 1 { for k in j..fb.len() + 1 { From 2b3d984256c227248a811d161c8abda47e8698db Mon Sep 17 00:00:00 2001 From: Giacomo Stevanato Date: Tue, 19 Mar 2024 16:53:42 +0100 Subject: [PATCH 3/4] Test with MIRI in CI --- .github/workflows/rust.yml | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index 46d42e4..ea08970 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -119,4 +119,21 @@ jobs: with: target: wasm32-unknown-unknown - name: Check wasm - run: cargo check --target wasm32-unknown-unknown \ No newline at end of file + run: cargo check --target wasm32-unknown-unknown + + miri: + runs-on: ubuntu-latest + strategy: + matrix: + # Check builds only on nightly + rust: [nightly] + steps: + - uses: actions/checkout@v4 + - uses: dtolnay/rust-toolchain@stable + with: + profile: minimal + toolchain: ${{ matrix.rust }} + components: miri + override: true + - name: Run miri + run: cargo miri test From 55545632bd616a47e3b8e694b2cdcdb6f0812391 Mon Sep 17 00:00:00 2001 From: Giacomo Stevanato Date: Tue, 19 Mar 2024 16:58:14 +0100 Subject: [PATCH 4/4] Clean up invalid action inputs --- .github/workflows/rust.yml | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index ea08970..09e45a3 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -25,9 +25,7 @@ jobs: - uses: dtolnay/rust-toolchain@stable with: target: x86_64-unknown-linux-gnu - profile: minimal toolchain: ${{ matrix.rust }} - override: true - name: Tests (x86_64) run: | cargo test -v --no-default-features --tests --lib && @@ -45,9 +43,7 @@ jobs: - uses: dtolnay/rust-toolchain@stable with: target: aarch64-unknown-linux-gnu - profile: minimal toolchain: ${{ matrix.rust }} - override: true - name: Tests (aarch64) run: cargo check --target aarch64-unknown-linux-gnu @@ -63,10 +59,8 @@ jobs: - uses: actions/checkout@v4 - uses: dtolnay/rust-toolchain@stable with: - profile: minimal toolchain: ${{ matrix.rust }} components: clippy - override: true - name: Run Clippy run: | cargo clippy @@ -82,10 +76,8 @@ jobs: - uses: actions/checkout@v4 - uses: dtolnay/rust-toolchain@stable with: - profile: minimal toolchain: ${{ matrix.rust }} components: rustfmt - override: true - name: Run Clippy run: | cargo fmt --all --check @@ -101,10 +93,8 @@ jobs: - uses: actions/checkout@v4 - uses: dtolnay/rust-toolchain@stable with: - profile: minimal toolchain: ${{ matrix.rust }} components: clippy - override: true - name: Run Clippy run: | cd benches @@ -131,9 +121,7 @@ jobs: - uses: actions/checkout@v4 - uses: dtolnay/rust-toolchain@stable with: - profile: minimal toolchain: ${{ matrix.rust }} components: miri - override: true - name: Run miri run: cargo miri test