Failing to drop HeaderMap::Drain causes double-free

[Qwaz]

http/src/header/map.rs

Lines 2115 to 2122 in 9c05e39

	unsafe {
	let entry = &(*self.map).entries[idx];
	// Read the header name
	key = ptr::read(&entry.key as *const _);
	value = ptr::read(&entry.value as *const _);
	next = entry.links.map(|l| l.next);
	};

http/src/header/map.rs

Lines 2140 to 2148 in 9c05e39

	impl<'a, T> Drop for Drain<'a, T> {
	fn drop(&mut self) {
	unsafe {
	let map = &mut *self.map;
	debug_assert!(map.extra_values.is_empty());
	map.entries.set_len(0);
	}
	}
	}

Failing to drop a value is considered safe in Rust, and unsafe code should not rely on this behavior.

It is reasonable for safe code to assume that destructor leaks do not happen, as any program that leaks destructors is probably wrong. However unsafe code cannot rely on destructors to be run in order to be safe.

HeaderMap::Drain uses ptr::read to move out entries from the map when it iterates, and calls map.entries.set_len(0) to clear the map at once when it is dropped. If Drain's drop is not called, double-free happens when HeaderMap is dropped. Also, if Drain is dropped without iterating to the end, it leaks memory.

Demonstration

[Shnatsel]

Double free is an exploitable security vulnerability. After releasing a fixed version please file a security advisory and, if possible, yank the affected versions from crates.io.

[hawkw]

I started attempting to fix this using a similar approach as Vec::drain, setting the length of self.entries to 0 when creating the Drain iterator, and then setting the length back if the Drain iterator is dropped before draining all elements. This way, mem::forgetting the iterator would only amplify a leak, rather than calling a double-free.

However, this isn't trivially possible here: the ValueDrain iterator calls HeaderMap::remove_extra_values, which relies on indexing the entries vector. If we've already set it's length to 0, the bounds checks there will fail. So, either that method needs to be refactored to not rely on indexing, or we need a different solution here (possibly involving ManuallyDrop?). I'll keep working on it.