commit 1ff868169c83ab770f36ed932e30e3c902f771ef
author bnc <bnc@google.com> Tue Sep 28 06:00:44 2021 -0700
committer Copybara-Service <copybara-worker@google.com> Tue Sep 28 06:02:14 2021 -0700
tree a8566b3f4d02cb28e8210a142f9c21bfecf0e9c8
parent 62caa6c4f5a751d56e7321dea321d8cb742c5af0

Do not preallocate memory in HttpDecoder for frame payload.

DATA, HEADERS, and unknown frame payload fragments are passed immediately to
Visitor without buffering.  CANCEL_PUSH and PUSH_PROMISE frames are not
processed.  GOAWAY and MAX_PUSH_ID frames have small payloads.  Only SETTINGS,
PRIORITY_UPDATE, and ACCEPT_CH frames may be buffered by HttpDecoder and are
allowed to have large payloads.  In most cases these frames are small and the
payload is sent in a single fragment, in which case HttpDecoder parses it
directly from the input without buffering.  Buffering only happens in the rest
of the cases, which clearly is not a hot path.  This CL changes HttpDecoder not
to preallocate memory for the payload buffer in these cases, and let std::string
deal with growing internally.

Also introduce kPayloadLengthLimit constant, and do not call MaxFrameLength()
for frames that are not buffered.

PiperOrigin-RevId: 399421301

quic/core/http/http_decoder.cc

diff --git a/quic/core/http/http_decoder.cc b/quic/core/http/http_decoder.cc
index de86955..2265b77 100644
--- a/quic/core/http/http_decoder.cc
+++ b/quic/core/http/http_decoder.cc
@@ -5,7 +5,6 @@
 #include "quic/core/http/http_decoder.h"
 
 #include <cstdint>
-#include <limits>
 
 #include "absl/base/attributes.h"
 #include "absl/strings/string_view.h"
@@ -21,6 +20,17 @@
 
 namespace quic {
 
+namespace {
+
+// Limit on the payload length for frames that are buffered by HttpDecoder.
+// If a frame header indicating a payload length exceeding this limit is
+// received, HttpDecoder closes the connection.  Does not apply to frames that
+// are not buffered here but each payload fragment is immediately passed to
+// Visitor, like HEADERS, DATA, and unknown frames.
+constexpr QuicByteCount kPayloadLengthLimit = 1024 * 1024;
+
+}  // anonymous namespace
+
 HttpDecoder::HttpDecoder(Visitor* visitor) : HttpDecoder(visitor, Options()) {}
 HttpDecoder::HttpDecoder(Visitor* visitor, Options options)
     : visitor_(visitor),
@@ -227,11 +237,8 @@
     return false;
   }
 
-  if (current_frame_length_ > MaxFrameLength(current_frame_type_)) {
-    // MaxFrameLength() returns numeric_limits::max()
-    // if IsFrameBuffered() is false.
-    QUICHE_DCHECK(IsFrameBuffered());
-
+  if (IsFrameBuffered() &&
+      current_frame_length_ > MaxFrameLength(current_frame_type_)) {
     RaiseError(QUIC_HTTP_FRAME_TOO_LARGE, "Frame is too large.");
     return false;
   }
@@ -470,10 +477,6 @@
     continue_processing = ParseEntirePayload(&current_payload_reader);
     reader->Seek(current_frame_length_);
   } else {
-    if (buffer_.empty()) {
-      buffer_.reserve(current_frame_length_);
-    }
-
     // Buffer as much of the payload as |*reader| contains.
     QuicByteCount bytes_to_read = std::min<QuicByteCount>(
         remaining_frame_length_, reader->BytesRemaining());
@@ -654,23 +657,22 @@
 }
 
 QuicByteCount HttpDecoder::MaxFrameLength(uint64_t frame_type) {
+  QUICHE_DCHECK(IsFrameBuffered());
+
   switch (frame_type) {
     case static_cast<uint64_t>(HttpFrameType::SETTINGS):
-      // This limit is arbitrary.
-      return 1024 * 1024;
+      return kPayloadLengthLimit;
     case static_cast<uint64_t>(HttpFrameType::GOAWAY):
       return VARIABLE_LENGTH_INTEGER_LENGTH_8;
     case static_cast<uint64_t>(HttpFrameType::MAX_PUSH_ID):
       return VARIABLE_LENGTH_INTEGER_LENGTH_8;
     case static_cast<uint64_t>(HttpFrameType::PRIORITY_UPDATE_REQUEST_STREAM):
-      // This limit is arbitrary.
-      return 1024 * 1024;
+      return kPayloadLengthLimit;
     case static_cast<uint64_t>(HttpFrameType::ACCEPT_CH):
-      // This limit is arbitrary.
-      return 1024 * 1024;
+      return kPayloadLengthLimit;
     default:
-      // Other frames require no data buffering, so it's safe to have no limit.
-      return std::numeric_limits<QuicByteCount>::max();
+      QUICHE_NOTREACHED();
+      return 0;
   }
 }