From 605785cf90e7bbb47a6e34863cf18d4814f4b839 Mon Sep 17 00:00:00 2001 From: caiyuanhao Date: Wed, 15 Dec 2021 21:45:17 +0800 Subject: [PATCH 1/2] Consume buf only if header name and value are both decoded Decoding error when processing continuation header which contains normal header name at boundary --- src/hpack/decoder.rs | 86 ++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 79 insertions(+), 7 deletions(-) diff --git a/src/hpack/decoder.rs b/src/hpack/decoder.rs index e4b34d1f..b2d20f00 100644 --- a/src/hpack/decoder.rs +++ b/src/hpack/decoder.rs @@ -279,10 +279,13 @@ impl Decoder { // First, read the header name if table_idx == 0 { + let old_pos = buf.position(); + let name_marker = self.try_decode_string(buf)?; + let value_marker = self.try_decode_string(buf)?; + buf.set_position(old_pos); // Read the name as a literal - let name = self.decode_string(buf)?; - let value = self.decode_string(buf)?; - + let name = take_string_marker(buf, name_marker); + let value = take_string_marker(buf, value_marker); Header::new(name, value) } else { let e = self.table.get(table_idx)?; @@ -292,7 +295,11 @@ impl Decoder { } } - fn decode_string(&mut self, buf: &mut Cursor<&mut BytesMut>) -> Result { + fn try_decode_string( + &mut self, + buf: &mut Cursor<&mut BytesMut>, + ) -> Result<((usize, usize), Option), DecoderError> { + let old_pos = buf.position(); const HUFF_FLAG: u8 = 0b1000_0000; // The first bit in the first byte contains the huffman encoded flag. @@ -309,17 +316,27 @@ impl Decoder { return Err(DecoderError::NeedMore(NeedMore::StringUnderflow)); } + let offset = (buf.position() - old_pos) as usize; if huff { let ret = { let raw = &buf.chunk()[..len]; - huffman::decode(raw, &mut self.buffer).map(BytesMut::freeze) + huffman::decode(raw, &mut self.buffer) + .map(|buf| ((offset, len), Some(BytesMut::freeze(buf)))) }; buf.advance(len); - return ret; + ret + } else { + buf.advance(len); + Ok(((offset, len), None)) } + } - Ok(take(buf, len)) + fn decode_string(&mut self, buf: &mut Cursor<&mut BytesMut>) -> Result { + let old_pos = buf.position(); + let marker = self.try_decode_string(buf)?; + buf.set_position(old_pos); + Ok(take_string_marker(buf, marker)) } } @@ -433,6 +450,21 @@ fn take(buf: &mut Cursor<&mut BytesMut>, n: usize) -> Bytes { head.freeze() } +fn take_string_marker( + buf: &mut Cursor<&mut BytesMut>, + marker: ((usize, usize), Option), +) -> Bytes { + let ((offset, len), string) = marker; + buf.advance(offset); + match string { + Some(string) => { + buf.advance(len); + string + } + None => take(buf, len), + } +} + fn consume(buf: &mut Cursor<&mut BytesMut>) { // remove bytes from the internal BytesMut when they have been successfully // decoded. This is a more permanent cursor position, which will be @@ -850,4 +882,44 @@ mod test { huffman::encode(src, &mut buf); buf } + + #[test] + fn test_decode_continuation_header() { + let mut de = Decoder::new(0); + let value = huff_encode(b"bar"); + let mut buf = BytesMut::new(); + buf.extend(&[0b01000000, 0x00 | 3]); + buf.extend(b"foo"); + buf.extend(&[0x80 | 3]); + buf.extend(&value[0..1]); + + let mut res = vec![]; + let e = de + .decode(&mut Cursor::new(&mut buf), |h| { + res.push(h); + }) + .unwrap_err(); + assert_eq!(e, DecoderError::NeedMore(NeedMore::StringUnderflow)); + buf.extend(&value[1..]); + + let _ = de + .decode(&mut Cursor::new(&mut buf), |h| { + res.push(h); + }) + .unwrap(); + + assert_eq!(res.len(), 1); + assert_eq!(de.table.size(), 0); + + match res[0] { + Header::Field { + ref name, + ref value, + } => { + assert_eq!(name, "foo"); + assert_eq!(value, "bar"); + } + _ => panic!(), + } + } } From e0e47c517b8b4365679b1b3338f0c16bcde1268e Mon Sep 17 00:00:00 2001 From: caiyuanhao Date: Wed, 9 Feb 2022 14:44:01 +0800 Subject: [PATCH 2/2] Use struct to represent the context of string decoding --- src/hpack/decoder.rs | 55 ++++++++++++++++++++++++++++---------------- 1 file changed, 35 insertions(+), 20 deletions(-) diff --git a/src/hpack/decoder.rs b/src/hpack/decoder.rs index b2d20f00..988b48db 100644 --- a/src/hpack/decoder.rs +++ b/src/hpack/decoder.rs @@ -142,6 +142,12 @@ struct Table { max_size: usize, } +struct StringMarker { + offset: usize, + len: usize, + string: Option, +} + // ===== impl Decoder ===== impl Decoder { @@ -284,8 +290,8 @@ impl Decoder { let value_marker = self.try_decode_string(buf)?; buf.set_position(old_pos); // Read the name as a literal - let name = take_string_marker(buf, name_marker); - let value = take_string_marker(buf, value_marker); + let name = name_marker.consume(buf); + let value = value_marker.consume(buf); Header::new(name, value) } else { let e = self.table.get(table_idx)?; @@ -298,7 +304,7 @@ impl Decoder { fn try_decode_string( &mut self, buf: &mut Cursor<&mut BytesMut>, - ) -> Result<((usize, usize), Option), DecoderError> { + ) -> Result { let old_pos = buf.position(); const HUFF_FLAG: u8 = 0b1000_0000; @@ -320,15 +326,22 @@ impl Decoder { if huff { let ret = { let raw = &buf.chunk()[..len]; - huffman::decode(raw, &mut self.buffer) - .map(|buf| ((offset, len), Some(BytesMut::freeze(buf)))) + huffman::decode(raw, &mut self.buffer).map(|buf| StringMarker { + offset, + len, + string: Some(BytesMut::freeze(buf)), + }) }; buf.advance(len); ret } else { buf.advance(len); - Ok(((offset, len), None)) + Ok(StringMarker { + offset, + len, + string: None, + }) } } @@ -336,7 +349,7 @@ impl Decoder { let old_pos = buf.position(); let marker = self.try_decode_string(buf)?; buf.set_position(old_pos); - Ok(take_string_marker(buf, marker)) + Ok(marker.consume(buf)) } } @@ -450,18 +463,16 @@ fn take(buf: &mut Cursor<&mut BytesMut>, n: usize) -> Bytes { head.freeze() } -fn take_string_marker( - buf: &mut Cursor<&mut BytesMut>, - marker: ((usize, usize), Option), -) -> Bytes { - let ((offset, len), string) = marker; - buf.advance(offset); - match string { - Some(string) => { - buf.advance(len); - string +impl StringMarker { + fn consume(self, buf: &mut Cursor<&mut BytesMut>) -> Bytes { + buf.advance(self.offset); + match self.string { + Some(string) => { + buf.advance(self.len); + string + } + None => take(buf, self.len), } - None => take(buf, len), } } @@ -884,12 +895,14 @@ mod test { } #[test] - fn test_decode_continuation_header() { + fn test_decode_continuation_header_with_non_huff_encoded_name() { let mut de = Decoder::new(0); let value = huff_encode(b"bar"); let mut buf = BytesMut::new(); + // header name is non_huff encoded buf.extend(&[0b01000000, 0x00 | 3]); buf.extend(b"foo"); + // header value is partial buf.extend(&[0x80 | 3]); buf.extend(&value[0..1]); @@ -899,9 +912,11 @@ mod test { res.push(h); }) .unwrap_err(); + // decode error because the header value is partial assert_eq!(e, DecoderError::NeedMore(NeedMore::StringUnderflow)); - buf.extend(&value[1..]); + // extend buf with the remaining header value + buf.extend(&value[1..]); let _ = de .decode(&mut Cursor::new(&mut buf), |h| { res.push(h);