safety-dance
safety-dance copied to clipboard
rust-secure-code
Audit httparse crate
https://crates.io/crates/httparse
HTTP protocol parser. 10,000 downloads/day. Used in reqwest among others, exposed to untrusted input from the network.
Contains unsafe code, some of it is for SIMD but there is some that should be possible to eliminate.
There are trivial violations of validity invariants in private functions, so it warrants a closer look. Example: https://github.com/seanmonstar/httparse/issues/58
I've looked at the code and found the following:
All unsafe in src/lib.rs is about str::from_utf8_unchecked() and self-implemented methods .bump() and .slice_skip().
Usage of the methods is safe: they bump/skip only previously checked chars (\n and \r).
I've looked on implementation in src/iter.rs. .bump() .advance() looks Ok, but I'm not sure, if debug_assert! should be changed to assert!.
The .slice_skip() looks a bit cryptic to me, so no comments.)
I created a fork of httparse which uses a lot less unsafe.
No pull request because benchmarking shows that my safe implementation of slice_skip is slower than the original implementation (although nothing outside of slice_skip that I changed has any negative performance impact).
All uses of unsafe except for shrink in lib.rs and the stuff in the SIMD module can be fairly easily implemented without using unsafe.
The
.slice_skip()looks a bit cryptic to me, so no comments.)
slice_skip resets self.slice to start at the index referred to by self.pos and sets self.pos to 0 so that self.slice[self.pos] refers to the same byte after calling slice_skip that it did before. It then returns a slice containing all but the last skip bytes from the section that was cut off of self.slice. So it is safe as long as skip is less than or equal to self.slice.len() and makes sense as long as skip is less than or equal to self.pos (Hence starting the method with debug_assert!(self.pos >= skip);)
Pull requests making things safer (especially when performance stays the same!) are always welcome.
