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.

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

@seanmonstar 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
Member

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
Member

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

👍

@carllerche
Copy link
Collaborator

@carllerche 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 hyperium:master Apr 23, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@goffrie goffrie deleted the goffrie:bounds-checks branch Mar 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants