Skip to content

Added fuzz test case and initialized the boolean that previously was uninitialized#12014

Merged
asraa merged 2 commits intoenvoyproxy:masterfrom
KBaichoo:http-filter-fuzz-test-ub-bug
Jul 10, 2020
Merged

Added fuzz test case and initialized the boolean that previously was uninitialized#12014
asraa merged 2 commits intoenvoyproxy:masterfrom
KBaichoo:http-filter-fuzz-test-ub-bug

Conversation

@KBaichoo
Copy link
Contributor

@KBaichoo KBaichoo commented Jul 9, 2020

Commit Message: Fixes a filter related fuzz bug.
Additional Description: Initialized the boolean member to false, based on observations of usage in unit tests (https://github.com/envoyproxy/envoy/blob/8614e837cf0e4ba35eea8a45085953274b88fa03/test/extensions/filters/http/admission_control/admission_control_filter_test.cc)
Risk Level: Low
Testing: Corpus entries added as regression.
Docs Changes: N/A
Release Notes: N/A
Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=23843

Signed-off-by: Kevin Baichoo kbaichoo@google.com

…issue.

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>
@KBaichoo KBaichoo requested a review from mattklein123 as a code owner July 9, 2020 22:18
@dio
Copy link
Member

dio commented Jul 10, 2020

#12018 is merged. Please sync your branch with head to fix the CI (Linux-x64 asan) issue. Thanks!

…test-ub-bug

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>
@KBaichoo
Copy link
Contributor Author

Done, thanks for the heads up.

@asraa asraa assigned asraa and tonya11en and unassigned asraa Jul 10, 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.

Thanks Kevin!! This looks great, glad to see more fuzz bugs fixed :)

Just passing the review to @tonya11en to address this question because my read on it may be that it needs to be flipped.
if

if (Grpc::Common::isGrpcResponseHeaders(headers, end_stream)) {

isn't executed (and so the bool isn't initialized there) because of an early return/false condition, then we probably expect the grpc status to be in trailers?

@tonya11en
Copy link
Member

tonya11en commented Jul 10, 2020

Thanks Kevin!! This looks great, glad to see more fuzz bugs fixed :)

Just passing the review to @tonya11en to address this question because my read on it may be that it needs to be flipped.
if

if (Grpc::Common::isGrpcResponseHeaders(headers, end_stream)) {

isn't executed (and so the bool isn't initialized there) because of an early return/false condition, then we probably expect the grpc status to be in trailers?

You can always expect a gRPC content-type header when it's a gRPC reply, so in any scenario where that's not parsed, the boolean would not be set and we shouldn't expect to parse a gRPC status in the trailers. Setting the boolean to false here would maintain this behavior, where setting it true would cause the filter to attempt to parse a gRPC status from the trailers in all of the early return cases.

This LGTM as-is. Thanks for fixing!

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 @tonya11en for the feedback!

@asraa asraa merged commit a023627 into envoyproxy:master Jul 10, 2020
scheler pushed a commit to scheler/envoy that referenced this pull request Aug 4, 2020
…issue. (envoyproxy#12014)

Commit Message: Fixes a filter related fuzz bug due to an uninitialized bool
Additional Description: Initialized the boolean member to false, based on observations of usage in unit tests
Signed-off-by: Kevin Baichoo <kbaichoo@google.com>
Signed-off-by: scheler <santosh.cheler@appdynamics.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.

4 participants