Skip to content

http2: fixing upstream sending metadata after ending the stream#14061

Merged
mattklein123 merged 7 commits intoenvoyproxy:masterfrom
adisuissa:metadata_asan_decoder_bug
Nov 19, 2020
Merged

http2: fixing upstream sending metadata after ending the stream#14061
mattklein123 merged 7 commits intoenvoyproxy:masterfrom
adisuissa:metadata_asan_decoder_bug

Conversation

@adisuissa
Copy link
Copy Markdown
Contributor

Commit Message: fixing upstream sending metadata after ending the stream.
Additional Description:
A fuzz test detected that when upstream ends the stream, and the proceeds to send metadata, the response decoder is accessed after it was freed.
This happens when either the UpstreamRequest or the ActiveRequest are destroyed (as in the added integration test, and the failed fuzz test, respectively), and a following metadata frame sent by upstream is being processed by ConnectionImpl::onMetadataFrameComplete.
This PR verifies that a stream was not remotely closed before processing the metadata frame.

Risk Level: Medium - changes to codec (Metadata processing only).
Testing: Added an integration test and the fuzz test that detected the issue.
Docs Changes: N/A.
Release Notes: N/A.
Platform Specific Features: N/A.
Fixes fuzz issue: 26834

Signed-off-by: Adi Suissa-Peleg adip@google.com

Signed-off-by: Adi Suissa-Peleg <adip@google.com>
Signed-off-by: Adi Suissa-Peleg <adip@google.com>
Signed-off-by: Adi Suissa-Peleg <adip@google.com>
Signed-off-by: Adi Suissa-Peleg <adip@google.com>
…oder_bug

Signed-off-by: Adi Suissa-Peleg <adip@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!

Signed-off-by: Adi Suissa-Peleg <adip@google.com>
…oder_bug

Signed-off-by: Adi Suissa-Peleg <adip@google.com>
@adisuissa
Copy link
Copy Markdown
Contributor Author

@asraa Thanks for the comments.
I couldn't reproduce the failed CI tests (QUIC tsan and coverage error), so let's see if CI complains again, and I'll take a deeper look.

@yanavlasov yanavlasov self-assigned this Nov 18, 2020
@adisuissa
Copy link
Copy Markdown
Contributor Author

/retest

@repokitteh-read-only
Copy link
Copy Markdown

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #14061 (comment) was created by @adisuissa.

see: more, trace.

@mattklein123 mattklein123 merged commit f6fba53 into envoyproxy:master Nov 19, 2020
andreyprezotto pushed a commit to andreyprezotto/envoy that referenced this pull request Nov 24, 2020
qqustc pushed a commit to qqustc/envoy that referenced this pull request Nov 24, 2020
…yproxy#14061)

Signed-off-by: Adi Suissa-Peleg <adip@google.com>
Signed-off-by: Qin Qin <qqin@google.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.

5 participants