Signal FRAME_SIZE_ERROR on mis-sized control frames.

Change GO_AWAY error code returned for mis-sized control frames to be
FRAME_SIZE_ERROR to match the specification.

This CL lands server change 116794299 by dahollings.

BUG=488484

Review URL: https://codereview.chromium.org/1857763003

Cr-Commit-Position: refs/heads/master@{#385614}

net/spdy/spdy_framer.cc

@@ -423,6 +423,8 @@ const char* SpdyFramer::ErrorCodeToString(int error_code) {
       return "INVALID_CONTROL_FRAME";
     case SPDY_CONTROL_PAYLOAD_TOO_LARGE:
       return "CONTROL_PAYLOAD_TOO_LARGE";
+    case SPDY_INVALID_CONTROL_FRAME_SIZE:
+      return "INVALID_CONTROL_FRAME_SIZE";
     case SPDY_ZLIB_INIT_FAILURE:
       return "ZLIB_INIT_FAILURE";
     case SPDY_UNSUPPORTED_VERSION:
@@ -903,13 +905,11 @@ void SpdyFramer::ProcessControlFrameHeader(int control_frame_type_field) {
       }
       break;
     case RST_STREAM:
-      // TODO(bnc): Enforce the length of the header, and change error to
-      // FRAME_SIZE_ERROR.
       if ((current_frame_length_ != GetRstStreamMinimumSize() &&
            protocol_version_ == SPDY3) ||
           (current_frame_length_ < GetRstStreamMinimumSize() &&
            protocol_version_ == HTTP2)) {
-        set_error(SPDY_INVALID_CONTROL_FRAME);
+        set_error(SPDY_INVALID_CONTROL_FRAME_SIZE);
       } else if (current_frame_flags_ != 0) {
         set_error(SPDY_INVALID_CONTROL_FRAME_FLAGS);
       }
@@ -926,7 +926,7 @@ void SpdyFramer::ProcessControlFrameHeader(int control_frame_type_field) {
           % setting_size != values_prefix_size) {
         DLOG(WARNING) << "Invalid length for SETTINGS frame: "
                       << current_frame_length_;
-        set_error(SPDY_INVALID_CONTROL_FRAME);
+        set_error(SPDY_INVALID_CONTROL_FRAME_SIZE);
       } else if (protocol_version_ == SPDY3 &&
                  current_frame_flags_ &
                      ~SETTINGS_FLAG_CLEAR_PREVIOUSLY_PERSISTED_SETTINGS) {
@@ -937,13 +937,13 @@ void SpdyFramer::ProcessControlFrameHeader(int control_frame_type_field) {
       } else if (protocol_version_ == HTTP2 &&
                  current_frame_flags_ & SETTINGS_FLAG_ACK &&
                  current_frame_length_ > GetSettingsMinimumSize()) {
-        set_error(SPDY_INVALID_CONTROL_FRAME);
+        set_error(SPDY_INVALID_CONTROL_FRAME_SIZE);
       }
       break;
     }
     case PING:
       if (current_frame_length_ != GetPingSize()) {
-        set_error(SPDY_INVALID_CONTROL_FRAME);
+        set_error(SPDY_INVALID_CONTROL_FRAME_SIZE);
       } else if ((protocol_version_ == SPDY3 && current_frame_flags_ != 0) ||
                  (current_frame_flags_ & ~PING_FLAG_ACK)) {
         set_error(SPDY_INVALID_CONTROL_FRAME_FLAGS);
@@ -989,7 +989,7 @@ void SpdyFramer::ProcessControlFrameHeader(int control_frame_type_field) {
       break;
     case WINDOW_UPDATE:
       if (current_frame_length_ != GetWindowUpdateSize()) {
-        set_error(SPDY_INVALID_CONTROL_FRAME);
+        set_error(SPDY_INVALID_CONTROL_FRAME_SIZE);
       } else if (current_frame_flags_ != 0) {
         set_error(SPDY_INVALID_CONTROL_FRAME_FLAGS);
       }
@@ -1032,7 +1032,7 @@ void SpdyFramer::ProcessControlFrameHeader(int control_frame_type_field) {
     case PRIORITY:
       if (protocol_version_ == SPDY3 ||
           current_frame_length_ != GetPrioritySize()) {
-        set_error(SPDY_INVALID_CONTROL_FRAME);
+        set_error(SPDY_INVALID_CONTROL_FRAME_SIZE);
       } else if (current_frame_flags_ != 0) {
         set_error(SPDY_INVALID_CONTROL_FRAME_FLAGS);
       }

net/spdy/spdy_framer.h

@@ -340,6 +340,7 @@ class NET_EXPORT_PRIVATE SpdyFramer {
     SPDY_INVALID_DATA_FRAME_FLAGS,     // Data frame has invalid flags.
     SPDY_INVALID_CONTROL_FRAME_FLAGS,  // Control frame has invalid flags.
     SPDY_UNEXPECTED_FRAME,             // Frame received out of order.
+    SPDY_INVALID_CONTROL_FRAME_SIZE,   // Control frame not sized to spec
 
     LAST_ERROR,  // Must be the last entry in the enum.
   };

net/spdy/spdy_framer_test.cc

@@ -3471,6 +3471,9 @@ TEST_P(SpdyFramerTest, ReadBogusLenSettingsFrame) {
   // Should generate an error, since its not possible to have a
   // settings frame of length kNewLength.
   EXPECT_EQ(1, visitor.error_count_);
+  EXPECT_EQ(SpdyFramer::SPDY_INVALID_CONTROL_FRAME_SIZE,
+            visitor.framer_.error_code())
+      << SpdyFramer::ErrorCodeToString(visitor.framer_.error_code());
 }
 
 // Tests handling of SETTINGS frames larger than the frame buffer size.
@@ -4228,6 +4231,9 @@ TEST_P(SpdyFramerTest, ErrorCodeToStringTest) {
   EXPECT_STREQ("CONTROL_PAYLOAD_TOO_LARGE",
                SpdyFramer::ErrorCodeToString(
                    SpdyFramer::SPDY_CONTROL_PAYLOAD_TOO_LARGE));
+  EXPECT_STREQ("INVALID_CONTROL_FRAME_SIZE",
+               SpdyFramer::ErrorCodeToString(
+                   SpdyFramer::SPDY_INVALID_CONTROL_FRAME_SIZE));
   EXPECT_STREQ("ZLIB_INIT_FAILURE",
                SpdyFramer::ErrorCodeToString(
                    SpdyFramer::SPDY_ZLIB_INIT_FAILURE));
@@ -4832,7 +4838,7 @@ TEST_P(SpdyFramerTest, SettingsFrameFlags) {
     } else if (flags & SETTINGS_FLAG_ACK) {
       // The frame is invalid because ACK frames should have no payload.
       EXPECT_EQ(SpdyFramer::SPDY_ERROR, framer.state());
-      EXPECT_EQ(SpdyFramer::SPDY_INVALID_CONTROL_FRAME,
+      EXPECT_EQ(SpdyFramer::SPDY_INVALID_CONTROL_FRAME_SIZE,
                 framer.error_code())
           << SpdyFramer::ErrorCodeToString(framer.error_code());
     } else {
@@ -5660,7 +5666,68 @@ TEST_P(SpdyFramerTest, ReadIncorrectlySizedPriority) {
   visitor.SimulateInFramer(kFrameData, sizeof(kFrameData));
 
   EXPECT_EQ(SpdyFramer::SPDY_ERROR, visitor.framer_.state());
-  EXPECT_EQ(SpdyFramer::SPDY_INVALID_CONTROL_FRAME,
+  EXPECT_EQ(SpdyFramer::SPDY_INVALID_CONTROL_FRAME_SIZE,
+            visitor.framer_.error_code())
+      << SpdyFramer::ErrorCodeToString(visitor.framer_.error_code());
+}
+
+// Tests handling of PING frame with incorrect size.
+TEST_P(SpdyFramerTest, ReadIncorrectlySizedPing) {
+  if (!IsHttp2()) {
+    return;
+  }
+
+  // PING frame of size 4, which isn't correct.
+  const unsigned char kFrameData[] = {
+      0x00, 0x00, 0x04, 0x06, 0x00, 0x00, 0x00,
+      0x00, 0x03, 0x00, 0x00, 0x00, 0x01,
+  };
+
+  TestSpdyVisitor visitor(spdy_version_);
+  visitor.SimulateInFramer(kFrameData, sizeof(kFrameData));
+
+  EXPECT_EQ(SpdyFramer::SPDY_ERROR, visitor.framer_.state());
+  EXPECT_EQ(SpdyFramer::SPDY_INVALID_CONTROL_FRAME_SIZE,
+            visitor.framer_.error_code())
+      << SpdyFramer::ErrorCodeToString(visitor.framer_.error_code());
+}
+
+// Tests handling of WINDOW_UPDATE frame with incorrect size.
+TEST_P(SpdyFramerTest, ReadIncorrectlySizedWindowUpdate) {
+  if (!IsHttp2()) {
+    return;
+  }
+
+  // WINDOW_UPDATE frame of size 3, which isn't correct.
+  const unsigned char kFrameData[] = {
+      0x00, 0x00, 0x03, 0x08, 0x00, 0x00, 0x00, 0x00, 0x03, 0x00, 0x00, 0x01,
+  };
+
+  TestSpdyVisitor visitor(spdy_version_);
+  visitor.SimulateInFramer(kFrameData, sizeof(kFrameData));
+
+  EXPECT_EQ(SpdyFramer::SPDY_ERROR, visitor.framer_.state());
+  EXPECT_EQ(SpdyFramer::SPDY_INVALID_CONTROL_FRAME_SIZE,
+            visitor.framer_.error_code())
+      << SpdyFramer::ErrorCodeToString(visitor.framer_.error_code());
+}
+
+// Tests handling of RST_STREAM frame with incorrect size.
+TEST_P(SpdyFramerTest, ReadIncorrectlySizedRstStream) {
+  if (!IsHttp2()) {
+    return;
+  }
+
+  // RST_STREAM frame of size 3, which isn't correct.
+  const unsigned char kFrameData[] = {
+      0x00, 0x00, 0x03, 0x03, 0x00, 0x00, 0x00, 0x00, 0x03, 0x00, 0x00, 0x01,
+  };
+
+  TestSpdyVisitor visitor(spdy_version_);
+  visitor.SimulateInFramer(kFrameData, sizeof(kFrameData));
+
+  EXPECT_EQ(SpdyFramer::SPDY_ERROR, visitor.framer_.state());
+  EXPECT_EQ(SpdyFramer::SPDY_INVALID_CONTROL_FRAME_SIZE,
             visitor.framer_.error_code())
       << SpdyFramer::ErrorCodeToString(visitor.framer_.error_code());
 }

net/spdy/spdy_network_transaction_unittest.cc

@@ -3599,12 +3599,11 @@ TEST_P(SpdyNetworkTransactionTest, GoAwayOnFrameSizeError) {
   scoped_ptr<SpdySerializedFrame> req(
       spdy_util_.ConstructSpdyGet(nullptr, 0, 1, LOWEST, true));
   scoped_ptr<SpdySerializedFrame> goaway(spdy_util_.ConstructSpdyGoAway(
-      0, GOAWAY_PROTOCOL_ERROR, "Framer error: 1 (INVALID_CONTROL_FRAME)."));
+      0, GOAWAY_FRAME_SIZE_ERROR,
+      "Framer error: 12 (INVALID_CONTROL_FRAME_SIZE)."));
   MockWrite writes[] = {CreateMockWrite(*req, 0), CreateMockWrite(*goaway, 2)};
 
   // Read WINDOW_UPDATE with incorrectly-sized payload.
-  // TODO(jgraettinger): SpdyFramer signals this as an INVALID_CONTROL_FRAME,
-  // which is mapped to a protocol error, and not a frame size error.
   scoped_ptr<SpdySerializedFrame> bad_window_update(
       spdy_util_.ConstructSpdyWindowUpdate(1, 1));
   test::SetFrameLength(bad_window_update.get(),
@@ -3617,7 +3616,7 @@ TEST_P(SpdyNetworkTransactionTest, GoAwayOnFrameSizeError) {
       CreateGetRequest(), DEFAULT_PRIORITY, BoundNetLog(), GetParam(), NULL);
   helper.RunToCompletion(&data);
   TransactionHelperResult out = helper.output();
-  EXPECT_EQ(ERR_SPDY_PROTOCOL_ERROR, out.rv);
+  EXPECT_EQ(ERR_SPDY_FRAME_SIZE_ERROR, out.rv);
 }
 
 // Test that we shutdown correctly on write errors.

net/spdy/spdy_session.cc

@@ -374,6 +374,8 @@ SpdyProtocolErrorDetails MapFramerErrorToProtocolError(
       return SPDY_ERROR_INVALID_CONTROL_FRAME_FLAGS;
     case SpdyFramer::SPDY_UNEXPECTED_FRAME:
       return SPDY_ERROR_UNEXPECTED_FRAME;
+    case SpdyFramer::SPDY_INVALID_CONTROL_FRAME_SIZE:
+      return SPDY_ERROR_INVALID_CONTROL_FRAME_SIZE;
     default:
       NOTREACHED();
       return static_cast<SpdyProtocolErrorDetails>(-1);
@@ -406,6 +408,8 @@ Error MapFramerErrorToNetError(SpdyFramer::SpdyError err) {
       return ERR_SPDY_PROTOCOL_ERROR;
     case SpdyFramer::SPDY_UNEXPECTED_FRAME:
       return ERR_SPDY_PROTOCOL_ERROR;
+    case SpdyFramer::SPDY_INVALID_CONTROL_FRAME_SIZE:
+      return ERR_SPDY_FRAME_SIZE_ERROR;
     default:
       NOTREACHED();
       return ERR_SPDY_PROTOCOL_ERROR;

net/spdy/spdy_session.h

@@ -97,6 +97,7 @@ enum SpdyProtocolErrorDetails {
   SPDY_ERROR_INVALID_DATA_FRAME_FLAGS = 8,
   SPDY_ERROR_INVALID_CONTROL_FRAME_FLAGS = 9,
   SPDY_ERROR_UNEXPECTED_FRAME = 31,
+  SPDY_ERROR_INVALID_CONTROL_FRAME_SIZE = 37,
   // SpdyRstStreamStatus mappings.
   // RST_STREAM_INVALID not mapped.
   STATUS_CODE_PROTOCOL_ERROR = 11,
@@ -125,7 +126,7 @@ enum SpdyProtocolErrorDetails {
   PROTOCOL_ERROR_RECEIVE_WINDOW_VIOLATION = 28,
 
   // Next free value.
-  NUM_SPDY_PROTOCOL_ERROR_DETAILS = 37,
+  NUM_SPDY_PROTOCOL_ERROR_DETAILS = 38,
 };
 SpdyProtocolErrorDetails NET_EXPORT_PRIVATE
     MapFramerErrorToProtocolError(SpdyFramer::SpdyError error);
@@ -136,7 +137,7 @@ SpdyGoAwayStatus NET_EXPORT_PRIVATE MapNetErrorToGoAwayStatus(Error err);
 
 // If these compile asserts fail then SpdyProtocolErrorDetails needs
 // to be updated with new values, as do the mapping functions above.
-static_assert(12 == SpdyFramer::LAST_ERROR,
+static_assert(13 == SpdyFramer::LAST_ERROR,
               "SpdyProtocolErrorDetails / Spdy Errors mismatch");
 static_assert(17 == RST_STREAM_NUM_STATUS_CODES,
               "SpdyProtocolErrorDetails / RstStreamStatus mismatch");

tools/metrics/histograms/histograms.xml

@@ -82861,6 +82861,11 @@ To add a new entry, add it with any value and run test to compute valid value.
       label="Connection established in response to CONNECT request was
              abnormally closed"/>
   <int value="34" label="Peer exhibiting suspect behavior."/>
+  <int value="35" label="Inadequate security."/>
+  <int value="36" label="HTTP/1.1 required."/>
+<!-- More SpdyFramer::SpdyErrors -->
+
+  <int value="37" label="Invalid control frame size."/>
 </enum>
 
 <enum name="SpdyProtocolVersion" type="int">