Check for invalid use of data frame opcodes.

net/websockets/websocket_channel.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/websockets/websocket_channel.h"
 
 #include <algorithm>
 
 #include "base/basictypes.h"  // for size_t
 #include "base/bind.h"
@@ skipping 211 matching lines @@
     : event_interface_(event_interface.Pass()),
       url_request_context_(url_request_context),
       send_quota_low_water_mark_(kDefaultSendQuotaLowWaterMark),
       send_quota_high_water_mark_(kDefaultSendQuotaHighWaterMark),
       current_send_quota_(0),
       timeout_(base::TimeDelta::FromSeconds(kClosingHandshakeTimeoutSeconds)),
       closing_code_(0),
       state_(FRESHLY_CONSTRUCTED),
       notification_sender_(new HandshakeNotificationSender(this)),
       sending_text_message_(false),
-      receiving_text_message_(false) {}
+      receiving_text_message_(false),
+      expecting_to_read_continuation_(false) {}
 
 WebSocketChannel::~WebSocketChannel() {
   // The stream may hold a pointer to read_frames_, and so it needs to be
   // destroyed first.
   stream_.reset();
   // The timer may have a callback pointing back to us, so stop it just in case
   // someone decides to run the event loop from their destructor.
   timer_.Stop();
 }
 
@@ skipping 415 matching lines @@
       default:
         frame_name = "Unknown frame type";
         break;
     }
     // FailChannel() won't send another Close frame.
     return FailChannel(
         frame_name + " received after close", kWebSocketErrorProtocolError, "");
   }
   switch (opcode) {
     case WebSocketFrameHeader::kOpCodeText:    // fall-thru
-    case WebSocketFrameHeader::kOpCodeBinary:  // fall-thru
+    case WebSocketFrameHeader::kOpCodeBinary:
+      return HandleDataFrame(opcode, final, data_buffer, size, false);
+
     case WebSocketFrameHeader::kOpCodeContinuation:
-      if (state_ == CONNECTED) {
-        if (opcode == WebSocketFrameHeader::kOpCodeText ||
-            (opcode == WebSocketFrameHeader::kOpCodeContinuation &&
-             receiving_text_message_)) {
-          // This call is not redundant when size == 0 because it tells us what
-          // the current state is.
-          StreamingUtf8Validator::State state =
-              incoming_utf8_validator_.AddBytes(
-                  size ? data_buffer->data() : NULL, size);
-          if (state == StreamingUtf8Validator::INVALID ||
-              (state == StreamingUtf8Validator::VALID_MIDPOINT && final)) {
-            return FailChannel("Could not decode a text frame as UTF-8.",
-                               kWebSocketErrorProtocolError,
-                               "Invalid UTF-8 in text frame");
-          }
-          receiving_text_message_ = !final;
-          DCHECK(!final || state == StreamingUtf8Validator::VALID_ENDPOINT);
-        }
-        // TODO(ricea): Can this copy be eliminated?
-        const char* const data_begin = size ? data_buffer->data() : NULL;
-        const char* const data_end = data_begin + size;
-        const std::vector<char> data(data_begin, data_end);
-        // TODO(ricea): Handle the case when ReadFrames returns far
-        // more data at once than should be sent in a single IPC. This needs to
-        // be handled carefully, as an overloaded IO thread is one possible
-        // cause of receiving very large chunks.
-
-        // Sends the received frame to the renderer process.
-        return event_interface_->OnDataFrame(final, opcode, data);
-      }
-      VLOG(3) << "Ignored data packet received in state " << state_;
-      return CHANNEL_ALIVE;
+      return HandleDataFrame(opcode, final, data_buffer, size, true);
 
     case WebSocketFrameHeader::kOpCodePing:
       VLOG(1) << "Got Ping of size " << size;
       if (state_ == CONNECTED)
         return SendIOBuffer(
             true, WebSocketFrameHeader::kOpCodePong, data_buffer, size);
       VLOG(3) << "Ignored ping in state " << state_;
       return CHANNEL_ALIVE;
 
     case WebSocketFrameHeader::kOpCodePong:
@@ skipping 40 matching lines @@
     }
 
     default:
       return FailChannel(
           base::StringPrintf("Unrecognized frame opcode: %d", opcode),
           kWebSocketErrorProtocolError,
           "Unknown opcode");
   }
 }
 
+ChannelState WebSocketChannel::HandleDataFrame(
+    const WebSocketFrameHeader::OpCode opcode,
+    bool final,
+    const scoped_refptr<IOBuffer>& data_buffer,
+    size_t size,
+    bool expecting_continuation) {
+  if (state_ != CONNECTED) {
+    DVLOG(3) << "Ignored data packet received in state " << state_;
+    return CHANNEL_ALIVE;
+  }
+  if (expecting_continuation != expecting_to_read_continuation_) {

[yhirano, 2014/02/13 08:51:28]

[opt] Just using |opcode| instead of |expecting_continuation| here and unifying
the switch branches seems cleaner for me.

[Adam Rice, 2014/02/18 05:22:36]

Yeah, this code is ugly. But I don't want to check |opcode| again when I just
switched on it immediately before calling this method.

I'm going to send to tyoshino to see if he has any ideas for how to do this
nicely.

[tyoshino (SeeGerritForStatus), 2014/02/18 06:24:12]

In general, it's good to have exhaustive switch-case for handling enum. But
here, I think you can choose to compare opcode with OpcodeContinuation using ==
by also adding DCHECKs to ensure that we're here only when opcode is one of
text, binary and cont. There's no worry to forget handling a new enum (opcode).
Then, we fail with DCHECK.

[Adam Rice, 2014/02/18 11:12:43]

Is this what you mean?

+    const bool got_continuation = !expecting_to_read_continuation_;
+    const std::string console_log = got_continuation
+        ? "Received unexpected continuation frame."
+        : "Received start of new message but previous message is unfinished.";
+    const std::string reason = got_continuation
+        ? "Unexpected continuation"
+        : "Previous data frame unfinished";
+    return FailChannel(console_log, kWebSocketErrorProtocolError, reason);
+  }
+  expecting_to_read_continuation_ = !final;
+  if (opcode == WebSocketFrameHeader::kOpCodeText ||
+      (opcode == WebSocketFrameHeader::kOpCodeContinuation &&
+       receiving_text_message_)) {
+    // This call is not redundant when size == 0 because it tells us what
+    // the current state is.
+    StreamingUtf8Validator::State state = incoming_utf8_validator_.AddBytes(
+        size ? data_buffer->data() : NULL, size);
+    if (state == StreamingUtf8Validator::INVALID ||
+        (state == StreamingUtf8Validator::VALID_MIDPOINT && final)) {
+      return FailChannel("Could not decode a text frame as UTF-8.",
+                         kWebSocketErrorProtocolError,
+                         "Invalid UTF-8 in text frame");
+    }
+    receiving_text_message_ = !final;
+    DCHECK(!final || state == StreamingUtf8Validator::VALID_ENDPOINT);
+  }
+  // TODO(ricea): Can this copy be eliminated?
+  const char* const data_begin = size ? data_buffer->data() : NULL;
+  const char* const data_end = data_begin + size;
+  const std::vector<char> data(data_begin, data_end);
+  // TODO(ricea): Handle the case when ReadFrames returns far
+  // more data at once than should be sent in a single IPC. This needs to
+  // be handled carefully, as an overloaded IO thread is one possible
+  // cause of receiving very large chunks.
+
+  // Sends the received frame to the renderer process.
+  return event_interface_->OnDataFrame(final, opcode, data);
+}
+
 ChannelState WebSocketChannel::SendIOBuffer(
     bool fin,
     WebSocketFrameHeader::OpCode op_code,
     const scoped_refptr<IOBuffer>& buffer,
     size_t size) {
   DCHECK(state_ == CONNECTED || state_ == RECV_CLOSED);
   DCHECK(stream_);
   scoped_ptr<WebSocketFrame> frame(new WebSocketFrame(op_code));
   WebSocketFrameHeader& header = frame->header;
   header.final = fin;
@@ skipping 135 matching lines @@
 
 void WebSocketChannel::CloseTimeout() {
   stream_->Close();
   DCHECK_NE(CLOSED, state_);
   state_ = CLOSED;
   AllowUnused(DoDropChannel(false, kWebSocketErrorAbnormalClosure, ""));
   // |this| has been deleted.
 }
 
 }  // namespace net