Soundness: Double-Drop of ZSTs implementing Drop in extend_from_iter

[Manishearth]

Note

This finding was identified during an agentic unsafe Rust code review performed by Gemini AI, followed by human review and verification.

The Issue

In src/arrayvec.rs, ArrayVec::extend_from_iter (called by Extend::extend and FromIterator::from_iter) copies elements from an iterator into the uninitialized buffer of the vector:

arrayvec/src/arrayvec.rs

Line 1103 in 0aede87

if mem::size_of::<T>() != 0 {

if mem::size_of::<T>() != 0 {
    ptr.write(elt);
}

If T is a ZST (mem::size_of::<T>() == 0), writing elt to ptr is conditionally skipped.

Skipping ptr.write(elt) means the local variable elt (yielded by the iterator) is never moved into the buffer. Consequently, elt remains in local scope and is automatically dropped by the compiler at the end of the loop iteration.

Meanwhile, the loop unconditionally increments guard.data (which updates self.len on function exit). Thus, the vector's length is incremented, indicating that the vector now owns the elements.

When the ArrayVec itself is subsequently dropped or cleared, its destructor drops elements in the range 0..self.len(), invoking Drop::drop on the ZST elements a second time. For any ZST implementing Drop, this results in an immediate double-drop (Undefined Behavior).

Any safe caller can trigger this Undefined Behavior simply by calling .extend() or .collect() on an ArrayVec with an iterator yielding ZSTs that implement Drop.

Minimal Reproduction

use arrayvec::ArrayVec;
use std::sync::atomic::{AtomicUsize, Ordering};

static DROP_COUNT: AtomicUsize = AtomicUsize::new(0);

struct ZstWithDrop;

impl Drop for ZstWithDrop {
    fn drop(&mut self) {
        DROP_COUNT.fetch_add(1, Ordering::SeqCst);
    }
}

fn main() {
    {
        let mut vec: ArrayVec<ZstWithDrop, 4> = ArrayVec::new();
        // Extend vector with 2 ZST items implementing Drop.
        vec.extend(std::iter::repeat_with(|| ZstWithDrop).take(2));

        // Inside extend_from_iter, `ptr.write(elt)` is skipped for ZSTs.
        // Thus, `elt` drops immediately at the end of each loop iteration (2 drops).
        // However, vector length is still incremented to 2.
    }
    // When `vec` goes out of scope, it drops the 2 elements in `0..vec.len()`,
    // dropping the ZST instances a second time (2 more drops).
    // Total drops = 4 for only 2 constructed items (double drop / Undefined Behavior).
    assert_eq!(DROP_COUNT.load(Ordering::SeqCst), 4);
    println!("Double drop detected! ZST elements dropped {} times (expected 4)", DROP_COUNT.load(Ordering::SeqCst));
}

Double drop detected! ZST elements dropped 4 times (expected 4)

---

Note

The full audit report below also contains additional minor findings (such as missing safety comments or undocumented FFI assumptions) that are probably worth fixing as well but not the primary goal of this issue.

Full Gemini Unsafe Code Audit Report

Unsafe Rust Review: arrayvec (v0_7)

Overall Safety Assessment

The arrayvec crate is a fundamental and widely used Rust library providing stack-allocated vectors and strings (ArrayVec and ArrayString). Because it manages uninitialized memory and manually controls length, element creation, and destruction, it relies heavily on unsafe Rust.

While the unsafe code is generally structured carefully and correctly handles panic safety in complex destructors like Drain and retain, there is a critical vulnerability regarding the handling of Zero-Sized Types (ZSTs) in the iterator extension code.

Furthermore, the crate has a severe deficiency in safety documentation. Almost none of the unsafe blocks are accompanied by // SAFETY: comments explaining why the operations are safe, and several public and internal unsafe functions lack # Safety documentation. This makes the crate difficult to audit and maintain without risk of introducing soundness bugs.

Critical Findings

1. Double-Drop of Zero-Sized Types in ArrayVec::extend_from_iter / extend / FromIterator 🔴 🤦

• Severity: 🔴 High
• Threat Vector: 🤦 Accidental Misuse
• Bug Type: Double Drop

• File: src/arrayvec.rs

• Function: ArrayVec::extend_from_iter (called by Extend::extend and
  FromIterator::from_iter)

• Code Location: src/arrayvec.rs:1103

• Vulnerability: In extend_from_iter, when copying elements from the
  iterator into the vector's buffer, the code checks:

  if mem::size_of::<T>() != 0 {
      ptr.write(elt);
  }

If T is a Zero-Sized Type (ZST) (i.e. mem::size_of::<T>() == 0), the write to the pointer is skipped.

Skipping ptr.write(elt) means the local variable elt (yielded by the iterator) is not moved. Consequently, elt goes out of scope at the end of the loop iteration, and the compiler automatically runs its Drop implementation.

At the same time, the loop increments guard.data (which is written back to self.len on function exit). Thus, the vector's length is still incremented, indicating that the vector now owns the elements.

When the ArrayVec itself is dropped, it will drop the elements in 0..len, dropping the ZST elements a second time. This leads to a double-drop of ZST elements that implement Drop, which is undefined behavior.

• Impact: A safe caller can trigger undefined behavior (double-drop) by
  calling .extend() or .collect() on an ArrayVec with an iterator yielding ZSTs that implement Drop.

• Remediation: Always call ptr.write(elt) for all types, including ZSTs.
  ptr::write on a ZST pointer is safe and compiles to a no-op, while properly moving the value so it is not dropped at the end of the iteration.

  - if mem::size_of::<T>() != 0 {
  -     ptr.write(elt);
  - }
  + ptr.write(elt);
  

Fishy Findings

None.

Missing Safety Comments

Almost all unsafe blocks and unsafe functions in this crate lack safety comments and safety documentation. The following is a list of the most critical locations that require documentation:

1. char::encode_utf8 (src/char.rs:32) 🟡

   • API Type: pub unsafe fn

   • Preconditions: The # Safety section must document:

     • ptr must be non-null, aligned for u8 (which is always true), and
       valid for writes of len bytes.
     • The allocation pointed to by ptr must remain live for the duration
       of the write.

   • Proposed Safety Comment:

     /// # Safety
     ///
     /// The caller must ensure that `ptr` is non-null and valid for writes of at least
     /// `len` bytes, and that the pointed-to allocation remains live.
     

2. ArrayVec::push_unchecked (src/arrayvec.rs:230 and
   ArrayVecImpl::push_unchecked in src/arrayvec_impl.rs:54) 🟡

   • API Type: pub unsafe fn

   • Preconditions: The # Safety section must document:

     • The caller must ensure that the vector's length is strictly less
       than its capacity (self.len() < self.capacity()).

   • Proposed Safety Comment:

     /// # Safety
     ///
     /// The caller must ensure that the vector's current length is strictly less than
     /// its capacity (`self.len() < self.capacity()`). Calling this on a full vector
     /// results in an out-of-bounds write and undefined behavior.
     

3. ArrayVec::set_len (src/arrayvec.rs:545) 🟠

   • API Type: pub unsafe fn

   • Preconditions: The # Safety section must document:

     • length must be less than or equal to the capacity of the vector.
     • The first length elements of the backing array must be
       initialized.

   • Proposed Safety Comment:

     /// # Safety
     ///
     /// The caller must ensure that:
     /// 1. `length <= self.capacity()`.
     /// 2. The elements in the range `0..length` are fully initialized values of type `T`.
     

4. ArrayVec::into_inner_unchecked (src/arrayvec.rs:669) 🟡

   • API Type: pub unsafe fn

   • Preconditions:

     • The vector must be full (self.len() == self.capacity()).

   • Proposed Safety Comment:

     /// # Safety
     ///
     /// The caller must ensure that the vector is fully filled, i.e., `self.len() == self.capacity()`.
     /// If the vector is not full, this will read uninitialized memory as initialized values of type `T`,
     /// leading to undefined behavior.
     

5. ArrayString::set_len (src/array_string.rs:404) 🟠

   • API Type: pub unsafe fn

   • Preconditions:

     • length <= self.capacity().
     • The first length bytes of the buffer must be initialized.
     • The initialized bytes must form a valid UTF-8 string.

   • Proposed Safety Comment:

     /// # Safety
     ///
     /// The caller must ensure that:
     /// 1. `length <= self.capacity()`.
     /// 2. The bytes in range `0..length` are initialized.
     /// 3. The bytes in range `0..length` constitute a valid UTF-8 string.
     

6. ArrayVec and ArrayString Unsafe Blocks: 🔴

   • None of the unsafe blocks (e.g., in try_insert, retain,
     IntoIter::next, Drain::drop, From<[T; CAP]>, and Deref/DerefMut for ArrayString) contain // SAFETY: comments.
   • All these blocks should be documented with local proofs explaining how standard library requirements (for ptr::copy, ptr::copy_nonoverlapping, ptr::write, ptr::read, and slice::from_raw_parts_mut) are satisfied.

[bluss]

I think that's fixed by #308 which is overdue release. Thanks for the report.

[Manishearth]

Ah, sorry. I spot checked open issues but not PRs.

[Manishearth]

Fixed by #308