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,
