WebSocketFrameChunk should use IOBuffer to hold data

net/websockets/websocket_frame_parser.cc

 // Copyright (c) 2012 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_frame_parser.h"
 
 #include <algorithm>
+#include <limits>
 
 #include "base/basictypes.h"
 #include "base/logging.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/scoped_vector.h"
 #include "net/base/big_endian.h"
+#include "net/base/io_buffer.h"
 #include "net/websockets/websocket_frame.h"
 
 namespace {
 
 const uint8 kFinalBit = 0x80;
 const uint8 kReserved1Bit = 0x40;
 const uint8 kReserved2Bit = 0x20;
 const uint8 kReserved3Bit = 0x10;
 const uint8 kOpCodeMask = 0xF;
 const uint8 kMaskBit = 0x80;
@@ skipping 146 matching lines @@
 
 scoped_ptr<WebSocketFrameChunk> WebSocketFrameParser::DecodeFramePayload(
     bool first_chunk) {
   static const int kMaskingKeyLength = WebSocketFrameHeader::kMaskingKeyLength;
 
   const char* current = &buffer_.front() + current_read_pos_;
   const char* end = &buffer_.front() + buffer_.size();
   uint64 next_size = std::min<uint64>(
       end - current,
       current_frame_header_->payload_length - frame_offset_);
+  DCHECK_LE(next_size, static_cast<uint64>(std::numeric_limits<int>::max()));

[Ryan Sleevi, 2012/07/30 07:21:13]

nit: Is this more appropriate as a CHECK, rather than a DCHECK()?

Or even better, something that can/should be handled at the protocol layer since
it reflects "user" input (well, network input).

It seems very dangerous to have this simply be a DCHECK, due to the interaction
between line 191 (which allocates clamped on int) and line 193 (which copies
clamped on size_t).

[Takashi Toyoshima, 2012/07/30 08:01:21]

Agreed.

RFC defines status code 1009 meaning "Message Too Big".
When a client receives a too big frame to receive, we can just dispose it, then
close the connection with the code 1009.

CHECK looks better than DCHECK here. But if I'd implement graceful error
handling, this component should report an error to upper layer component.

Currently, this component stop to decode by |failed_| flag. I use this failure
status for now, and will introduce more concrete error code handling in another
change.

[mmenke, 2012/07/30 13:26:57]

Just thought I'd point out that these don't correspond to over the wire frames,
but rather Chrome-created chunks which are subsets of a frame's body, so this
will not prevent us from handling large frames.

Since these are only created by the browser process from data received from the
stream (Where we use integer-sized read buffers), or received from the renderer
process (And there's a max IPC length), this code should be fine.

That having been said, I think a CHECK is reasonable.

 
   scoped_ptr<WebSocketFrameChunk> frame_chunk(new WebSocketFrameChunk);
   if (first_chunk) {
     frame_chunk->header.reset(new WebSocketFrameHeader(*current_frame_header_));
   }
   frame_chunk->final_chunk = false;
-  frame_chunk->data.assign(current, current + next_size);
-  if (current_frame_header_->masked) {
-    // Unmask the payload.
-    // TODO(yutak): This could be faster by doing unmasking for each
-    // machine word (instead of each byte).
-    size_t key_offset = frame_offset_ % kMaskingKeyLength;
-    for (uint64 i = 0; i < next_size; ++i) {
-      frame_chunk->data[i] ^= masking_key_[key_offset];
-      key_offset = (key_offset + 1) % kMaskingKeyLength;
+  if (next_size) {
+    frame_chunk->data = new IOBufferWithSize(static_cast<int>(next_size));
+    char* io_data = frame_chunk->data->data();
+    memcpy(io_data, current, next_size);
+    if (current_frame_header_->masked) {
+      // Unmask the payload.
+      // TODO(yutak): This could be faster by doing unmasking for each
+      // machine word (instead of each byte).

[Ryan Sleevi, 2012/07/30 07:21:13]

FWIW, when you do go and begin to (micro?)-optimize, it might be worth looking
at _mm_xor_si128 , which is an SSE2 instruction that lets you xor based on
quadwords, provided the quadwords are aligned properly (support for which is
being added by DaleCurtis in http://codereview.chromium.org/10828054/ ).

The media/ code already has selective functionality based on CPU discovery (eg:
using SSE2 -> MMX -> hand-unrolled). However, if you can guaranteed aligned
allocation (~line 191), hand-calling SSE2 will be the most performant unrolling
possible, better than the compiler would be legally allowed to do.

[Takashi Toyoshima, 2012/07/30 08:01:21]

Wow, he made many interesting optimizations.
Thank you for good information.
We'll check his changes to support optimization.

+      size_t key_offset = frame_offset_ % kMaskingKeyLength;
+      for (uint64 i = 0; i < next_size; ++i) {
+        io_data[i] ^= masking_key_[key_offset];
+        key_offset = (key_offset + 1) % kMaskingKeyLength;
+      }
     }
+
+    current_read_pos_ += next_size;
+    frame_offset_ += next_size;
   }
 
-  current_read_pos_ += next_size;
-  frame_offset_ += next_size;
-
   DCHECK_LE(frame_offset_, current_frame_header_->payload_length);
   if (frame_offset_ == current_frame_header_->payload_length) {
     frame_chunk->final_chunk = true;
     current_frame_header_.reset();
     frame_offset_ = 0;
   }
 
   return frame_chunk.Pass();
 }
 
 }  // namespace net