relying on bnc fix

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
alyssawilk · May 15, 2024 · 5a8e4f1 · 5a8e4f1
1 parent 65c5c49
commit 5a8e4f1
Show file tree

Hide file tree

Showing 5 changed files with 77 additions and 8 deletions.
diff --git a/source/common/http/http1/balsa_parser.cc b/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;
 }

diff --git a/source/common/http/http1/balsa_parser.h b/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

diff --git a/source/extensions/transport_sockets/http_11_proxy/connect.cc b/source/extensions/transport_sockets/http_11_proxy/connect.cc
@@ -23,7 +23,8 @@ bool UpstreamHttp11ConnectSocket::isValidConnectResponse(absl::string_view respo
   bytes_processed = parser.parser().execute(response_payload.data(), response_payload.length());
   headers_complete = parser.headersComplete();
 
-  return parser.headersComplete() && parser.status200();
+  return parser.parser().getStatus() != Http::Http1::ParserStatus::Error &&
+         parser.headersComplete() && parser.parser().statusCode() == Http::Code::OK;
 }
 
 UpstreamHttp11ConnectSocket::UpstreamHttp11ConnectSocket(

diff --git a/source/extensions/transport_sockets/http_11_proxy/connect.h b/source/extensions/transport_sockets/http_11_proxy/connect.h
@@ -81,9 +81,6 @@ class SelfContainedParser : public Http::Http1::ParserCallbacks {
   }
   Http::Http1::CallbackResult onHeadersComplete() override {
     headers_complete_ = true;
-    // Latch this to work around https://github.com/envoyproxy/envoy/issues/34096
-    status_200_ = parser().getStatus() != Http::Http1::ParserStatus::Error &&
-                  parser().statusCode() == Http::Code::OK;
     parser_.pause();
     return Http::Http1::CallbackResult::Success;
   }
@@ -94,12 +91,10 @@ class SelfContainedParser : public Http::Http1::ParserCallbacks {
   void onChunkHeader(bool) override {}
 
   bool headersComplete() const { return headers_complete_; }
-  bool status200() const { return status_200_; }
   Http::Http1::BalsaParser& parser() { return parser_; }
 
 private:
   bool headers_complete_ = false;
-  bool status_200_ = false;
   Http::Http1::BalsaParser parser_;
 };
 

diff --git a/test/extensions/transport_sockets/http_11_proxy/connect_test.cc b/test/extensions/transport_sockets/http_11_proxy/connect_test.cc
@@ -22,6 +22,7 @@ using testing::ByMove;
 using testing::Const;
 using testing::InSequence;
 using testing::NiceMock;
+using testing::Optional;
 using testing::Return;
 using testing::ReturnRef;
 
@@ -431,13 +432,69 @@ 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) {
   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) {
+  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) {
+  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) {
+  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