commit 05d00fba2aab5b9ced8a7ce625d51ae657493913
author birenroy <birenroy@google.com> Mon Oct 31 07:19:02 2022 -0700
committer Copybara-Service <copybara-worker@google.com> Mon Oct 31 07:20:18 2022 -0700
tree 92757a47714a90388c42ec671a83a95ca9e9237d
parent 97d97a5358600b69b37716e60d73b8b9d59c800f

Initializes the memory backing CallbackVisitor::current_frame_ in the constructor.

With the equivalent change in `//third_party/envoy/external_quiche`, the fuzz test case passes:
```
$ blaze --blazerc=/dev/null test --config=msan-fuzzer --test_strategy=local \
--test_sharding_strategy=disabled   --test_env=ENABLE_BLAZE_TEST_FUZZING=1 \
--test_arg=-runs=100 --test_arg=/tmp/testcase-4551436517376000 \
//third_party/envoy/src/test/common/http:http2_codec_impl_fuzz_test
```
http://sponge2/126361b6-06d7-4a4d-a10c-0c21faf44546
PiperOrigin-RevId: 485040699

quiche/http2/adapter/callback_visitor.cc

diff --git a/quiche/http2/adapter/callback_visitor.cc b/quiche/http2/adapter/callback_visitor.cc
index ea782e7..b5e6433 100644
--- a/quiche/http2/adapter/callback_visitor.cc
+++ b/quiche/http2/adapter/callback_visitor.cc
@@ -47,6 +47,7 @@
   nghttp2_session_callbacks_new(&c);
   *c = callbacks;
   callbacks_ = MakeCallbacksPtr(c);
+  memset(&current_frame_, 0, sizeof(current_frame_));
 }
 
 int64_t CallbackVisitor::OnReadyToSend(absl::string_view serialized) {
@@ -78,8 +79,14 @@
                  << ", type=" << int(type) << ", length=" << length
                  << ", flags=" << int(flags) << ")";
   if (static_cast<FrameType>(type) == FrameType::CONTINUATION) {
-    // Treat CONTINUATION as HEADERS
-    QUICHE_DCHECK_EQ(current_frame_.hd.stream_id, stream_id);
+    if (static_cast<FrameType>(current_frame_.hd.type) != FrameType::HEADERS ||
+        current_frame_.hd.stream_id == 0 ||
+        current_frame_.hd.stream_id != stream_id) {
+      // CONTINUATION frames must follow HEADERS on the same stream. If no
+      // frames have been received, the type is initialized to zero, and the
+      // comparison will fail.
+      return false;
+    }
     current_frame_.hd.length += length;
     current_frame_.hd.flags |= flags;
     QUICHE_DLOG_IF(ERROR, length == 0) << "Empty CONTINUATION!";

quiche/http2/adapter/callback_visitor_test.cc

diff --git a/quiche/http2/adapter/callback_visitor_test.cc b/quiche/http2/adapter/callback_visitor_test.cc
index 3a42f53..b0bf971 100644
--- a/quiche/http2/adapter/callback_visitor_test.cc
+++ b/quiche/http2/adapter/callback_visitor_test.cc
@@ -199,7 +199,7 @@
 
   // HEADERS on stream 1
   EXPECT_CALL(callbacks, OnBeginFrame(HasFrameHeader(1, HEADERS, 0x0)));
-  visitor.OnFrameHeader(1, 23, HEADERS, 0x0);
+  ASSERT_TRUE(visitor.OnFrameHeader(1, 23, HEADERS, 0x0));
 
   EXPECT_CALL(callbacks,
               OnBeginHeaders(IsHeaders(1, _, NGHTTP2_HCAT_RESPONSE)));
@@ -212,7 +212,7 @@
   visitor.OnHeaderForStream(1, "server", "my-fake-server");
 
   EXPECT_CALL(callbacks, OnBeginFrame(HasFrameHeader(1, CONTINUATION, 0x4)));
-  visitor.OnFrameHeader(1, 23, CONTINUATION, 0x4);
+  ASSERT_TRUE(visitor.OnFrameHeader(1, 23, CONTINUATION, 0x4));
 
   EXPECT_CALL(callbacks,
               OnHeader(_, "date", "Tue, 6 Apr 2021 12:54:01 GMT", _));
@@ -225,6 +225,50 @@
   visitor.OnEndHeadersForStream(1);
 }
 
+TEST(ClientCallbackVisitorUnitTest, ContinuationNoHeaders) {
+  testing::StrictMock<MockNghttp2Callbacks> callbacks;
+  CallbackVisitor visitor(Perspective::kClient,
+                          *MockNghttp2Callbacks::GetCallbacks(), &callbacks);
+  // Because no stream precedes the CONTINUATION frame, the stream ID does not
+  // match, and the method returns false.
+  EXPECT_FALSE(visitor.OnFrameHeader(1, 23, CONTINUATION, 0x4));
+}
+
+TEST(ClientCallbackVisitorUnitTest, ContinuationWrongPrecedingType) {
+  testing::StrictMock<MockNghttp2Callbacks> callbacks;
+  CallbackVisitor visitor(Perspective::kClient,
+                          *MockNghttp2Callbacks::GetCallbacks(), &callbacks);
+
+  EXPECT_CALL(callbacks, OnBeginFrame(HasFrameHeader(1, WINDOW_UPDATE, _)));
+  visitor.OnFrameHeader(1, 4, WINDOW_UPDATE, 0);
+
+  // Because the CONTINUATION frame does not follow HEADERS, the method returns
+  // false.
+  EXPECT_FALSE(visitor.OnFrameHeader(1, 23, CONTINUATION, 0x4));
+}
+
+TEST(ClientCallbackVisitorUnitTest, ContinuationWrongStream) {
+  testing::StrictMock<MockNghttp2Callbacks> callbacks;
+  CallbackVisitor visitor(Perspective::kClient,
+                          *MockNghttp2Callbacks::GetCallbacks(), &callbacks);
+  // HEADERS on stream 1
+  EXPECT_CALL(callbacks, OnBeginFrame(HasFrameHeader(1, HEADERS, 0x0)));
+  ASSERT_TRUE(visitor.OnFrameHeader(1, 23, HEADERS, 0x0));
+
+  EXPECT_CALL(callbacks,
+              OnBeginHeaders(IsHeaders(1, _, NGHTTP2_HCAT_RESPONSE)));
+  visitor.OnBeginHeadersForStream(1);
+
+  EXPECT_CALL(callbacks, OnHeader(_, ":status", "200", _));
+  visitor.OnHeaderForStream(1, ":status", "200");
+
+  EXPECT_CALL(callbacks, OnHeader(_, "server", "my-fake-server", _));
+  visitor.OnHeaderForStream(1, "server", "my-fake-server");
+
+  // The CONTINUATION stream ID does not match the one from the HEADERS.
+  EXPECT_FALSE(visitor.OnFrameHeader(3, 23, CONTINUATION, 0x4));
+}
+
 TEST(ClientCallbackVisitorUnitTest, ResetAndGoaway) {
   testing::StrictMock<MockNghttp2Callbacks> callbacks;
   CallbackVisitor visitor(Perspective::kClient,