Skip to content

fuzz: fix oss crash on http utility function#11687

Merged
asraa merged 6 commits intoenvoyproxy:masterfrom
samflattery:utility_crash
Jun 24, 2020
Merged

fuzz: fix oss crash on http utility function#11687
asraa merged 6 commits intoenvoyproxy:masterfrom
samflattery:utility_crash

Conversation

@samflattery
Copy link
Contributor

@samflattery samflattery commented Jun 22, 2020

Commit Message: Fix crash on initializeAndValidateOptions fuzz test
Additional Description:

  • the crash occurred because validateCustomSettingsParameters does not allow duplicate custom settings identifiers and the fuzzer fed it an input that had duplicates
  • fixed by returning early when duplicates are present in the input
  • from aguedez@: "I think it's helpful to keep the unique custom settings parameter precondition. Even though the H/2 protocol allows duplicates to be sent to the peer, I think it's helpful to check during config load whether the user has mistakenly configured multiple parameters with the same identifier but different values."

Risk Level: Low
Testing: Passes the existing utility test corpora and new regression entry added
Docs Changes: N/A
Release Notes: N/A
Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=23616

Sam Flattery added 3 commits June 22, 2020 15:54
Signed-off-by: Sam Flattery <samflattery@google.com>
Signed-off-by: Sam Flattery <samflattery@google.com>
Signed-off-by: Sam Flattery <samflattery@google.com>
@samflattery
Copy link
Contributor Author

/cc @asraa
/cc @AndresGuedez

@asraa asraa requested a review from AndresGuedez June 22, 2020 14:58
Copy link
Contributor

@asraa asraa left a comment

Choose a reason for hiding this comment

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

Thanks Sam!

Looks like the docstring does warn that collisions results in failures, so thanks for checking on that!

// Collisions will trigger config validation failure on load/update. Likewise, inconsistencies

@asraa asraa self-assigned this Jun 22, 2020
Signed-off-by: Sam Flattery <samflattery@google.com>
asraa
asraa previously approved these changes Jun 22, 2020
Copy link
Contributor

@asraa asraa left a comment

Choose a reason for hiding this comment

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

LGTM!

Signed-off-by: Sam Flattery <samflattery@google.com>
Copy link
Contributor

@asraa asraa left a comment

Choose a reason for hiding this comment

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

LGTM!

AndresGuedez
AndresGuedez previously approved these changes Jun 23, 2020
Copy link
Contributor

@AndresGuedez AndresGuedez left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

Signed-off-by: Sam Flattery <samflattery@google.com>
Copy link
Contributor

@asraa asraa left a comment

Choose a reason for hiding this comment

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

Thanks!

@asraa asraa merged commit f190f05 into envoyproxy:master Jun 24, 2020
@samflattery samflattery deleted the utility_crash branch June 25, 2020 12:32
songhu pushed a commit to songhu/envoy that referenced this pull request Jun 25, 2020
Commit Message: Fix crash on initializeAndValidateOptions fuzz test
the crash occurred because validateCustomSettingsParameters does not allow duplicate custom settings identifiers and the fuzzer fed it an input that had duplicates. fixed by catching expected exceptions from duplicate custom settings identifiers.
Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=23616

Signed-off-by: Sam Flattery <samflattery@google.com>
yashwant121 pushed a commit to yashwant121/envoy that referenced this pull request Jul 24, 2020
Commit Message: Fix crash on initializeAndValidateOptions fuzz test
the crash occurred because validateCustomSettingsParameters does not allow duplicate custom settings identifiers and the fuzzer fed it an input that had duplicates. fixed by catching expected exceptions from duplicate custom settings identifiers.
Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=23616

Signed-off-by: Sam Flattery <samflattery@google.com>
Signed-off-by: yashwant121 <yadavyashwant36@gmail.com>
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