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
Merged

Conversation

goffrie
Copy link
Contributor

@goffrie goffrie commented Apr 17, 2018

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

Copy link
Member

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 {
Copy link
Member

Choose a reason for hiding this comment

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

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"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@carllerche
Copy link
Collaborator

Thanks @goffrie!

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

@carllerche carllerche merged commit 11f9141 into hyperium:master Apr 23, 2018
@goffrie goffrie deleted the bounds-checks branch March 1, 2020 07:29
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 this pull request may close these issues.

3 participants