grpc_http1_reverse_bridge: fix SEGFAULT when buffering large response without response_size_header#45880
Open
relvira wants to merge 2 commits into
Open
Conversation
relvira
had a problem deploying
to
external-contributors
GitHub Actions
Error
|
Hi @relvira, welcome and thank you for your contribution. We will try to review your Pull Request as quickly as possible. In the meantime, please take a look at the contribution guidelines if you have not done so already. |
relvira
force-pushed
the
fix-grpc-reverse-bridge-streaming-upstream
branch
from
832583b to
a9ed2a4
Compare
relvira
had a problem deploying
to
external-contributors
GitHub Actions
Error
relvira
force-pushed
the
fix-grpc-reverse-bridge-streaming-upstream
branch
from
a9ed2a4 to
f074569
Compare
relvira
temporarily deployed
to
external-contributors
GitHub Actions
Inactive
relvira
changed the title
nahratzah
reviewed
Jun 29, 2026
| uint64_t content_length = 0; | ||
| if (!length_header.empty() && absl::SimpleAtoi(length_header, &content_length) && | ||
| content_length > 0) { | ||
| response_message_length_ = static_cast<uint32_t>(content_length); |
Contributor
There was a problem hiding this comment.
I think changing response_message_length_ to uint64_t might be doable, and gets rid of the cast. (Not sure if that requires changing upstream_response_bytes_ too.)
Contributor
|
@relvira format check is complaining: https://github.com/envoyproxy/envoy/actions/runs/28396207097/job/84135494358#step:21:755 /wait |
…s available When withhold_grpc_frames is enabled without response_size_header, the filter previously buffered the entire response body and released it in a single encodeData call on end_stream. For large responses (e.g. 154MB), this generates ~9,600 H2 DATA frames in one burst, which can trigger memory corruption via write buffer high watermark callbacks during the Send loop, causing a SEGFAULT in Http2::ConnectionImpl::StreamImpl::encodeData(). This fix uses the Content-Length header from HTTP/1.1 upstream responses to enable the existing streaming path (previously only available via response_size_header config). Data now flows through in chunks as received, avoiding the frame burst that crashes the H2 codec. Responses without Content-Length continue to use the buffering path. Signed-off-by: Rafael Elvira <me@relvira.org>
relvira
force-pushed
the
fix-grpc-reverse-bridge-streaming-upstream
branch
from
f074569 to
2270398
Compare
relvira
temporarily deployed
to
external-contributors
GitHub Actions
Inactive
Upstream changed route() return type to OptRef, so Return(nullptr) no longer compiles. Signed-off-by: Rafael Elvira <me@relvira.org>
relvira
requested a deployment
to
external-contributors
GitHub Actions
Waiting
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description:
Fixes #45860
When
withhold_grpc_framesis enabled withoutresponse_size_header, the filter buffers the entire upstream response body and releases it all in a singleencodeData()call onend_stream. For responses larger thaninitial_stream_window_size(default 16MB since #40716), this causes the H2 codec to queue thousands of DATA frames in a singlesendPendingFrames()call. The write buffer exceeds the high watermark during the frame burst, triggering callbacks that interact with the stream mid-encode, resulting in memory corruption and a SEGFAULT atHttp2::ConnectionImpl::StreamImpl::encodeData().This bug is latent since the filter's introduction but was previously masked by the 256MB default stream window. Commit 9ad6cf1 (#40716) reduced the default to 16MB, exposing the crash for any response >16MB.
This fix uses the
Content-Lengthheader from upstream HTTP/1.1 responses to enable the existing streaming path (previously only available viaresponse_size_headerconfig). Data now flows through in chunks as received from the upstream, avoiding the frame burst that crashes the H2 codec.Responses without
Content-Lengthcontinue to use the existing buffering path (no behavior change).Risk Level: Low — only activates when
Content-Lengthis present andwithhold_grpc_frames=truewithoutresponse_size_header. Existing behavior unchanged for all other cases.Testing:
WithholdGrpcFramesStreamsWithContentLengthtest — validates streaming happy pathWithholdGrpcFramesContentLengthMismatchtest — validates error on size mismatchGrpcRequestNoContentLengthtest unchanged — confirms fallback to bufferingAI disclosure: Claude (Anthropic) was used to assist with writing this patch and tests. I have reviewed and understand all submitted code.
Signed-off-by: Rafael Elvira me@relvira.org