[balsa] Delay resetting BalsaFrame. (#34140)

Delay resetting BalsaFrame until the first byte of the next message is parsed. This is necessary for being able to access information about the parsed headers after the message is fully parsed, for example, response status code, and content-length and transfer-encoding headers.

Fixes #34096

Balsa implementation tracking issue: #21245

Commit Message: [balsa] Delay resetting BalsaFrame.
Additional Description:
Risk Level: low
Testing: test/extensions/transport_sockets/http_11_proxy:connect_test
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a
Runtime guard: yes

Signed-off-by: Bence Béky <bnc@google.com>
Co-authored-by: Bence Béky <bnc@google.com>

changelogs/current.yaml

@@ -87,6 +87,10 @@ bug_fixes:
   change: |
     Handle ``append_action`` from :ref:`external authorization service <envoy_v3_api_msg_service.auth.v3.CheckResponse>`
     that was ignored.
+- area: http
+  change: |
+    Fix BalsaParser resetting state too early, guarded by default-true
+    ``envoy.reloadable_features.http1_balsa_delay_reset``.
 
 removed_config_or_runtime:
 # *Normally occurs at the end of the* :ref:`deprecation period <deprecated>`

source/common/http/http1/balsa_parser.cc

@@ -182,6 +182,14 @@ size_t BalsaParser::execute(const char* slice, int len) {
   ASSERT(status_ != ParserStatus::Error);
 
   if (len > 0 && !first_byte_processed_) {
+    if (delay_reset_) {
+      if (first_message_) {
+        first_message_ = false;
+      } else {
+        framer_.Reset();
+      }
+    }
+
     if (message_type_ == MessageType::Request && !allow_custom_methods_ &&
         !isFirstCharacterOfValidMethod(*slice)) {
       status_ = ParserStatus::Error;
@@ -353,7 +361,9 @@ void BalsaParser::MessageDone() {
     return;
   }
   status_ = convertResult(connection_->onMessageComplete());
-  framer_.Reset();
+  if (!delay_reset_) {
+    framer_.Reset();
+  }
   first_byte_processed_ = false;
   headers_done_ = false;
 }

source/common/http/http1/balsa_parser.h

@@ -3,6 +3,7 @@
 #include <memory>
 
 #include "source/common/http/http1/parser.h"
+#include "source/common/runtime/runtime_features.h"
 
 #include "absl/base/attributes.h"
 #include "quiche/balsa/balsa_enums.h"
@@ -76,9 +77,14 @@ class BalsaParser : public Parser, public quiche::BalsaVisitorInterface {
   const bool allow_custom_methods_ = false;
   bool first_byte_processed_ = false;
   bool headers_done_ = false;
+  // True until the first byte of the second message arrives.
+  bool first_message_ = true;
   ParserStatus status_ = ParserStatus::Ok;
   // An error message, often seemingly arbitrary to match http-parser behavior.
   absl::string_view error_message_;
+  // Latched value of `envoy.reloadable_features.http1_balsa_delay_reset`.
+  const bool delay_reset_ =
+      Runtime::runtimeFeatureEnabled("envoy.reloadable_features.http1_balsa_delay_reset");
 };
 
 } // namespace Http1

source/common/runtime/runtime_features.cc

@@ -51,6 +51,7 @@ RUNTIME_GUARD(envoy_reloadable_features_ext_authz_http_send_original_xff);
 RUNTIME_GUARD(envoy_reloadable_features_grpc_http1_reverse_bridge_change_http_status);
 RUNTIME_GUARD(envoy_reloadable_features_grpc_http1_reverse_bridge_handle_empty_response);
 RUNTIME_GUARD(envoy_reloadable_features_hmac_base64_encoding_only);
+RUNTIME_GUARD(envoy_reloadable_features_http1_balsa_delay_reset);
 RUNTIME_GUARD(envoy_reloadable_features_http1_connection_close_header_in_redirect);
 // Ignore the automated "remove this flag" issue: we should keep this for 1 year.
 RUNTIME_GUARD(envoy_reloadable_features_http1_use_balsa_parser);

test/extensions/transport_sockets/http_11_proxy/connect_test.cc

@@ -11,6 +11,7 @@
 #include "test/mocks/ssl/mocks.h"
 #include "test/test_common/environment.h"
 #include "test/test_common/network_utility.h"
+#include "test/test_common/test_runtime.h"
 #include "test/test_common/utility.h"
 
 #include "gmock/gmock.h"
@@ -22,6 +23,7 @@ using testing::ByMove;
 using testing::Const;
 using testing::InSequence;
 using testing::NiceMock;
+using testing::Optional;
 using testing::Return;
 using testing::ReturnRef;
 
@@ -431,13 +433,81 @@ TEST(ParseTest, TestValidResponse) {
 
 // The SelfContainedParser is only intended for header parsing but for coverage,
 // test a request with a body.
-TEST(ParseTest, CoverResponseBody) {
+TEST(ParseTest, CoverResponseBodyHttp10) {
+  TestScopedRuntime scoped_runtime;
+  scoped_runtime.mergeValues({{"envoy.reloadable_features.http1_balsa_delay_reset", "true"}});
+
   std::string headers = "HTTP/1.0 200 OK\r\ncontent-length: 2\r\n\r\n";
   std::string body = "ab";
 
   SelfContainedParser parser;
   parser.parser().execute(headers.c_str(), headers.length());
+  EXPECT_TRUE(parser.headersComplete());
   parser.parser().execute(body.c_str(), body.length());
+
+  EXPECT_NE(parser.parser().getStatus(), Http::Http1::ParserStatus::Error);
+  EXPECT_EQ(parser.parser().statusCode(), Http::Code::OK);
+  EXPECT_FALSE(parser.parser().isHttp11());
+  EXPECT_THAT(parser.parser().contentLength(), Optional(2));
+  EXPECT_FALSE(parser.parser().isChunked());
+  EXPECT_FALSE(parser.parser().hasTransferEncoding());
+}
+
+TEST(ParseTest, CoverResponseBodyHttp11) {
+  TestScopedRuntime scoped_runtime;
+  scoped_runtime.mergeValues({{"envoy.reloadable_features.http1_balsa_delay_reset", "true"}});
+
+  std::string headers = "HTTP/1.1 200 OK\r\ncontent-length: 2\r\n\r\n";
+  std::string body = "ab";
+
+  SelfContainedParser parser;
+  parser.parser().execute(headers.c_str(), headers.length());
+  EXPECT_TRUE(parser.headersComplete());
+  parser.parser().execute(body.c_str(), body.length());
+
+  EXPECT_NE(parser.parser().getStatus(), Http::Http1::ParserStatus::Error);
+  EXPECT_EQ(parser.parser().statusCode(), Http::Code::OK);
+  EXPECT_TRUE(parser.parser().isHttp11());
+  EXPECT_THAT(parser.parser().contentLength(), Optional(2));
+  EXPECT_FALSE(parser.parser().isChunked());
+  EXPECT_FALSE(parser.parser().hasTransferEncoding());
+}
+
+// Regression tests for #34096.
+TEST(ParseTest, ContentLengthZeroHttp10) {
+  TestScopedRuntime scoped_runtime;
+  scoped_runtime.mergeValues({{"envoy.reloadable_features.http1_balsa_delay_reset", "true"}});
+
+  constexpr absl::string_view headers = "HTTP/1.0 200 OK\r\ncontent-length: 0\r\n\r\n";
+
+  SelfContainedParser parser;
+  parser.parser().execute(headers.data(), headers.length());
+  EXPECT_TRUE(parser.headersComplete());
+
+  EXPECT_NE(parser.parser().getStatus(), Http::Http1::ParserStatus::Error);
+  EXPECT_EQ(parser.parser().statusCode(), Http::Code::OK);
+  EXPECT_FALSE(parser.parser().isHttp11());
+  EXPECT_THAT(parser.parser().contentLength(), Optional(0));
+  EXPECT_FALSE(parser.parser().isChunked());
+  EXPECT_FALSE(parser.parser().hasTransferEncoding());
+}
+
+TEST(ParseTest, ContentLengthZeroHttp11) {
+  TestScopedRuntime scoped_runtime;
+  scoped_runtime.mergeValues({{"envoy.reloadable_features.http1_balsa_delay_reset", "true"}});
+
+  constexpr absl::string_view headers = "HTTP/1.1 200 OK\r\ncontent-length: 0\r\n\r\n";
+
+  SelfContainedParser parser;
+  parser.parser().execute(headers.data(), headers.length());
+  EXPECT_TRUE(parser.headersComplete());
+
+  EXPECT_NE(parser.parser().getStatus(), Http::Http1::ParserStatus::Error);
+  EXPECT_EQ(parser.parser().statusCode(), Http::Code::OK);
+  EXPECT_TRUE(parser.parser().isHttp11());
+  EXPECT_THAT(parser.parser().contentLength(), Optional(0));
+  EXPECT_FALSE(parser.parser().isChunked());
+  EXPECT_FALSE(parser.parser().hasTransferEncoding());
 }
 
 } // namespace