Skip to content

Commit c1d75a0

Browse files
committed
contrib/golang: address race condition in Go HTTP filter
addresses #44320 the issue here arises when the client sends data and the data is pending to be dispatched. Previously, the stream could have been destroyed already by the time `continueStatusInternal` was invoked. This PR adds safety-checks in the Go shim, and on the C++ filter side to avoid it this was tested by building Envoy + a test client that sends HTTP/1.1 + POST with a body, and verifying the crash happens before consistently, and no longer crashes with the patch Signed-off-by: Henry Wang <henry.wang@datadoghq.com>
1 parent 65fdd68 commit c1d75a0

3 files changed

Lines changed: 46 additions & 0 deletions

File tree

contrib/golang/filters/http/source/go/pkg/http/shim.go

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -157,6 +157,9 @@ func getRequest(r *C.httpRequest) *httpRequest {
157157
func getState(s *C.processState) *processState {
158158
r := s.req
159159
req := getRequest(r)
160+
if req == nil {
161+
return nil
162+
}
160163
if s.is_encoding == 0 {
161164
return &req.decodingState.processState
162165
}
@@ -237,6 +240,9 @@ func envoyGoFilterOnHttpHeader(s *C.processState, endStream, headerNum, headerBy
237240
//export envoyGoFilterOnHttpData
238241
func envoyGoFilterOnHttpData(s *C.processState, endStream, buffer, length uint64) uint64 {
239242
state := getState(s)
243+
if state == nil {
244+
return uint64(api.Continue)
245+
}
240246

241247
req := state.request
242248
if req.pInfo.paniced {
@@ -389,6 +395,9 @@ func envoyGoFilterOnHttpDestroy(r *C.httpRequest, reason uint64) {
389395
//export envoyGoRequestSemaDec
390396
func envoyGoRequestSemaDec(r *C.httpRequest) {
391397
req := getRequest(r)
398+
if req == nil {
399+
return
400+
}
392401
defer req.recoverPanic()
393402
req.resumeWaitCallback()
394403
}

contrib/golang/filters/http/source/golang_filter.cc

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -404,12 +404,19 @@ void Filter::continueStatusInternal(ProcessorState& state, GolangStatus status)
404404

405405
case FilterState::ProcessingTrailer:
406406
state.continueDoData();
407+
if (hasDestroyed()) {
408+
return;
409+
}
407410
state.continueProcessing();
408411
break;
409412

410413
default:
411414
ASSERT(0, "unexpected state");
412415
}
416+
417+
if (hasDestroyed()) {
418+
return;
419+
}
413420
}
414421

415422
ENVOY_LOG(debug,
@@ -426,6 +433,9 @@ void Filter::continueStatusInternal(ProcessorState& state, GolangStatus status)
426433
auto done = doDataGo(state, state.getBufferData(), state.getEndStream());
427434
if (done) {
428435
state.continueDoData();
436+
if (hasDestroyed()) {
437+
return;
438+
}
429439
} else {
430440
// do not process trailers when data is not finished
431441
return;

contrib/golang/filters/http/test/golang_integration_test.cc

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2128,4 +2128,31 @@ TEST_P(GolangIntegrationTest, DrainConnectionUponCompletion) {
21282128
cleanupUpstreamAndDownstream();
21292129
}
21302130

2131+
// Regression test for https://github.com/envoyproxy/envoy/issues/44320.
2132+
TEST_P(GolangIntegrationTest, AsyncContinueWithNoHealthyUpstream) {
2133+
config_helper_.addConfigModifier([](envoy::config::bootstrap::v3::Bootstrap& bootstrap) {
2134+
auto* cluster = bootstrap.mutable_static_resources()->mutable_clusters(0);
2135+
cluster->mutable_load_assignment()->mutable_endpoints(0)->clear_lb_endpoints();
2136+
});
2137+
2138+
initializeBasicFilter(BASIC, "test.com");
2139+
2140+
codec_client_ = makeHttpConnection(makeClientConnection(lookupPort("http")));
2141+
2142+
Http::TestRequestHeaderMapImpl request_headers{
2143+
{":method", "POST"}, {":path", "/test?async=1&sleep=1"}, {":scheme", "http"},
2144+
{":authority", "test.com"}, {"x-test-header-0", "foo"},
2145+
};
2146+
2147+
auto encoder_decoder = codec_client_->startRequest(request_headers);
2148+
Http::RequestEncoder& request_encoder = encoder_decoder.first;
2149+
auto response = std::move(encoder_decoder.second);
2150+
codec_client_->sendData(request_encoder, "request body", true);
2151+
2152+
ASSERT_TRUE(response->waitForEndStream());
2153+
EXPECT_EQ("503", response->headers().getStatusValue());
2154+
2155+
cleanup();
2156+
}
2157+
21312158
} // namespace Envoy

0 commit comments

Comments
 (0)