diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 783cccd4139a..c691a10a3a8c 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -133,6 +133,11 @@ minor_behavior_changes: bug_fixes: # *Changes expected to improve the state of the world and are unlikely to have negative effects* +- area: router + change: | + Fixed a bug where the router's buffer overflow check double-counted the request body size when an + upstream filter (e.g. the buffer filter) had already buffered all data. This caused requests with + body size between half and full buffer limit to incorrectly cancel internal redirects. - area: router change: | Fixed a crash in scoped RDS when on-demand update is triggered for a scope that uses inline diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 1cb812071485..f7c4022b019e 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -1005,15 +1005,19 @@ Http::FilterDataStatus Filter::decodeData(Buffer::Instance& data, bool end_strea // Check if we would exceed buffer limits, regardless of current buffering state // This ensures error details are set even if retry state was cleared due to upstream reset. - const bool would_exceed_buffer = - (getLength(callbacks_->decodingBuffer()) + data.length() > effective_buffer_limit); + // When an upstream filter (e.g. the buffer filter) has already buffered all data, + // the decoding buffer and `data` may be the same object. Avoid double-counting in that case. + const Buffer::Instance* decoding_buffer = callbacks_->decodingBuffer(); + const uint64_t payload_length = + (decoding_buffer != &data) ? getLength(decoding_buffer) + data.length() : data.length(); + const bool would_exceed_buffer = (payload_length > effective_buffer_limit); // Handle buffer overflow. if (buffering && would_exceed_buffer) { ENVOY_LOG(debug, "The request payload has at least {} bytes data which exceeds buffer limit {}. " "Giving up on buffering.", - getLength(callbacks_->decodingBuffer()) + data.length(), effective_buffer_limit); + payload_length, effective_buffer_limit); cluster_->trafficStats()->retry_or_shadow_abandoned_.inc(); retry_state_.reset(); ENVOY_LOG(debug, "retry or redirect buffer overflow: skipping buffering"); diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index 138902f8a916..3617d522a60a 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -5457,6 +5457,37 @@ TEST_F(RouterTest, InternalRedirectWithRequestBodyBufferOverflow) { .value()); } +// Regression test: when an upstream filter (e.g. the buffer filter) has already buffered all +// request data, the decoding buffer and the data argument to decodeData() are the same object. +// The buffer overflow check must not double-count the size in this case. +TEST_F(RouterTest, InternalRedirectNoDoubleCountWhenDecodingBufferIsData) { + // Body is 15 bytes, limit is 20. Without the fix, the check would compute 15+15=30 > 20 + // and incorrectly mark the request as buffer-overflowed. + Buffer::OwnedImpl body_data("fifteen_bytes!!"); + ASSERT_EQ(15, body_data.length()); + + EXPECT_CALL(callbacks_.route_->route_entry_, requestBodyBufferLimit()).WillRepeatedly(Return(20)); + // Return a pointer to the SAME buffer to simulate the buffer filter having + // already buffered the data into the shared decoding buffer. + EXPECT_CALL(callbacks_, decodingBuffer()).WillRepeatedly(Return(&body_data)); + EXPECT_CALL(callbacks_, addDecodedData(_, true)); + + enableRedirects(); + sendRequest(false); + + EXPECT_CALL(callbacks_.dispatcher_, createTimer_); + + // Data should be buffered (not rejected) since 15 < 20. + EXPECT_EQ(Http::FilterDataStatus::StopIterationNoBuffer, router_->decodeData(body_data, true)); + + // The redirect should proceed because the buffer did not overflow. + EXPECT_EQ(0U, cm_.thread_local_cluster_.cluster_.info_->stats_store_ + .counter("retry_or_shadow_abandoned") + .value()); + + router_->onDestroy(); +} + TEST_F(RouterTest, CrossSchemeRedirectRejectedByPolicy) { enableRedirects(); sendRequest();