`GenericArrayIter::nth` is not exception safe

[Evian-Zhang]

Hello, we are security researchers targeting Rust security. By running our tool in this repository, we found a soundness issue. This report is written by 100% human. We promise that all you read will never be generated by LLM. Moreover, we are responsible, so if you confirm these issues do exist, we are happy to file PRs to fix it.

The problem happens here:

generic-array/src/iter.rs

Lines 204 to 218 in 31e9171

	fn nth(&mut self, n: usize) -> Option<T> {
	// First consume values prior to the nth.
	let next_index = self.index + cmp::min(n, self.len());
	// SAFETY: `index <= next_index <= index_back <= N`, so `index..next_index` is a
	// range of live, in-bounds elements. Drop exactly those in place via a raw slice
	// pointer (no reference spanning moved-out slots), then advance `index` past them.
	unsafe {
	ptr::drop_in_place(raw_subslice!(mut self.array, self.index, next_index));
	}
	self.index = next_index;
	self.next()
	}

The exception safety violation happens if T::drop() panics, then ptr::drop_in_place panics. Since self.index has not been updated, when unwinding, the destructor of GenericArrayIter will accordingly free self.array, leading to a Double-Free for those elements that should have been freed in ptr::drop_in_place.

Although there is a RFC said unwinding from Drop should be forbidden (rust-lang/rfcs#3288), but it still has some unresolved questions. And for now, we should refer to the std documentation of Drop::drop:

Note that even if this panics, the value is considered to be dropped; you must not cause drop to be called again. This is normally automatically handled by the compiler, but when using unsafe code, can sometimes occur unintentionally, particularly when using ptr::drop_in_place.

To fix it, I think we can just move self.index = next_index before the ptr::drop_in_place.

The same problem also happens in GenericArrayIter::nth_back:

generic-array/src/iter.rs

Lines 275 to 288 in 31e9171

	fn nth_back(&mut self, n: usize) -> Option<T> {
	let next_back = self.index_back - cmp::min(n, self.len());
	// SAFETY: `index <= next_back <= index_back <= N`, so `next_back..index_back` is a
	// range of live, in-bounds elements. Drop exactly those in place via a raw slice
	// pointer (no reference spanning moved-out slots), then retreat `index_back`.
	unsafe {
	ptr::drop_in_place(raw_subslice!(mut self.array, next_back, self.index_back));
	}
	self.index_back = next_back;
	self.next_back()
	}

[Evian-Zhang]

@novacrazy Thank you for fixing this, and sorry for disturbing you again. I wonder if it is OK that I file a RustSec advisory for this soundness issue, as there are already some exception-safety-related RustSec advisories (like 1, 2, 3). It's totally fine if you think it's not OK, and I will do nothing if you say so.

[novacrazy]

Personally, I don't think it's necessary, but I do tend to play a little looser than some. This is firmly in the realm of "probably hasn't happened this decade but an LLM pointed it out."

I'll put out a new release tonight so this is published, too.

[Evian-Zhang]

Thank you for your nice words! I totally understand, and I won't request a RustSec. Anyway, thank you!