seanmonstar / httparse

A push parser for the HTTP 1.x protocol in Rust.
https://docs.rs/httparse
Apache License 2.0
573 stars 113 forks source link

Private function shrink() is unsound #58

Open Shnatsel opened 5 years ago

Shnatsel commented 5 years ago

This code does not uphold Rust safety invariants: either debug_assert!() should be assert!() or the function must be marked unsafe fn:

https://github.com/seanmonstar/httparse/blob/6f696f5d027f35e11a70181c839b574e20335a74/src/lib.rs#L38-L43

Also, it's weird to see a custom function for this - slice[..len] looks like it should be sufficient, but I'm probably missing something.

seanmonstar commented 5 years ago

Good call, the function should be unsafe for sure.

It's been so long, I can't remember why I wrote like this instead of with slice syntax. Do the lifetimes work out? It might have been to not need the bounds check, but I can't remember if that showed in the benchmarks.

cuviper commented 4 years ago

You can't directly re-slice it, but you can disconnect the lifetimes with mem::replace like:

#[inline]
fn shrink<T>(slice: &mut &mut [T], len: usize) {
    let full = mem::replace(slice, &mut []);
    *slice = &mut full[..len];
}

Note that this will be bounds-checked, if that matters for your performance, and if it does panic the original will be left with the empty slice. I guess you could pre-assert the length to avoid that, and then hope that LLVM will elide the later check.