commit 9e875938052443c7f51b25012093be59e20598ac
author birenroy <birenroy@google.com> Thu Feb 15 14:25:56 2024 -0800
committer Copybara-Service <copybara-worker@google.com> Thu Feb 15 14:28:26 2024 -0800
tree 8324aed657f53bd45fde01b24ee71d11f4b9f9bb
parent 2958de853a8c3999f7d73242aa1d9cd9be9458c2

Fixes handling of DATA frame padding in OgHttp2Session.

Mishandling of padding can lead to a flow control leak, potentially causing the kind of connection stalls observed in https://github.com/envoyproxy/envoy/issues/32401.

Thanks to eagle-eyed https://github.com/diannahu for the root cause!

PiperOrigin-RevId: 607455607

quiche/http2/adapter/oghttp2_adapter_test.cc

diff --git a/quiche/http2/adapter/oghttp2_adapter_test.cc b/quiche/http2/adapter/oghttp2_adapter_test.cc
index be2b1ce..ddaffae 100644
--- a/quiche/http2/adapter/oghttp2_adapter_test.cc
+++ b/quiche/http2/adapter/oghttp2_adapter_test.cc
@@ -8123,7 +8123,6 @@
 
   const int64_t result = adapter->ProcessBytes(frames);
   EXPECT_EQ(result, frames.size());
-  EXPECT_LT(adapter->GetReceiveWindowSize(), 2048);
 
   EXPECT_TRUE(adapter->want_write());
 
@@ -8131,17 +8130,19 @@
   EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x0, 0));
   EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, ACK_FLAG));
   EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, ACK_FLAG, 0));
-  // BUG: Most of the flow control window consumed is padding, so the adapter
-  // should have generated window updates.
-  EXPECT_CALL(visitor, OnBeforeFrameSent(WINDOW_UPDATE, 1, _, 0x0)).Times(0);
-  EXPECT_CALL(visitor, OnFrameSent(WINDOW_UPDATE, 1, _, 0x0, 0)).Times(0);
-  EXPECT_CALL(visitor, OnBeforeFrameSent(WINDOW_UPDATE, 0, _, 0x0)).Times(0);
-  EXPECT_CALL(visitor, OnFrameSent(WINDOW_UPDATE, 0, _, 0x0, 0)).Times(0);
+  // Since most of the flow control window consumed is padding, the adapter
+  // generates window updates.
+  EXPECT_CALL(visitor, OnBeforeFrameSent(WINDOW_UPDATE, 1, _, 0x0)).Times(1);
+  EXPECT_CALL(visitor, OnFrameSent(WINDOW_UPDATE, 1, _, 0x0, 0)).Times(1);
+  EXPECT_CALL(visitor, OnBeforeFrameSent(WINDOW_UPDATE, 0, _, 0x0)).Times(1);
+  EXPECT_CALL(visitor, OnFrameSent(WINDOW_UPDATE, 0, _, 0x0, 0)).Times(1);
 
   const int send_result = adapter->Send();
   EXPECT_EQ(0, send_result);
   EXPECT_THAT(visitor.data(),
-              EqualsFrames({SpdyFrameType::SETTINGS, SpdyFrameType::SETTINGS}));
+              EqualsFrames({SpdyFrameType::SETTINGS, SpdyFrameType::SETTINGS,
+                            SpdyFrameType::WINDOW_UPDATE,
+                            SpdyFrameType::WINDOW_UPDATE}));
 }
 
 // Verifies that NoopHeaderValidator allows several header combinations that

quiche/http2/adapter/oghttp2_session.cc

diff --git a/quiche/http2/adapter/oghttp2_session.cc b/quiche/http2/adapter/oghttp2_session.cc
index bd9464f..6527c1c 100644
--- a/quiche/http2/adapter/oghttp2_session.cc
+++ b/quiche/http2/adapter/oghttp2_session.cc
@@ -1147,12 +1147,16 @@
 
 void OgHttp2Session::OnStreamPadLength(spdy::SpdyStreamId stream_id,
                                        size_t value) {
-  bool result = visitor_.OnDataPaddingLength(stream_id, 1 + value);
+  const size_t padding_length = 1 + value;
+  const bool result = visitor_.OnDataPaddingLength(stream_id, padding_length);
   if (!result) {
     fatal_visitor_callback_failure_ = true;
     decoder_.StopProcessing();
   }
-  MarkDataBuffered(stream_id, 1 + value);
+  connection_window_manager_.MarkWindowConsumed(padding_length);
+  if (auto it = stream_map_.find(stream_id); it != stream_map_.end()) {
+    it->second.window_manager.MarkWindowConsumed(padding_length);
+  }
 }
 
 void OgHttp2Session::OnStreamPadding(spdy::SpdyStreamId /*stream_id*/, size_t