Skip to content

Fixed grpc health check bug#13870

Merged
asraa merged 7 commits intoenvoyproxy:masterfrom
zasweq:grpc-fuzz-bug-fix
Nov 5, 2020
Merged

Fixed grpc health check bug#13870
asraa merged 7 commits intoenvoyproxy:masterfrom
zasweq:grpc-fuzz-bug-fix

Conversation

@zasweq
Copy link
Copy Markdown
Contributor

@zasweq zasweq commented Nov 2, 2020

Signed-off-by: Zach Reyes zasweq@google.com

Commit Message: Fixed gRPC health check bug
Additional Description: Fixes issue https://oss-fuzz.com/testcase-detail/6242204122873856. My health check fuzzer ran into this bug. What happened with this bug was that the decoder for gRPC frames sometimes encoded frames into a frame vector even though decode() returns false in the case of valid frames and then invalid frames in the buffer. Thus, this section of decodeData() called decoder_->decode() which returned false, which led to onRpcComplete being called: https://github.com/envoyproxy/envoy/blob/master/source/common/upstream/health_checker_impl.cc#L623. Thus, I added a return (which used to be implicit) after this, as frames() may still put frames in the grpc frame vector, thus calling onRpcComplete() again in the same decodeData() and causing a null dereference. I also added unit tests to show that the decoder can still put frames in the output vector even though it fails eventually and returns false.

Same as #13855, but different fix.
Risk Level: Low
Testing: Added regression test.

Signed-off-by: Zach <zasweq@google.com>
Signed-off-by: Zach <zasweq@google.com>
@zasweq
Copy link
Copy Markdown
Contributor Author

zasweq commented Nov 2, 2020

/assign @asraa @adisuissa @htuch @mattklein123

Copy link
Copy Markdown
Contributor

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this, I'll close my PR for now.
What happens if the gRPC decoder clears the output (frames) if the decoding fails?

Comment thread test/common/grpc/codec_test.cc Outdated
Comment thread test/common/grpc/codec_test.cc Outdated
Copy link
Copy Markdown
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.

Out of curiosity, is it the presence of null bytes in the frame data that makes the grpc decoder fail or is it something in conjunction with that?

(if so, is it easy to simplify the test case?)

It looks like

decoding_error_ = true;
is the only case to cause a decoding_error_

Comment thread test/common/upstream/health_checker_impl_test.cc Outdated
@asraa
Copy link
Copy Markdown
Contributor

asraa commented Nov 3, 2020

\cc @fengli79
cc'ing in case you have some context on the grpc decoder contract

@mattklein123 mattklein123 removed their assignment Nov 3, 2020
Signed-off-by: Zach <zasweq@google.com>
Copy link
Copy Markdown
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, please check format!

Comment thread test/common/grpc/codec_test.cc Outdated
Comment thread test/common/grpc/codec_test.cc Outdated
Comment thread test/common/grpc/codec_test.cc Outdated
Comment thread test/common/grpc/codec_test.cc Outdated
Comment thread test/common/grpc/codec_test.cc Outdated
Comment thread test/common/grpc/codec_test.cc Outdated
Comment thread test/common/grpc/codec_test.cc Outdated
Comment thread test/common/grpc/codec_test.cc
Comment thread test/common/grpc/codec_test.cc Outdated
Comment thread test/common/grpc/codec_test.cc Outdated
Signed-off-by: Zach <zasweq@google.com>
Copy link
Copy Markdown
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.

Great tests! LGTM
/wait

Comment thread test/common/grpc/codec_test.cc Outdated
Comment thread test/common/grpc/codec_test.cc Outdated
Signed-off-by: Zach <zasweq@google.com>
asraa
asraa previously approved these changes Nov 4, 2020
Copy link
Copy Markdown
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, LGTM!
@envoyproxy/senior-maintainers could you please take a very quick look to approve this bug fix handling a faulty response?

Signed-off-by: Zach <zasweq@google.com>
@mattklein123
Copy link
Copy Markdown
Member

LGTM can you merge main?

/wait

@asraa asraa merged commit a491dc5 into envoyproxy:master Nov 5, 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

Development

Successfully merging this pull request may close these issues.

5 participants