Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Not to consume buf when decoding non_huff header name(which consumes the buf) succeeded but header value failed (DecoderError::NeedMore) #589

Merged
merged 2 commits into from Feb 24, 2022
Merged

Not to consume buf when decoding non_huff header name(which consumes the buf) succeeded but header value failed (DecoderError::NeedMore) #589

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
605785c
Consume buf only if header name and value are both decoded
Dec 15, 2021
e0e47c5
Use struct to represent the context of string decoding
Feb 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
86 changes: 79 additions & 7 deletions src/hpack/decoder.rs
View file
Open in desktop
Expand Up @@ -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)?;
Expand All @@ -292,7 +295,11 @@ impl Decoder {
}
}

fn decode_string(&mut self, buf: &mut Cursor<&mut BytesMut>) -> Result<Bytes, DecoderError> {
fn try_decode_string(
&mut self,
buf: &mut Cursor<&mut BytesMut>,
) -> Result<((usize, usize), Option<Bytes>), DecoderError> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's hard to know what these values are supposed to be. What if instead of returning a tuple, it return some struct DecodedString { .. } that named the usizes and Option<Bytes>?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for reply.

I am using tonic as grpc client and would receive a large HEADERS frame(grpc-status-details-bin) whose name is non_huff format and value is partial from C++ server. When decoding this header, the name is consumed because it is not huff, and then the value decoding is failed for DecoderError::NeedMore, and then the decode function returns with error but the buf is consumed.

I see that the CONTINUATION header currently tested in CI but both name and value are huff_encoded.

My first patch is simply replacing consume() with buf copy, and @olix0r pointed that the copy can be optimized. So i tried to save the decoding context and consume the buf only if both header name and header buf is successfully decoded.

I will update my patch to easily show the meaning of those values.

let old_pos = buf.position();
const HUFF_FLAG: u8 = 0b1000_0000;

// The first bit in the first byte contains the huffman encoded flag.
Expand All @@ -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<Bytes, DecoderError> {
let old_pos = buf.position();
let marker = self.try_decode_string(buf)?;
buf.set_position(old_pos);
Ok(take_string_marker(buf, marker))
}
}

Expand Down Expand Up @@ -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>),
) -> 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
Expand Down Expand Up @@ -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!(),
}
}
}