diff --git a/src/hpack/decoder.rs b/src/hpack/decoder.rs index 58ec7c7..c879b69 100644 --- a/src/hpack/decoder.rs +++ b/src/hpack/decoder.rs @@ -572,7 +572,6 @@ impl From for frame::Error { /// Get an entry from the static table pub fn get_static(idx: usize) -> Header { - use http::header; use http::header::HeaderValue; match idx { diff --git a/src/hpack/encoder.rs b/src/hpack/encoder.rs index d6e8e12..c61fcf1 100644 --- a/src/hpack/encoder.rs +++ b/src/hpack/encoder.rs @@ -133,10 +133,13 @@ impl Encoder { // which case, we skip table lookup and just use the same index // as the previous entry. Err(value) => { - let index = last_index.as_ref().unwrap_or_else(|| { - panic!("encoding header without name, but not previous index to use for name"); - }); - let res = self.encode_header_without_name(index, &value, dst); + let res = self.encode_header_without_name( + last_index.as_ref().unwrap_or_else(|| { + panic!("encoding header without name, but no previous index to use for name"); + }), + &value, + dst, + ); if res.is_err() { dst.truncate(len); @@ -446,6 +449,23 @@ mod test { assert_eq!(1, res.len()); } + #[test] + fn test_encode_indexed_name_literal_value() { + let mut encoder = Encoder::default(); + let res = encode(&mut encoder, vec![header("content-language", "foo")]); + + assert_eq!(res[0], 0b01000000 | 27); // Indexed name + assert_eq!(res[1], 0x80 | 2); // header value w/ huffman coding + + assert_eq!("foo", huff_decode(&res[2..4])); + + // Same name, new value should still use incremental + let res = encode(&mut encoder, vec![header("content-language", "bar")]); + assert_eq!(res[0], 0b01000000 | 27); // Indexed name + assert_eq!(res[1], 0x80 | 3); // header value w/ huffman coding + assert_eq!("bar", huff_decode(&res[2..5])); + } + #[test] fn test_repeated_headers_are_indexed() { let mut encoder = Encoder::default(); @@ -773,7 +793,7 @@ mod test { #[test] fn test_nameless_header_at_resume() { let mut encoder = Encoder::default(); - let mut dst = BytesMut::from(Vec::with_capacity(11)); + let mut dst = BytesMut::from(Vec::with_capacity(15)); let mut input = vec![ Header::Field { @@ -784,6 +804,10 @@ mod test { name: None, value: HeaderValue::from_bytes(b"zomg").unwrap(), }, + Header::Field { + name: None, + value: HeaderValue::from_bytes(b"sup").unwrap(), + }, ].into_iter(); let resume = match encoder.encode(None, &mut input, &mut dst) { @@ -800,12 +824,14 @@ mod test { match encoder.encode(Some(resume), &mut input, &mut dst) { Encode::Full => {}, - _ => panic!(), + unexpected => panic!("resume returned unexpected: {:?}", unexpected), } // Next is not indexed assert_eq!(&[15, 47, 0x80 | 3], &dst[0..3]); - assert_eq!("zomg", huff_decode(&dst[3..])); + assert_eq!("zomg", huff_decode(&dst[3..6])); + assert_eq!(&[15, 47, 0x80 | 3], &dst[6..9]); + assert_eq!("sup", huff_decode(&dst[9..])); } #[test] @@ -825,8 +851,6 @@ mod test { } fn header(name: &str, val: &str) -> Header> { - use http::header::{HeaderName, HeaderValue}; - let name = HeaderName::from_bytes(name.as_bytes()).unwrap(); let value = HeaderValue::from_bytes(val.as_bytes()).unwrap(); diff --git a/src/hpack/table.rs b/src/hpack/table.rs index 9fe446b..f8771b0 100644 --- a/src/hpack/table.rs +++ b/src/hpack/table.rs @@ -125,7 +125,7 @@ impl Table { Indexed(idx, ..) => idx, Name(idx, ..) => idx, Inserted(idx) => idx + DYN_OFFSET, - InsertedValue(idx, _) => idx, + InsertedValue(_name_idx, slot_idx) => slot_idx + DYN_OFFSET, NotIndexed(_) => panic!("cannot resolve index"), } } @@ -140,6 +140,10 @@ impl Table { // Right now, if this is true, the header name is always in the // static table. At some point in the future, this might not be true // and this logic will need to be updated. + debug_assert!( + statik.is_some(), + "skip_value_index requires a static name", + ); return Index::new(statik, header); } @@ -191,7 +195,7 @@ impl Table { return self.index_vacant(header, hash, dist, probe, statik); } else if pos.hash == hash && self.slots[slot_idx].header.name() == header.name() { // Matching name, check values - return self.index_occupied(header, hash, pos.index); + return self.index_occupied(header, hash, pos.index, statik.map(|(n, _)| n)); } } else { return self.index_vacant(header, hash, dist, probe, statik); @@ -201,7 +205,13 @@ impl Table { }); } - fn index_occupied(&mut self, header: Header, hash: HashValue, mut index: usize) -> Index { + fn index_occupied( + &mut self, + header: Header, + hash: HashValue, + mut index: usize, + statik: Option, + ) -> Index { debug_assert!(self.assert_valid_state("top")); // There already is a match for the given header name. Check if a value @@ -222,6 +232,8 @@ impl Table { } if header.is_sensitive() { + // Should we assert this? + // debug_assert!(statik.is_none()); return Index::Name(real_idx + DYN_OFFSET, header); } @@ -245,7 +257,12 @@ impl Table { // Even if the previous header was evicted, we can still reference // it when inserting the new one... - return Index::InsertedValue(real_idx + DYN_OFFSET, 0); + return if let Some(n) = statik { + // If name is in static table, use it instead + Index::InsertedValue(n, 0) + } else { + Index::InsertedValue(real_idx + DYN_OFFSET, 0) + }; } } diff --git a/src/hpack/test/fuzz.rs b/src/hpack/test/fuzz.rs index b5be1a8..6624abd 100644 --- a/src/hpack/test/fuzz.rs +++ b/src/hpack/test/fuzz.rs @@ -1,4 +1,5 @@ extern crate bytes; +extern crate env_logger; extern crate quickcheck; extern crate rand; @@ -16,6 +17,7 @@ const MAX_CHUNK: usize = 2 * 1024; #[test] fn hpack_fuzz() { + let _ = env_logger::try_init(); fn prop(fuzz: FuzzHpack) -> TestResult { fuzz.run(); TestResult::from_bool(true) @@ -88,14 +90,34 @@ impl FuzzHpack { _ => {}, } + let mut is_name_required = true; + for _ in 0..rng.gen_range(1, (num - added) + 1) { - added += 1; let x: f64 = rng.gen_range(0.0, 1.0); let x = x.powi(skew); let i = (x * source.len() as f64) as usize; - frame.headers.push(source[i].clone()); + + let header = &source[i]; + match header { + Header::Field { name: None, .. } => { + if is_name_required { + continue; + } + }, + Header::Field { .. } => { + is_name_required = false; + }, + _ => { + // pseudos can't be followed by a header with no name + is_name_required = true; + } + } + + frame.headers.push(header.clone()); + + added += 1; } frames.push(frame); @@ -125,10 +147,30 @@ impl FuzzHpack { let mut decoder = Decoder::default(); for frame in frames { - expect.extend(frame.headers.clone()); + // build "expected" frames, such that decoding headers always + // includes a name + let mut prev_name = None; + for header in &frame.headers { + match header.clone().reify() { + Ok(h) => { + prev_name = match h { + Header::Field { ref name, .. } => Some(name.clone()), + _ => None, + }; + expect.push(h); + }, + Err(value) => { + expect.push(Header::Field { + name: prev_name.as_ref().cloned().expect("previous header name"), + value, + }); + } + } + + } - let mut index = None; let mut input = frame.headers.into_iter(); + let mut index = None; let mut buf = BytesMut::with_capacity(chunks.pop().unwrap_or(MAX_CHUNK)); @@ -149,10 +191,11 @@ impl FuzzHpack { // Decode the chunk! decoder - .decode(&mut Cursor::new(&mut buf), |e| { - assert_eq!(e, expect.remove(0).reify().unwrap()); + .decode(&mut Cursor::new(&mut buf), |h| { + let e = expect.remove(0); + assert_eq!(h, e); }) - .unwrap(); + .expect("partial decode"); buf = BytesMut::with_capacity(chunks.pop().unwrap_or(MAX_CHUNK)); }, @@ -161,10 +204,11 @@ impl FuzzHpack { // Decode the chunk! decoder - .decode(&mut Cursor::new(&mut buf), |e| { - assert_eq!(e, expect.remove(0).reify().unwrap()); + .decode(&mut Cursor::new(&mut buf), |h| { + let e = expect.remove(0); + assert_eq!(h, e); }) - .unwrap(); + .expect("full decode"); } assert_eq!(0, expect.len()); @@ -232,7 +276,11 @@ fn gen_header(g: &mut StdRng) -> Header> { _ => unreachable!(), } } else { - let name = gen_header_name(g); + let name = if g.gen_weighted_bool(10) { + None + } else { + Some(gen_header_name(g)) + }; let mut value = gen_header_value(g); if g.gen_weighted_bool(30) { @@ -240,8 +288,8 @@ fn gen_header(g: &mut StdRng) -> Header> { } Header::Field { - name: Some(name), - value: value, + name, + value, } } }