seanmonstar / httparse

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

`parse_headers()` enables undefined behavior #34

Closed abonander closed 7 years ago

abonander commented 7 years ago

Simplest reproduction.

extern crate httparse;

use httparse::{EMPTY_HEADER, parse_headers};

fn main() {
    let mut buf = *b"Foo: Bar\r\n\r\n";

    let mut headers = [EMPTY_HEADER];

    let headers_len = {
        let (_, headers) = parse_headers(&mut buf, &mut headers).unwrap().unwrap();
        headers.len()
    } ;

    assert_eq!(headers_len, 1);

    buf[0] = b'B';

    // Prints "Boo"
    println!("{:?}", headers[0].name);
}

As you can see, parse_headers() allows borrows to buf to escape in headers, creating a double-borrow where the original buffer can be mutated while views to it exist.

Discovered by accident, I was working on some infinite-loop bugs in multipart when I took a double-take at this function and thought, "Wait a minute, how the hell did this work to begin with?" The r.consume() at 80 shouldn't be allowed, but the borrow is escaping.

seanmonstar commented 7 years ago

I believe this is caused by the implementation of the internal shrink function, which is just trying to shorten the reference it has to the headers slice. In my experimenting, it doesn't seem to be immediately obvious how to do so and make the borrow checker happy...

abonander commented 7 years ago

Holy crap, this may be Rust's failing unless I'm fundamentally misunderstanding something here: https://is.gd/pFdpPJ

abonander commented 7 years ago

So yeah, it's a bug with Rust, haha.

seanmonstar commented 7 years ago

@abonander Whew! I was genuinely staring at this last night, and wondering "but, I have lifetimes specified, how did I screw this up?!"

abonander commented 7 years ago

rust-lang/rust#40288 is closed now and any breakage was addressed via Crater run so I think this is safe to close now.