Improve handing of frames with empty payload in HttpDecoder.
The current draft allows HTTP frames to have an empty payload. Before this CL, there were some issues with our implementation:
(1) Frames with empty payload were not processed until data from the next frame arrived.
(2) OnDataFrameStart() and OnHeadersFrameStart() were only called after some more data (frame payload or next frame) arrived, even though all necessary argument values were already available when the frame header was processed.
(3) If a DATA or HEADERS or PUSH_PROMISE frame had no payload, On*FramePayload() was called once with an empty string argument.
This CL adds one more state to the state machine, and fixes these issues. It also documents that the |payload| argument of On*FramePayload() cannot be empty by adding some DCHECKs.
gfe-relnote: n/a. Change in QUIC v99 only, code path not used in production.
PiperOrigin-RevId: 238811718
Change-Id: I020dd16ebdb529a77fb2fa7969a65ceede667530
diff --git a/quic/core/http/http_decoder.cc b/quic/core/http/http_decoder.cc
index 7bccbbc..01d5e4b 100644
--- a/quic/core/http/http_decoder.cc
+++ b/quic/core/http/http_decoder.cc
@@ -3,6 +3,9 @@
// found in the LICENSE file.
#include "net/third_party/quiche/src/quic/core/http/http_decoder.h"
+
+#include <type_traits>
+
#include "net/third_party/quiche/src/quic/core/quic_data_reader.h"
#include "net/third_party/quiche/src/quic/platform/api/quic_bug_tracker.h"
#include "net/third_party/quiche/src/quic/platform/api/quic_fallthrough.h"
@@ -41,7 +44,8 @@
QuicByteCount HttpDecoder::ProcessInput(const char* data, QuicByteCount len) {
QuicDataReader reader(data, len);
- while (error_ == QUIC_NO_ERROR && reader.BytesRemaining() != 0) {
+ while (error_ == QUIC_NO_ERROR &&
+ (reader.BytesRemaining() != 0 || state_ == STATE_FINISH_PARSING)) {
switch (state_) {
case STATE_READING_FRAME_LENGTH:
ReadFrameLength(&reader);
@@ -52,6 +56,9 @@
case STATE_READING_FRAME_PAYLOAD:
ReadFramePayload(&reader);
break;
+ case STATE_FINISH_PARSING:
+ FinishParsing();
+ break;
case STATE_ERROR:
break;
default:
@@ -91,18 +98,25 @@
return;
}
- state_ = STATE_READING_FRAME_PAYLOAD;
+ // Calling the following two visitor methods does not require parsing of any
+ // frame payload.
+ if (current_frame_type_ == 0x0) {
+ visitor_->OnDataFrameStart(Http3FrameLengths(
+ current_length_field_size_ + kFrameTypeLength, current_frame_length_));
+ } else if (current_frame_type_ == 0x1) {
+ visitor_->OnHeadersFrameStart(Http3FrameLengths(
+ current_length_field_size_ + kFrameTypeLength, current_frame_length_));
+ }
+
+ state_ = (remaining_frame_length_ == 0) ? STATE_FINISH_PARSING
+ : STATE_READING_FRAME_PAYLOAD;
}
void HttpDecoder::ReadFramePayload(QuicDataReader* reader) {
DCHECK_NE(0u, reader->BytesRemaining());
+ DCHECK_NE(0u, remaining_frame_length_);
switch (current_frame_type_) {
case 0x0: { // DATA
- if (current_frame_length_ == remaining_frame_length_) {
- visitor_->OnDataFrameStart(
- Http3FrameLengths(current_length_field_size_ + kFrameTypeLength,
- current_frame_length_));
- }
QuicByteCount bytes_to_read = std::min<QuicByteCount>(
remaining_frame_length_, reader->BytesRemaining());
QuicStringPiece payload;
@@ -110,21 +124,12 @@
RaiseError(QUIC_INTERNAL_ERROR, "Unable to read data");
return;
}
+ DCHECK(!payload.empty());
visitor_->OnDataFramePayload(payload);
remaining_frame_length_ -= payload.length();
- if (remaining_frame_length_ == 0) {
- state_ = STATE_READING_FRAME_LENGTH;
- current_length_field_size_ = 0;
- visitor_->OnDataFrameEnd();
- }
- return;
+ break;
}
case 0x1: { // HEADERS
- if (current_frame_length_ == remaining_frame_length_) {
- visitor_->OnHeadersFrameStart(
- Http3FrameLengths(current_length_field_size_ + kFrameTypeLength,
- current_frame_length_));
- }
QuicByteCount bytes_to_read = std::min<QuicByteCount>(
remaining_frame_length_, reader->BytesRemaining());
QuicStringPiece payload;
@@ -132,46 +137,21 @@
RaiseError(QUIC_INTERNAL_ERROR, "Unable to read data");
return;
}
+ DCHECK(!payload.empty());
visitor_->OnHeadersFramePayload(payload);
remaining_frame_length_ -= payload.length();
- if (remaining_frame_length_ == 0) {
- state_ = STATE_READING_FRAME_LENGTH;
- current_length_field_size_ = 0;
- visitor_->OnHeadersFrameEnd(current_frame_length_);
- }
- return;
+ break;
}
case 0x2: { // PRIORITY
// TODO(rch): avoid buffering if the entire frame is present, and
// instead parse directly out of |reader|.
BufferFramePayload(reader);
- if (remaining_frame_length_ == 0) {
- PriorityFrame frame;
- QuicDataReader reader(buffer_.data(), current_frame_length_);
- if (!ParsePriorityFrame(&reader, &frame)) {
- return;
- }
- visitor_->OnPriorityFrame(frame);
- state_ = STATE_READING_FRAME_LENGTH;
- current_length_field_size_ = 0;
- }
- return;
+ break;
}
case 0x3: { // CANCEL_PUSH
// TODO(rch): Handle partial delivery.
BufferFramePayload(reader);
- if (remaining_frame_length_ == 0) {
- CancelPushFrame frame;
- QuicDataReader reader(buffer_.data(), current_frame_length_);
- if (!reader.ReadVarInt62(&frame.push_id)) {
- RaiseError(QUIC_INTERNAL_ERROR, "Unable to read push_id");
- return;
- }
- visitor_->OnCancelPushFrame(frame);
- state_ = STATE_READING_FRAME_LENGTH;
- current_length_field_size_ = 0;
- }
- return;
+ break;
}
case 0x4: { // SETTINGS
// TODO(rch): Handle overly large SETTINGS frames. Either:
@@ -179,17 +159,7 @@
// exceeded
// 2. Implement a streaming parsing mode.
BufferFramePayload(reader);
- if (remaining_frame_length_ == 0) {
- SettingsFrame frame;
- QuicDataReader reader(buffer_.data(), current_frame_length_);
- if (!ParseSettingsFrame(&reader, &frame)) {
- return;
- }
- visitor_->OnSettingsFrame(frame);
- state_ = STATE_READING_FRAME_LENGTH;
- current_length_field_size_ = 0;
- }
- return;
+ break;
}
case 0x5: { // PUSH_PROMISE
if (current_frame_length_ == remaining_frame_length_) {
@@ -203,75 +173,36 @@
remaining_frame_length_ -= bytes_remaining - reader->BytesRemaining();
visitor_->OnPushPromiseFrameStart(push_id);
}
+ DCHECK_LT(remaining_frame_length_, current_frame_length_);
QuicByteCount bytes_to_read = std::min<QuicByteCount>(
remaining_frame_length_, reader->BytesRemaining());
if (bytes_to_read == 0) {
- return;
+ break;
}
QuicStringPiece payload;
if (!reader->ReadStringPiece(&payload, bytes_to_read)) {
RaiseError(QUIC_INTERNAL_ERROR, "Unable to read data");
return;
}
+ DCHECK(!payload.empty());
visitor_->OnPushPromiseFramePayload(payload);
remaining_frame_length_ -= payload.length();
- if (remaining_frame_length_ == 0) {
- state_ = STATE_READING_FRAME_LENGTH;
- current_length_field_size_ = 0;
- visitor_->OnPushPromiseFrameEnd();
- }
- return;
+ break;
}
case 0x7: { // GOAWAY
BufferFramePayload(reader);
- if (remaining_frame_length_ == 0) {
- GoAwayFrame frame;
- QuicDataReader reader(buffer_.data(), current_frame_length_);
- uint64_t stream_id;
- if (!reader.ReadVarInt62(&stream_id)) {
- RaiseError(QUIC_INTERNAL_ERROR, "Unable to read GOAWAY stream_id");
- return;
- }
- frame.stream_id = stream_id;
- visitor_->OnGoAwayFrame(frame);
- state_ = STATE_READING_FRAME_LENGTH;
- current_length_field_size_ = 0;
- }
- return;
+ break;
}
case 0xD: { // MAX_PUSH_ID
// TODO(rch): Handle partial delivery.
BufferFramePayload(reader);
- if (remaining_frame_length_ == 0) {
- QuicDataReader reader(buffer_.data(), current_frame_length_);
- MaxPushIdFrame frame;
- if (!reader.ReadVarInt62(&frame.push_id)) {
- RaiseError(QUIC_INTERNAL_ERROR, "Unable to read push_id");
- return;
- }
- visitor_->OnMaxPushIdFrame(frame);
- state_ = STATE_READING_FRAME_LENGTH;
- current_length_field_size_ = 0;
- }
- return;
+ break;
}
case 0xE: { // DUPLICATE_PUSH
BufferFramePayload(reader);
- if (remaining_frame_length_ != 0) {
- return;
- }
- QuicDataReader reader(buffer_.data(), current_frame_length_);
- DuplicatePushFrame frame;
- if (!reader.ReadVarInt62(&frame.push_id)) {
- RaiseError(QUIC_INTERNAL_ERROR, "Unable to read push_id");
- return;
- }
- visitor_->OnDuplicatePushFrame(frame);
- state_ = STATE_READING_FRAME_LENGTH;
- current_length_field_size_ = 0;
- return;
+ break;
}
// Reserved frame types.
// TODO(rch): Since these are actually the same behavior as the
@@ -295,6 +226,104 @@
default:
DiscardFramePayload(reader);
}
+
+ if (remaining_frame_length_ == 0) {
+ state_ = STATE_FINISH_PARSING;
+ }
+}
+
+void HttpDecoder::FinishParsing() {
+ DCHECK_EQ(0u, remaining_frame_length_);
+ switch (current_frame_type_) {
+ case 0x0: { // DATA
+ visitor_->OnDataFrameEnd();
+ break;
+ }
+ case 0x1: { // HEADERS
+ visitor_->OnHeadersFrameEnd(current_frame_length_);
+ break;
+ }
+ case 0x2: { // PRIORITY
+ // TODO(rch): avoid buffering if the entire frame is present, and
+ // instead parse directly out of |reader|.
+ PriorityFrame frame;
+ QuicDataReader reader(buffer_.data(), current_frame_length_);
+ if (!ParsePriorityFrame(&reader, &frame)) {
+ return;
+ }
+ visitor_->OnPriorityFrame(frame);
+ break;
+ }
+ case 0x3: { // CANCEL_PUSH
+ // TODO(rch): Handle partial delivery.
+ CancelPushFrame frame;
+ QuicDataReader reader(buffer_.data(), current_frame_length_);
+ if (!reader.ReadVarInt62(&frame.push_id)) {
+ RaiseError(QUIC_INTERNAL_ERROR, "Unable to read push_id");
+ return;
+ }
+ visitor_->OnCancelPushFrame(frame);
+ break;
+ }
+ case 0x4: { // SETTINGS
+ // TODO(rch): Handle overly large SETTINGS frames. Either:
+ // 1. Impose a limit on SETTINGS frame size, and close the connection if
+ // exceeded
+ // 2. Implement a streaming parsing mode.
+ SettingsFrame frame;
+ QuicDataReader reader(buffer_.data(), current_frame_length_);
+ if (!ParseSettingsFrame(&reader, &frame)) {
+ return;
+ }
+ visitor_->OnSettingsFrame(frame);
+ break;
+ }
+ case 0x5: { // PUSH_PROMISE
+ visitor_->OnPushPromiseFrameEnd();
+ break;
+ }
+ case 0x7: { // GOAWAY
+ QuicDataReader reader(buffer_.data(), current_frame_length_);
+ GoAwayFrame frame;
+ static_assert(!std::is_same<decltype(frame.stream_id), uint64_t>::value,
+ "Please remove local |stream_id| variable and pass "
+ "&frame.stream_id directly to ReadVarInt62() when changing "
+ "QuicStreamId from uint32_t to uint64_t.");
+ uint64_t stream_id;
+ if (!reader.ReadVarInt62(&stream_id)) {
+ RaiseError(QUIC_INTERNAL_ERROR, "Unable to read GOAWAY stream_id");
+ return;
+ }
+ frame.stream_id = static_cast<QuicStreamId>(stream_id);
+ visitor_->OnGoAwayFrame(frame);
+ break;
+ }
+
+ case 0xD: { // MAX_PUSH_ID
+ QuicDataReader reader(buffer_.data(), current_frame_length_);
+ MaxPushIdFrame frame;
+ if (!reader.ReadVarInt62(&frame.push_id)) {
+ RaiseError(QUIC_INTERNAL_ERROR, "Unable to read push_id");
+ return;
+ }
+ visitor_->OnMaxPushIdFrame(frame);
+ break;
+ }
+
+ case 0xE: { // DUPLICATE_PUSH
+ QuicDataReader reader(buffer_.data(), current_frame_length_);
+ DuplicatePushFrame frame;
+ if (!reader.ReadVarInt62(&frame.push_id)) {
+ RaiseError(QUIC_INTERNAL_ERROR, "Unable to read push_id");
+ return;
+ }
+ visitor_->OnDuplicatePushFrame(frame);
+ break;
+ }
+ }
+
+ current_length_field_size_ = 0;
+ state_ = STATE_READING_FRAME_LENGTH;
}
void HttpDecoder::DiscardFramePayload(QuicDataReader* reader) {
diff --git a/quic/core/http/http_decoder.h b/quic/core/http/http_decoder.h
index cbfa65e..1cc973a 100644
--- a/quic/core/http/http_decoder.h
+++ b/quic/core/http/http_decoder.h
@@ -65,8 +65,9 @@
// Called when a DATA frame has been received.
// |frame_length| contains DATA frame length and payload length.
virtual void OnDataFrameStart(Http3FrameLengths frame_length) = 0;
- // Called when the payload of a DATA frame has read. May be called
- // multiple times for a single frame.
+ // Called when part of the payload of a DATA frame has been read. May be
+ // called multiple times for a single frame. |payload| is guaranteed to be
+ // non-empty.
virtual void OnDataFramePayload(QuicStringPiece payload) = 0;
// Called when a DATA frame has been completely processed.
virtual void OnDataFrameEnd() = 0;
@@ -74,8 +75,9 @@
// Called when a HEADERS frame has been recevied.
// |frame_length| contains HEADERS frame length and payload length.
virtual void OnHeadersFrameStart(Http3FrameLengths frame_length) = 0;
- // Called when the payload of a HEADERS frame has read. May be called
- // multiple times for a single frame.
+ // Called when part of the payload of a HEADERS frame has been read. May be
+ // called multiple times for a single frame. |payload| is guaranteed to be
+ // non-empty.
virtual void OnHeadersFramePayload(QuicStringPiece payload) = 0;
// Called when a HEADERS frame has been completely processed.
// |frame_len| is the length of the HEADERS frame payload.
@@ -83,8 +85,9 @@
// Called when a PUSH_PROMISE frame has been recevied for |push_id|.
virtual void OnPushPromiseFrameStart(PushId push_id) = 0;
- // Called when the payload of a PUSH_PROMISE frame has read. May be called
- // multiple times for a single frame.
+ // Called when part of the payload of a PUSH_PROMISE frame has been read.
+ // May be called multiple times for a single frame. |payload| is guaranteed
+ // to be non-empty.
virtual void OnPushPromiseFramePayload(QuicStringPiece payload) = 0;
// Called when a PUSH_PROMISE frame has been completely processed.
virtual void OnPushPromiseFrameEnd() = 0;
@@ -118,6 +121,7 @@
STATE_READING_FRAME_LENGTH,
STATE_READING_FRAME_TYPE,
STATE_READING_FRAME_PAYLOAD,
+ STATE_FINISH_PARSING,
STATE_ERROR
};
@@ -126,13 +130,18 @@
void ReadFrameLength(QuicDataReader* reader);
// Reads the type of a frame from |reader|. Sets error_ and error_detail_
- // if there are any errors.
+ // if there are any errors. Also calls OnDataFrameStart() or
+ // OnHeadersFrameStart() for appropriate frame types.
void ReadFrameType(QuicDataReader* reader);
// Reads the payload of the current frame from |reader| and processes it,
// possibly buffering the data or invoking the visitor.
void ReadFramePayload(QuicDataReader* reader);
+ // Optionally parses buffered data; calls visitor method to signal that frame
+ // had been parsed completely.
+ void FinishParsing();
+
// Discards any remaining frame payload from |reader|.
void DiscardFramePayload(QuicDataReader* reader);
diff --git a/quic/core/http/http_decoder_test.cc b/quic/core/http/http_decoder_test.cc
index 5e620cb..53b5f86 100644
--- a/quic/core/http/http_decoder_test.cc
+++ b/quic/core/http/http_decoder_test.cc
@@ -333,13 +333,13 @@
EXPECT_EQ("", decoder_.error_detail());
// Send the rest of the header.
+ EXPECT_CALL(visitor_, OnDataFrameStart(Http3FrameLengths(3, 2048)));
EXPECT_EQ(header_length - 1,
decoder_.ProcessInput(header.data() + 1, header_length - 1));
EXPECT_EQ(QUIC_NO_ERROR, decoder_.error());
EXPECT_EQ("", decoder_.error_detail());
// Send data.
- EXPECT_CALL(visitor_, OnDataFrameStart(Http3FrameLengths(3, 2048)));
EXPECT_CALL(visitor_, OnDataFramePayload(QuicStringPiece(input)));
EXPECT_CALL(visitor_, OnDataFrameEnd());
EXPECT_EQ(2048u, decoder_.ProcessInput(input.data(), 2048));
@@ -406,4 +406,74 @@
EXPECT_EQ("", decoder_.error_detail());
}
+TEST_F(HttpDecoderTest, EmptyDataFrame) {
+ char input[] = {0x00, // length
+ 0x00}; // type (DATA)
+
+ // Process the full frame.
+ InSequence s;
+ EXPECT_CALL(visitor_, OnDataFrameStart(Http3FrameLengths(2, 0)));
+ EXPECT_CALL(visitor_, OnDataFrameEnd());
+ EXPECT_EQ(QUIC_ARRAYSIZE(input),
+ decoder_.ProcessInput(input, QUIC_ARRAYSIZE(input)));
+ EXPECT_EQ(QUIC_NO_ERROR, decoder_.error());
+ EXPECT_EQ("", decoder_.error_detail());
+
+ // Process the frame incremently.
+ EXPECT_CALL(visitor_, OnDataFrameStart(Http3FrameLengths(2, 0)));
+ EXPECT_CALL(visitor_, OnDataFrameEnd());
+ for (char c : input) {
+ EXPECT_EQ(1u, decoder_.ProcessInput(&c, 1));
+ }
+ EXPECT_EQ(QUIC_NO_ERROR, decoder_.error());
+ EXPECT_EQ("", decoder_.error_detail());
+}
+
+TEST_F(HttpDecoderTest, EmptyHeadersFrame) {
+ char input[] = {0x00, // length
+ 0x01}; // type (HEADERS)
+
+ // Process the full frame.
+ InSequence s;
+ EXPECT_CALL(visitor_, OnHeadersFrameStart(Http3FrameLengths(2, 0)));
+ EXPECT_CALL(visitor_, OnHeadersFrameEnd(0));
+ EXPECT_EQ(QUIC_ARRAYSIZE(input),
+ decoder_.ProcessInput(input, QUIC_ARRAYSIZE(input)));
+ EXPECT_EQ(QUIC_NO_ERROR, decoder_.error());
+ EXPECT_EQ("", decoder_.error_detail());
+
+ // Process the frame incremently.
+ EXPECT_CALL(visitor_, OnHeadersFrameStart(Http3FrameLengths(2, 0)));
+ EXPECT_CALL(visitor_, OnHeadersFrameEnd(0));
+ for (char c : input) {
+ EXPECT_EQ(1u, decoder_.ProcessInput(&c, 1));
+ }
+ EXPECT_EQ(QUIC_NO_ERROR, decoder_.error());
+ EXPECT_EQ("", decoder_.error_detail());
+}
+
+TEST_F(HttpDecoderTest, PushPromiseFrameNoHeaders) {
+ char input[] = {0x01, // length
+ 0x05, // type (PUSH_PROMISE)
+ 0x01}; // Push Id
+
+ // Process the full frame.
+ InSequence s;
+ EXPECT_CALL(visitor_, OnPushPromiseFrameStart(1));
+ EXPECT_CALL(visitor_, OnPushPromiseFrameEnd());
+ EXPECT_EQ(QUIC_ARRAYSIZE(input),
+ decoder_.ProcessInput(input, QUIC_ARRAYSIZE(input)));
+ EXPECT_EQ(QUIC_NO_ERROR, decoder_.error());
+ EXPECT_EQ("", decoder_.error_detail());
+
+ // Process the frame incremently.
+ EXPECT_CALL(visitor_, OnPushPromiseFrameStart(1));
+ EXPECT_CALL(visitor_, OnPushPromiseFrameEnd());
+ for (char c : input) {
+ EXPECT_EQ(1u, decoder_.ProcessInput(&c, 1));
+ }
+ EXPECT_EQ(QUIC_NO_ERROR, decoder_.error());
+ EXPECT_EQ("", decoder_.error_detail());
+}
+
} // namespace quic
diff --git a/quic/core/http/quic_spdy_stream.cc b/quic/core/http/quic_spdy_stream.cc
index 8a6f002..012250a 100644
--- a/quic/core/http/quic_spdy_stream.cc
+++ b/quic/core/http/quic_spdy_stream.cc
@@ -68,6 +68,7 @@
}
void OnDataFramePayload(QuicStringPiece payload) override {
+ DCHECK(!payload.empty());
stream_->OnDataFramePayload(payload);
}
@@ -83,6 +84,7 @@
}
void OnHeadersFramePayload(QuicStringPiece payload) override {
+ DCHECK(!payload.empty());
if (!VersionUsesQpack(
stream_->session()->connection()->transport_version())) {
CloseConnectionOnWrongFrame("Headers");
@@ -105,6 +107,7 @@
}
void OnPushPromiseFramePayload(QuicStringPiece payload) override {
+ DCHECK(!payload.empty());
CloseConnectionOnWrongFrame("Push Promise");
}
diff --git a/quic/core/http/quic_spdy_stream_body_buffer.cc b/quic/core/http/quic_spdy_stream_body_buffer.cc
index dcb8030..c0a77b2 100644
--- a/quic/core/http/quic_spdy_stream_body_buffer.cc
+++ b/quic/core/http/quic_spdy_stream_body_buffer.cc
@@ -23,6 +23,7 @@
}
void QuicSpdyStreamBodyBuffer::OnDataPayload(QuicStringPiece payload) {
+ DCHECK(!payload.empty());
bodies_.push_back(payload);
total_body_bytes_received_ += payload.length();
total_body_bytes_readable_ += payload.length();
