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.

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

Bytes::peek_ahead is unsound #186

Open
hkBst opened this issue Oct 19, 2024 · 2 comments · May be fixed by #188
Open

Bytes::peek_ahead is unsound #186

hkBst opened this issue Oct 19, 2024 · 2 comments · May be fixed by #188

Comments

@hkBst
Copy link

hkBst commented Oct 19, 2024

If n is large enough to cause wrapping then self.cursor.wrapping_add(n) would likely be less than self.start. Either saturating_add is needed, or a test that n < end - cursor.

https://github.com/seanmonstar/httparse/blob/master/src/iter.rs#L44-L55

Edit: actually saturating_add does not (yet?) exist for pointer arithmetic

@seanmonstar
Copy link
Owner

Hm, that does seem true. I'll just note that a search of the method shows it's only ever used correctly. Actually, it's only used once, to check for the 5th byte after comparing the previous 4. That usage could probably be touched up... Though I recognize that match was very specifically tuned to improve performance...

@hkBst
Copy link
Author

hkBst commented Oct 22, 2024

Right, there is only a single use as you noted, here:

httparse/src/lib.rs

Lines 843 to 867 in 97c7e6e

pub fn parse_method<'a>(bytes: &mut Bytes<'a>) -> Result<&'a str> {
const GET: [u8; 4] = *b"GET ";
const POST: [u8; 4] = *b"POST";
match bytes.peek_n::<[u8; 4]>(4) {
Some(GET) => {
// SAFETY: matched the ASCII string and boundary checked
let method = unsafe {
bytes.advance(4);
let buf = bytes.slice_skip(1);
str::from_utf8_unchecked(buf)
};
Ok(Status::Complete(method))
}
Some(POST) if bytes.peek_ahead(4) == Some(b' ') => {
// SAFETY: matched the ASCII string and boundary checked
let method = unsafe {
bytes.advance(5);
let buf = bytes.slice_skip(1);
str::from_utf8_unchecked(buf)
};
Ok(Status::Complete(method))
}
_ => parse_token(bytes),
}
}

and the computed pointer is at most one element past the end of the bytes slice, but that also means that there is no need for wrapping_add instead of add. I've made a PR inlining this function and removing it: #188.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants