Propagates DATA padding length to the visitor in OgHttp2Adapter and CallbackVisitor. This fixes //gfe/gfe2/http2/e2e:end_to_end_test_http2_gfe3: http://sponge2/90e283dc-9ab1-4495-a4f1-92cd421368d0 Re-enabling the test will have to wait for the next QUICHE update and Envoy import, though. PiperOrigin-RevId: 424728304
diff --git a/http2/adapter/callback_visitor.cc b/http2/adapter/callback_visitor.cc index 4b2f078..1dd082e 100644 --- a/http2/adapter/callback_visitor.cc +++ b/http2/adapter/callback_visitor.cc
@@ -201,12 +201,23 @@ return true; } +bool CallbackVisitor::OnDataPaddingLength(Http2StreamId /*stream_id*/, + size_t padding_length) { + QUICHE_DCHECK_GE(remaining_data_, padding_length); + current_frame_.data.padlen = padding_length; + remaining_data_ -= padding_length; + if (remaining_data_ == 0 && callbacks_->on_frame_recv_callback != nullptr) { + const int result = callbacks_->on_frame_recv_callback( + nullptr, ¤t_frame_, user_data_); + return result == 0; + } + return true; +} + bool CallbackVisitor::OnBeginDataForStream(Http2StreamId /*stream_id*/, size_t payload_length) { - // TODO(b/181586191): Interpret padding, subtract padding from - // |remaining_data_|. remaining_data_ = payload_length; - if (remaining_data_ == 0 && callbacks_->on_frame_recv_callback) { + if (remaining_data_ == 0 && callbacks_->on_frame_recv_callback != nullptr) { const int result = callbacks_->on_frame_recv_callback( nullptr, ¤t_frame_, user_data_); return result == 0;
diff --git a/http2/adapter/callback_visitor.h b/http2/adapter/callback_visitor.h index 6d1915c..fa0c770 100644 --- a/http2/adapter/callback_visitor.h +++ b/http2/adapter/callback_visitor.h
@@ -35,6 +35,8 @@ absl::string_view name, absl::string_view value) override; bool OnEndHeadersForStream(Http2StreamId stream_id) override; + bool OnDataPaddingLength(Http2StreamId stream_id, + size_t padding_length) override; bool OnBeginDataForStream(Http2StreamId stream_id, size_t payload_length) override; bool OnDataForStream(Http2StreamId stream_id,
diff --git a/http2/adapter/callback_visitor_test.cc b/http2/adapter/callback_visitor_test.cc index f7b982e..9ed28f0 100644 --- a/http2/adapter/callback_visitor_test.cc +++ b/http2/adapter/callback_visitor_test.cc
@@ -291,6 +291,72 @@ visitor.OnCloseStream(3, Http2ErrorCode::INTERNAL_ERROR); } +TEST(ServerCallbackVisitorUnitTest, DataWithPadding) { + testing::StrictMock<MockNghttp2Callbacks> callbacks; + CallbackVisitor visitor(Perspective::kServer, + *MockNghttp2Callbacks::GetCallbacks(), &callbacks); + + const size_t kPaddingLength = 39; + const uint8_t kFlags = NGHTTP2_FLAG_PADDED | NGHTTP2_FLAG_END_STREAM; + + testing::InSequence seq; + + // DATA on stream 1 + EXPECT_CALL(callbacks, OnBeginFrame(HasFrameHeader(1, DATA, kFlags))); + EXPECT_TRUE(visitor.OnFrameHeader(1, 25 + kPaddingLength, DATA, kFlags)); + + EXPECT_TRUE(visitor.OnBeginDataForStream(1, 25 + kPaddingLength)); + + // Padding before data. + EXPECT_TRUE(visitor.OnDataPaddingLength(1, kPaddingLength)); + + EXPECT_CALL(callbacks, + OnDataChunkRecv(kFlags, 1, "This is the request body.")); + EXPECT_CALL(callbacks, OnFrameRecv(IsData(1, _, kFlags, kPaddingLength))); + EXPECT_TRUE(visitor.OnDataForStream(1, "This is the request body.")); + visitor.OnEndStream(1); + + EXPECT_CALL(callbacks, OnStreamClose(1, NGHTTP2_NO_ERROR)); + visitor.OnCloseStream(1, Http2ErrorCode::HTTP2_NO_ERROR); + + // DATA on stream 3 + EXPECT_CALL(callbacks, OnBeginFrame(HasFrameHeader(3, DATA, kFlags))); + EXPECT_TRUE(visitor.OnFrameHeader(3, 25 + kPaddingLength, DATA, kFlags)); + + EXPECT_TRUE(visitor.OnBeginDataForStream(3, 25 + kPaddingLength)); + + // Data before padding. + EXPECT_CALL(callbacks, + OnDataChunkRecv(kFlags, 3, "This is the request body.")); + EXPECT_TRUE(visitor.OnDataForStream(3, "This is the request body.")); + + EXPECT_CALL(callbacks, OnFrameRecv(IsData(3, _, kFlags, kPaddingLength))); + EXPECT_TRUE(visitor.OnDataPaddingLength(3, kPaddingLength)); + visitor.OnEndStream(3); + + EXPECT_CALL(callbacks, OnStreamClose(3, NGHTTP2_NO_ERROR)); + visitor.OnCloseStream(3, Http2ErrorCode::HTTP2_NO_ERROR); + + // DATA on stream 5 + EXPECT_CALL(callbacks, OnBeginFrame(HasFrameHeader(5, DATA, kFlags))); + EXPECT_TRUE(visitor.OnFrameHeader(5, 25 + kPaddingLength, DATA, kFlags)); + + EXPECT_TRUE(visitor.OnBeginDataForStream(5, 25 + kPaddingLength)); + + // Error during padding. + EXPECT_CALL(callbacks, + OnDataChunkRecv(kFlags, 5, "This is the request body.")); + EXPECT_TRUE(visitor.OnDataForStream(5, "This is the request body.")); + + EXPECT_CALL(callbacks, OnFrameRecv(IsData(5, _, kFlags, kPaddingLength))) + .WillOnce(testing::Return(NGHTTP2_ERR_CALLBACK_FAILURE)); + EXPECT_FALSE(visitor.OnDataPaddingLength(5, kPaddingLength)); + visitor.OnEndStream(3); + + EXPECT_CALL(callbacks, OnStreamClose(5, NGHTTP2_NO_ERROR)); + visitor.OnCloseStream(5, Http2ErrorCode::HTTP2_NO_ERROR); +} + } // namespace } // namespace test } // namespace adapter
diff --git a/http2/adapter/http2_visitor_interface.h b/http2/adapter/http2_visitor_interface.h index 0e8619f..574bac4 100644 --- a/http2/adapter/http2_visitor_interface.h +++ b/http2/adapter/http2_visitor_interface.h
@@ -144,6 +144,12 @@ virtual bool OnBeginDataForStream(Http2StreamId stream_id, size_t payload_length) = 0; + // Called when the optional padding length field is parsed as part of a DATA + // frame payload. `padding_length` represents the total amount of padding for + // this frame, including the length byte itself. + virtual bool OnDataPaddingLength(Http2StreamId stream_id, + size_t padding_length) = 0; + // Called when the connection receives some |data| (as part of a DATA frame // payload) for a stream. virtual bool OnDataForStream(Http2StreamId stream_id,
diff --git a/http2/adapter/mock_http2_visitor.h b/http2/adapter/mock_http2_visitor.h index 7454d2f..96f0978 100644 --- a/http2/adapter/mock_http2_visitor.h +++ b/http2/adapter/mock_http2_visitor.h
@@ -20,6 +20,7 @@ .WillByDefault(testing::Return(true)); ON_CALL(*this, OnHeaderForStream).WillByDefault(testing::Return(HEADER_OK)); ON_CALL(*this, OnEndHeadersForStream).WillByDefault(testing::Return(true)); + ON_CALL(*this, OnDataPaddingLength).WillByDefault(testing::Return(true)); ON_CALL(*this, OnBeginDataForStream).WillByDefault(testing::Return(true)); ON_CALL(*this, OnDataForStream).WillByDefault(testing::Return(true)); ON_CALL(*this, OnGoAway).WillByDefault(testing::Return(true)); @@ -50,6 +51,9 @@ MOCK_METHOD(bool, OnEndHeadersForStream, (Http2StreamId stream_id), (override)); + MOCK_METHOD(bool, OnDataPaddingLength, + (Http2StreamId strema_id, size_t padding_length), (override)); + MOCK_METHOD(bool, OnBeginDataForStream, (Http2StreamId stream_id, size_t payload_length), (override));
diff --git a/http2/adapter/nghttp2_adapter_test.cc b/http2/adapter/nghttp2_adapter_test.cc index a2fd68f..0b570c3 100644 --- a/http2/adapter/nghttp2_adapter_test.cc +++ b/http2/adapter/nghttp2_adapter_test.cc
@@ -2547,6 +2547,63 @@ spdy::SpdyFrameType::PING})); } +TEST(NgHttp2AdapterTest, ServerHandlesDataWithPadding) { + DataSavingVisitor visitor; + auto adapter = NgHttp2Adapter::CreateServerAdapter(visitor); + + const std::string frames = TestFrameSequence() + .ClientPreface() + .Headers(1, + {{":method", "POST"}, + {":scheme", "https"}, + {":authority", "example.com"}, + {":path", "/this/is/request/one"}}, + /*fin=*/false) + .Data(1, "This is the request body.", + /*fin=*/true, /*padding_length=*/39) + .Headers(3, + {{":method", "GET"}, + {":scheme", "http"}, + {":authority", "example.com"}, + {":path", "/this/is/request/two"}}, + /*fin=*/true) + .Serialize(); + testing::InSequence s; + + // Client preface (empty SETTINGS) + EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0)); + EXPECT_CALL(visitor, OnSettingsStart()); + EXPECT_CALL(visitor, OnSettingsEnd()); + + EXPECT_CALL(visitor, OnFrameHeader(1, _, HEADERS, 4)); + EXPECT_CALL(visitor, OnBeginHeadersForStream(1)); + EXPECT_CALL(visitor, OnHeaderForStream(1, _, _)).Times(4); + EXPECT_CALL(visitor, OnEndHeadersForStream(1)); + EXPECT_CALL(visitor, OnFrameHeader(1, 25 + 39, DATA, 0x9)); + EXPECT_CALL(visitor, OnBeginDataForStream(1, 25 + 39)); + EXPECT_CALL(visitor, OnDataForStream(1, "This is the request body.")); + // Note: nghttp2 passes padding information after the actual data. + EXPECT_CALL(visitor, OnDataPaddingLength(1, 39)); + EXPECT_CALL(visitor, OnEndStream(1)); + EXPECT_CALL(visitor, OnFrameHeader(3, _, HEADERS, 5)); + EXPECT_CALL(visitor, OnBeginHeadersForStream(3)); + EXPECT_CALL(visitor, OnHeaderForStream(3, _, _)).Times(4); + EXPECT_CALL(visitor, OnEndHeadersForStream(3)); + EXPECT_CALL(visitor, OnEndStream(3)); + + const int64_t result = adapter->ProcessBytes(frames); + EXPECT_EQ(frames.size(), result); + + EXPECT_TRUE(adapter->want_write()); + + EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x1)); + EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x1, 0)); + + int send_result = adapter->Send(); + EXPECT_EQ(0, send_result); + EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS})); +} + // Tests the case where the response body is in the progress of being sent while // trailers are queued. TEST(NgHttp2AdapterTest, ServerSubmitsTrailersWhileDataDeferred) {
diff --git a/http2/adapter/nghttp2_callbacks.cc b/http2/adapter/nghttp2_callbacks.cc index d96214d..fee8ffb 100644 --- a/http2/adapter/nghttp2_callbacks.cc +++ b/http2/adapter/nghttp2_callbacks.cc
@@ -63,6 +63,9 @@ // callbacks. This callback handles the point at which the entire logical // frame has been received and processed. case NGHTTP2_DATA: + if ((frame->hd.flags & NGHTTP2_FLAG_PADDED) != 0) { + visitor->OnDataPaddingLength(stream_id, frame->data.padlen); + } if (frame->hd.flags & NGHTTP2_FLAG_END_STREAM) { visitor->OnEndStream(stream_id); }
diff --git a/http2/adapter/nghttp2_test_utils.cc b/http2/adapter/nghttp2_test_utils.cc index d9a04f2..6e4b5b2 100644 --- a/http2/adapter/nghttp2_test_utils.cc +++ b/http2/adapter/nghttp2_test_utils.cc
@@ -101,8 +101,12 @@ public: DataMatcher(const testing::Matcher<uint32_t> stream_id, const testing::Matcher<size_t> length, - const testing::Matcher<int> flags) - : stream_id_(stream_id), length_(length), flags_(flags) {} + const testing::Matcher<int> flags, + const testing::Matcher<size_t> padding) + : stream_id_(stream_id), + length_(length), + flags_(flags), + padding_(padding) {} bool MatchAndExplain(const nghttp2_frame* frame, testing::MatchResultListener* listener) const override { @@ -121,6 +125,9 @@ if (!flags_.MatchAndExplain(frame->hd.flags, listener)) { matched = false; } + if (!padding_.MatchAndExplain(frame->data.padlen, listener)) { + matched = false; + } return matched; } @@ -142,6 +149,7 @@ const testing::Matcher<uint32_t> stream_id_; const testing::Matcher<size_t> length_; const testing::Matcher<int> flags_; + const testing::Matcher<size_t> padding_; }; class HeadersMatcher : public testing::MatcherInterface<const nghttp2_frame*> { @@ -405,8 +413,9 @@ testing::Matcher<const nghttp2_frame*> IsData( const testing::Matcher<uint32_t> stream_id, - const testing::Matcher<size_t> length, const testing::Matcher<int> flags) { - return MakeMatcher(new DataMatcher(stream_id, length, flags)); + const testing::Matcher<size_t> length, const testing::Matcher<int> flags, + const testing::Matcher<size_t> padding) { + return MakeMatcher(new DataMatcher(stream_id, length, flags, padding)); } testing::Matcher<const nghttp2_frame*> IsHeaders(
diff --git a/http2/adapter/nghttp2_test_utils.h b/http2/adapter/nghttp2_test_utils.h index 92632dc..24493d7 100644 --- a/http2/adapter/nghttp2_test_utils.h +++ b/http2/adapter/nghttp2_test_utils.h
@@ -66,7 +66,8 @@ testing::Matcher<const nghttp2_frame*> IsData( const testing::Matcher<uint32_t> stream_id, - const testing::Matcher<size_t> length, const testing::Matcher<int> flags); + const testing::Matcher<size_t> length, const testing::Matcher<int> flags, + const testing::Matcher<size_t> padding = testing::_); testing::Matcher<const nghttp2_frame*> IsHeaders( const testing::Matcher<uint32_t> stream_id,
diff --git a/http2/adapter/oghttp2_adapter_test.cc b/http2/adapter/oghttp2_adapter_test.cc index 7917bf2..474a86c 100644 --- a/http2/adapter/oghttp2_adapter_test.cc +++ b/http2/adapter/oghttp2_adapter_test.cc
@@ -3351,6 +3351,67 @@ EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::HEADERS})); } +TEST(OgHttp2AdapterServerTest, ServerHandlesDataWithPadding) { + DataSavingVisitor visitor; + OgHttp2Adapter::Options options{.perspective = Perspective::kServer}; + auto adapter = OgHttp2Adapter::Create(visitor, options); + + const std::string frames = TestFrameSequence() + .ClientPreface() + .Headers(1, + {{":method", "POST"}, + {":scheme", "https"}, + {":authority", "example.com"}, + {":path", "/this/is/request/one"}}, + /*fin=*/false) + .Data(1, "This is the request body.", + /*fin=*/true, /*padding_length=*/39) + .Headers(3, + {{":method", "GET"}, + {":scheme", "http"}, + {":authority", "example.com"}, + {":path", "/this/is/request/two"}}, + /*fin=*/true) + .Serialize(); + testing::InSequence s; + + // Client preface (empty SETTINGS) + EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0)); + EXPECT_CALL(visitor, OnSettingsStart()); + EXPECT_CALL(visitor, OnSettingsEnd()); + + EXPECT_CALL(visitor, OnFrameHeader(1, _, HEADERS, 4)); + EXPECT_CALL(visitor, OnBeginHeadersForStream(1)); + EXPECT_CALL(visitor, OnHeaderForStream(1, _, _)).Times(4); + EXPECT_CALL(visitor, OnEndHeadersForStream(1)); + EXPECT_CALL(visitor, OnFrameHeader(1, 25 + 39, DATA, 0x9)); + EXPECT_CALL(visitor, OnBeginDataForStream(1, 25 + 39)); + // Note: oghttp2 passes padding information before the actual data. + EXPECT_CALL(visitor, OnDataPaddingLength(1, 39)); + EXPECT_CALL(visitor, OnDataForStream(1, "This is the request body.")); + EXPECT_CALL(visitor, OnEndStream(1)); + EXPECT_CALL(visitor, OnFrameHeader(3, _, HEADERS, 5)); + EXPECT_CALL(visitor, OnBeginHeadersForStream(3)); + EXPECT_CALL(visitor, OnHeaderForStream(3, _, _)).Times(4); + EXPECT_CALL(visitor, OnEndHeadersForStream(3)); + EXPECT_CALL(visitor, OnEndStream(3)); + + const int64_t result = adapter->ProcessBytes(frames); + EXPECT_EQ(static_cast<int64_t>(frames.size()), result); + + EXPECT_TRUE(adapter->want_write()); + + EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x0)); + EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x0, 0)); + EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x1)); + EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x1, 0)); + + int send_result = adapter->Send(); + EXPECT_EQ(0, send_result); + EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS, + spdy::SpdyFrameType::SETTINGS})); +} + // Tests the case where the response body is in the progress of being sent while // trailers are queued. TEST(OgHttp2AdapterServerTest, ServerSubmitsTrailersWhileDataDeferred) {
diff --git a/http2/adapter/oghttp2_session.cc b/http2/adapter/oghttp2_session.cc index 5aea9f7..a0c2b6f 100644 --- a/http2/adapter/oghttp2_session.cc +++ b/http2/adapter/oghttp2_session.cc
@@ -1106,8 +1106,12 @@ void OgHttp2Session::OnStreamPadLength(spdy::SpdyStreamId stream_id, size_t value) { + bool result = visitor_.OnDataPaddingLength(stream_id, 1 + value); + if (!result) { + fatal_visitor_callback_failure_ = true; + decoder_.StopProcessing(); + } MarkDataBuffered(stream_id, 1 + value); - // TODO(181586191): Pass padding to the visitor? } void OgHttp2Session::OnStreamPadding(spdy::SpdyStreamId /*stream_id*/, size_t
diff --git a/http2/adapter/recording_http2_visitor.cc b/http2/adapter/recording_http2_visitor.cc index 74db309..b9cbcfb 100644 --- a/http2/adapter/recording_http2_visitor.cc +++ b/http2/adapter/recording_http2_visitor.cc
@@ -60,6 +60,13 @@ return true; } +bool RecordingHttp2Visitor::OnDataPaddingLength(Http2StreamId stream_id, + size_t padding_length) { + events_.push_back( + absl::StrFormat("OnDataPaddingLength %d %d", stream_id, padding_length)); + return true; +} + bool RecordingHttp2Visitor::OnBeginDataForStream(Http2StreamId stream_id, size_t payload_length) { events_.push_back(
diff --git a/http2/adapter/recording_http2_visitor.h b/http2/adapter/recording_http2_visitor.h index 6008725..3ded946 100644 --- a/http2/adapter/recording_http2_visitor.h +++ b/http2/adapter/recording_http2_visitor.h
@@ -33,6 +33,8 @@ absl::string_view name, absl::string_view value) override; bool OnEndHeadersForStream(Http2StreamId stream_id) override; + bool OnDataPaddingLength(Http2StreamId stream_id, + size_t padding_length) override; bool OnBeginDataForStream(Http2StreamId stream_id, size_t payload_length) override; bool OnDataForStream(Http2StreamId stream_id,