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

Add some missing bounds checks. #260

Merged
merged 2 commits into from Apr 23, 2018

Conversation

Projects
None yet
3 participants
@goffrie
Copy link
Contributor

goffrie commented Apr 17, 2018

Ran a fuzzer and found a few places where we were panicking instead of returning errors.

@hawkw hawkw requested review from carllerche and seanmonstar Apr 17, 2018

@seanmonstar
Copy link
Collaborator

seanmonstar left a comment

Cool, thanks for submitting these!

How'd you find them? It might be useful to have such tests in the repo directly...

@@ -153,6 +153,9 @@ impl Headers {

// Read the padding length
if flags.is_padded() {
if src.len() < 1 {
return Err(Error::MalformedMessage);
}
// TODO: Ensure payload is sized correctly

This comment has been minimized.

@seanmonstar

seanmonstar Apr 17, 2018

Collaborator

Maybe there's more involved, but wanted to check: does this addition essentially complete this TODO?

This comment has been minimized.

@goffrie

goffrie Apr 20, 2018

Author Contributor

Seems like it; I removed the comment.

@@ -322,6 +322,10 @@ where
let last_stream_id = frame.last_stream_id();
let err = frame.reason().into();

if actions.recv.max_stream_id() < last_stream_id {

This comment has been minimized.

@seanmonstar

seanmonstar Apr 17, 2018

Collaborator

I got a bit confused with this, until I went digging into recv to read the comments about max_stream_id. Whatcha think if there was a comment right here just saying to the effect of "if a new GOAWAY has a higher stream id than a previous GOAWAY, that's bad"?

This comment has been minimized.

@goffrie

goffrie Apr 20, 2018

Author Contributor

👍

@goffrie goffrie referenced this pull request Apr 20, 2018

Closed

Fuzzing with honggfuzz-rs #263

@carllerche

This comment has been minimized.

Copy link
Owner

carllerche commented Apr 23, 2018

Thanks @goffrie!

Looks like the fuzzing has been submitted in another PR. I'm good with this!

@carllerche carllerche merged commit 11f9141 into carllerche:master Apr 23, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.