use newest hpack index when repeating a header name

src/hpack/decoder.rs

@@ -572,7 +572,6 @@ impl From<DecoderError> 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 {

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(|| {
+                    let res = self.encode_header_without_name(
-                        panic!("encoding header without name, but not previous index to use for name");
+                        last_index.as_ref().unwrap_or_else(|| {
-                    });
+                            panic!("encoding header without name, but no previous index to use for name");
-                    let res = self.encode_header_without_name(index, &value, dst);
+                        }),
+                        &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<Option<HeaderName>> {
-        use http::header::{HeaderName, HeaderValue};
-
         let name = HeaderName::from_bytes(name.as_bytes()).unwrap();
         let value = HeaderValue::from_bytes(val.as_bytes()).unwrap();
 

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<usize>,
+    ) -> 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)
+            };
         }
     }
 

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| {
+                            .decode(&mut Cursor::new(&mut buf), |h| {
-                                assert_eq!(e, expect.remove(0).reify().unwrap());
+                                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| {
+                .decode(&mut Cursor::new(&mut buf), |h| {
-                    assert_eq!(e, expect.remove(0).reify().unwrap());
+                    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<Option<HeaderName>> {
             _ => 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<Option<HeaderName>> {
         }
 
         Header::Field {
-            name: Some(name),
+            name,
-            value: value,
+            value,
         }
     }
 }