Stress tests and safer behavior on footguns

Author: novacrazy

src/lib.rs

@@ -361,20 +361,31 @@ pub struct GenericArrayImplOdd<T, U> {
     data: T,
 }
 
+// NOTE: These `Clone` impls are intentionally never reached in normal use:
+// `GenericArray<T, N>::clone` delegates to `self.map(Clone::clone)`, so the recursive
+// container's own `Clone` is never invoked. Bodied as `unreachable!()` (rather than the
+// recursive clone) to avoid emitting the recursive-clone codegen that would otherwise be
+// dead. The `GenericArrayImpl*` types are `#[doc(hidden)]` internals; they must remain
+// nameable via `<N as ArrayLength>::ArrayType<T>` for `typenum` reasons, so a caller can
+// technically construct one and call `.clone()` on it. That misuse now panics
+// deterministically instead of hitting `unreachable_unchecked()` (UB).
 impl<T: Clone, U: Clone> Clone for GenericArrayImplEven<T, U> {
     #[inline(always)]
     fn clone(&self) -> GenericArrayImplEven<T, U> {
-        // Clone is never called on the GenericArrayImpl types,
-        // as we use `self.map(clone)` elsewhere. This helps avoid
-        // extra codegen for recursive clones when they are never used.
-        unsafe { core::hint::unreachable_unchecked() }
+        unreachable!(
+            "GenericArrayImplEven::clone should never be called; \
+             clone a GenericArray<T, N> instead of its internal ArrayType<T>"
+        )
     }
 }
 
 impl<T: Clone, U: Clone> Clone for GenericArrayImplOdd<T, U> {
     #[inline(always)]
     fn clone(&self) -> GenericArrayImplOdd<T, U> {
-        unsafe { core::hint::unreachable_unchecked() }
+        unreachable!(
+            "GenericArrayImplOdd::clone should never be called; \
+             clone a GenericArray<T, N> instead of its internal ArrayType<T>"
+        )
     }
 }
 

tests/alloc.rs

@@ -430,3 +430,96 @@ fn test_length_error_display() {
     let msg = alloc::format!("{}", LengthError);
     assert!(msg.contains("LengthError"));
 }
+
+// Drop-counting element for the panic-unwind tests below.
+#[derive(Clone)]
+struct Tracked<'a>(i32, &'a Cell<u32>);
+
+impl Drop for Tracked<'_> {
+    fn drop(&mut self) {
+        self.1.set(self.1.get() + 1);
+    }
+}
+
+// The Gemini audit's finding #2 worries that the by-value consumer paths
+// (`ArrayConsumer` / `IntrusiveArrayConsumer`, used by `map`/`zip`) leave moved-out
+// memory in a state that is unsound to touch. The real soundness contract those Drop
+// impls uphold is *unwind safety*: if the user closure panics partway through, every
+// element created is dropped exactly once - the elements already moved into the closure
+// are dropped there, and the still-unconsumed tail is dropped by the consumer's Drop.
+// Never zero, never twice. These tests assert "drops == elements created" after catching
+// a mid-iteration panic. Living in `alloc.rs` (rather than the `#![no_std]` `mod.rs`)
+// because `catch_unwind` needs `std`.
+//
+// Worth running under Miri, both aliasing models, to also rule out a double-free or
+// aliasing violation on the unwind path:
+//   cargo +nightly miri test --test alloc panic_unwind
+//   MIRIFLAGS="-Zmiri-tree-borrows" cargo +nightly miri test --test alloc panic_unwind
+#[test]
+fn panic_unwind_map_drops_all_once() {
+    extern crate std;
+    use core::panic::AssertUnwindSafe;
+
+    // `map` consumes `self` via the consumer. Panicking while handling element 2 means
+    // elements 0 and 1 were moved into (and dropped by) the closure, while 2 and 3 remain
+    // for the consumer's Drop. All 4 must be dropped, none twice.
+    let counter = Cell::new(0u32);
+    let result = std::panic::catch_unwind(AssertUnwindSafe(|| {
+        let a: GenericArray<Tracked, U4> = GenericArray::generate(|i| Tracked(i as i32, &counter));
+        let _mapped: GenericArray<i32, U4> = a.map(|x| {
+            if x.0 == 2 {
+                panic!("boom in map");
+            }
+            x.0
+        });
+    }));
+    assert!(result.is_err(), "closure should have panicked");
+    assert_eq!(counter.get(), 4, "every created element dropped exactly once");
+}
+
+#[test]
+fn panic_unwind_zip_drops_all_once() {
+    extern crate std;
+    use core::panic::AssertUnwindSafe;
+
+    // `zip` drives the two-array consumer path (inverted_zip / inverted_zip2). Panicking
+    // mid-iteration must still account for all 8 created elements (4 per array) exactly once.
+    let counter = Cell::new(0u32);
+    let result = std::panic::catch_unwind(AssertUnwindSafe(|| {
+        let a: GenericArray<Tracked, U4> = GenericArray::generate(|i| Tracked(i as i32, &counter));
+        let b: GenericArray<Tracked, U4> =
+            GenericArray::generate(|i| Tracked(i as i32 * 10, &counter));
+        let _summed: GenericArray<i32, U4> = a.zip(b, |x, y| {
+            if x.0 == 2 {
+                panic!("boom in zip");
+            }
+            x.0 + y.0
+        });
+    }));
+    assert!(result.is_err(), "closure should have panicked");
+    assert_eq!(counter.get(), 8, "every created element dropped exactly once");
+}
+
+#[test]
+fn panic_unwind_from_iter_drops_all_once() {
+    extern crate std;
+    use core::panic::AssertUnwindSafe;
+
+    // `FromIterator` uses the builder (not the consumer), but the same exactly-once
+    // accounting must hold if the *source iterator* panics partway: the initialized prefix
+    // is dropped, the not-yet-written slots are not. The panic fires before element 3 is
+    // created, so exactly 3 drops occur.
+    let counter = Cell::new(0u32);
+    let result = std::panic::catch_unwind(AssertUnwindSafe(|| {
+        let _: GenericArray<Tracked, U4> = (0..4)
+            .map(|i| {
+                if i == 3 {
+                    panic!("boom in source iterator");
+                }
+                Tracked(i, &counter)
+            })
+            .collect();
+    }));
+    assert!(result.is_err(), "iterator should have panicked");
+    assert_eq!(counter.get(), 3, "initialized prefix dropped exactly once");
+}

tests/clone_unreachable_repro.rs

@@ -0,0 +1,31 @@
+//! Regression test for the Gemini audit's finding #1.
+//!
+//! The internal `GenericArrayImpl*` containers are `#[doc(hidden)]` but must remain
+//! nameable via `<N as ArrayLength>::ArrayType<T>` (a `typenum` limitation). They carry
+//! a safe `Clone` impl whose body is unreachable in normal use, because
+//! `GenericArray<T, N>::clone` delegates to `self.map(Clone::clone)` and never invokes
+//! the container's own `Clone`.
+//!
+//! Previously that body was `unreachable_unchecked()`, so a caller who explicitly named
+//! the internal type and called `.clone()` on it hit UB from 100% safe code (reachable
+//! via the `const-default` feature's `ConstDefault::DEFAULT`). It is now `unreachable!()`,
+//! turning that misuse into a deterministic panic instead. This test pins that behavior.
+//!
+//! Requires `const-default` for a fully-safe way to construct the internal type.
+#![cfg(feature = "const-default")]
+
+use const_default::ConstDefault;
+use generic_array::ArrayLength;
+
+#[test]
+#[should_panic(expected = "should never be called")]
+fn cloning_internal_array_type_panics_not_ub() {
+    type Inner = <typenum::U2 as ArrayLength>::ArrayType<i32>;
+
+    // Fully safe construction of an internal container type.
+    let arr = <Inner as ConstDefault>::DEFAULT;
+
+    // Fully safe call into the never-meant-to-be-called Clone impl. Must panic
+    // deterministically (was UB via unreachable_unchecked before the fix).
+    let _cloned = arr.clone();
+}

tests/iter.rs

@@ -213,6 +213,151 @@ fn test_into_iter_drops() {
     assert_eq!(i.get(), 5);
 }
 
+// Targeted reproduction attempt for the Gemini audit's "slice fabrication over
+// partially-moved arrays" finding. The claim: every `get_unchecked(index..index_back)`
+// in the iterator auto-derefs through the whole-array `slice::from_raw_parts` over
+// `0..N`, and once leading/trailing elements have been moved out, forming that
+// whole-array reference is claimed to be UB.
+//
+// To give that claim the strongest possible chance to fire under Miri, the element
+// type is `Niche(NonZeroU32)`: a moved-out slot left as a zeroed bit pattern would be
+// a *validity-invalid* `NonZeroU32`, not merely uninitialized - so a reference spanning
+// it would be UB that Tree Borrows + validity checking must catch. A `Cell`-backed Drop
+// counter additionally proves exactly-once drop accounting through each path.
+//
+// The helpers below exercise *every* slice-forming call site the report named
+// (`as_slice`, `as_mut_slice`, `next`, `nth`, `next_back`, `nth_back`, `fold`, `rfold`,
+// `Drop`, and `Clone`) while the backing array is in a partially-moved state.
+//
+// Run under both aliasing models to adjudicate:
+//   cargo +nightly miri test --test iter partial_move
+//   MIRIFLAGS="-Zmiri-tree-borrows" cargo +nightly miri test --test iter partial_move
+
+use std::num::NonZeroU32;
+
+struct Niche<'a>(NonZeroU32, &'a Cell<u32>);
+
+impl Clone for Niche<'_> {
+    fn clone(&self) -> Self {
+        Niche(self.0, self.1)
+    }
+}
+
+impl Drop for Niche<'_> {
+    fn drop(&mut self) {
+        // Touch the value so a moved-out/zeroed slot is observable as a validity bug,
+        // and count the drop so callers can assert exactly-once semantics.
+        assert!(self.0.get() != 0, "dropped a moved-out / invalid Niche");
+        self.1.set(self.1.get() + 1);
+    }
+}
+
+// Build a fresh 5-element iterator whose elements all count drops into `c`.
+fn mk_iter(c: &Cell<u32>) -> generic_array::GenericArrayIter<Niche<'_>, U5> {
+    GenericArray::<Niche, U5>::from_iter(
+        (1..=5).map(|n| Niche(NonZeroU32::new(n).unwrap(), c)),
+    )
+    .into_iter()
+}
+
+#[test]
+fn test_partial_move_as_slice_both_ends() {
+    let c = Cell::new(0);
+    {
+        let mut iter = mk_iter(&c);
+        let _front = iter.next().unwrap(); // index advances; slot 0 moved out
+        let _back = iter.next_back().unwrap(); // index_back retreats; slot 4 moved out
+        // as_slice / as_mut_slice now form the whole-array ref with slots 0 and 4 dead.
+        assert_eq!(iter.as_slice().len(), 3);
+        for n in iter.as_mut_slice() {
+            assert!(n.0.get() != 0);
+        }
+        // Debug also routes through as_slice().
+        let _ = format!("{:?} {:?}", iter.as_slice().len(), c.get());
+        drop(iter); // Drop forms the remaining [1..4] slice and drop_in_place's it.
+    }
+    assert_eq!(c.get(), 5, "all 5 elements dropped exactly once");
+}
+
+#[test]
+fn test_partial_move_drain_to_empty() {
+    // Every slot moved out before Drop: Drop must form a zero-length slice, not touch
+    // any of the (now invalid) backing memory.
+    let c = Cell::new(0);
+    {
+        let mut iter = mk_iter(&c);
+        while iter.next().is_some() {}
+        assert_eq!(iter.as_slice().len(), 0);
+        drop(iter);
+    }
+    assert_eq!(c.get(), 5);
+}
+
+#[test]
+fn test_partial_move_nth_and_nth_back() {
+    // nth() drop_in_place's the skipped prefix slice, then next() reads through the
+    // whole-array deref; nth_back() does the mirror on the suffix.
+    let c = Cell::new(0);
+    {
+        let mut iter = mk_iter(&c);
+        let _ = iter.nth(1).unwrap(); // drops slots [0..1], returns slot 1
+        let _ = iter.nth_back(1).unwrap(); // drops slots [4..5)->[3..4], returns slot 3
+        assert_eq!(iter.as_slice().len(), 1); // only slot 2 remains live
+        drop(iter);
+    }
+    assert_eq!(c.get(), 5);
+}
+
+#[test]
+fn test_partial_move_fold_after_consume() {
+    // fold() forms get_unchecked(index..index_back) and ptr::reads through it while
+    // mutating the index, with both outer ends already moved out.
+    let c = Cell::new(0);
+    {
+        let mut iter = mk_iter(&c);
+        let _ = iter.next().unwrap();
+        let _ = iter.next_back().unwrap();
+        let sum = iter.fold(0u32, |acc, n| acc + n.0.get());
+        assert_eq!(sum, 2 + 3 + 4);
+    }
+    assert_eq!(c.get(), 5);
+}
+
+#[test]
+fn test_partial_move_rfold_after_consume() {
+    let c = Cell::new(0);
+    {
+        let mut iter = mk_iter(&c);
+        let _ = iter.next().unwrap();
+        let _ = iter.next_back().unwrap();
+        let sum = iter.rfold(0u32, |acc, n| acc + n.0.get());
+        assert_eq!(sum, 2 + 3 + 4);
+    }
+    assert_eq!(c.get(), 5);
+}
+
+#[test]
+fn test_partial_move_clone_after_consume() {
+    // Clone is the spiciest path: it ptr::read's the *entire* partially-moved backing
+    // array (bitwise) into a new iter, then writes clones into the live prefix via
+    // as_mut_slice(). If forming a ref over moved-out slots were UB, this is where it
+    // would bite hardest.
+    let c = Cell::new(0);
+    {
+        let mut iter = mk_iter(&c);
+        let _ = iter.next().unwrap(); // slot 0 dead
+        let _ = iter.next_back().unwrap(); // slot 4 dead
+        let cloned = iter.clone(); // bitwise-copies [_, 2, 3, 4, _], clones live 2,3,4
+        assert_eq!(cloned.as_slice().len(), 3);
+        // Iterator::map (lazy) over the cloned by-value iter, consuming the 3 clones.
+        let s: u32 = cloned.map(|n| n.0.get()).sum();
+        assert_eq!(s, 2 + 3 + 4);
+        drop(iter);
+    }
+    // 5 originals + 3 clones = 8 drops.
+    assert_eq!(c.get(), 8);
+}
+
 #[test]
 fn test_from_failing_iter() {
     let res: Result<GenericArray<_, U5>, ()> = GenericArray::from_fallible_iter(