[GCM] Fix crash when size packet splits two socket reads

google_apis/gcm/engine/connection_handler_impl.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 "google_apis/gcm/engine/connection_handler_impl.h"
 
 #include "base/message_loop/message_loop.h"
 #include "google/protobuf/io/coded_stream.h"
 #include "google_apis/gcm/base/mcs_util.h"
 #include "google_apis/gcm/base/socket_stream.h"
 #include "google_apis/gcm/protocol/mcs.pb.h"
 #include "net/base/net_errors.h"
 #include "net/socket/stream_socket.h"
 
 using namespace google::protobuf::io;
 
 namespace gcm {
 
 namespace {
 
 // # of bytes a MCS version packet consumes.
 const int kVersionPacketLen = 1;
 // # of bytes a tag packet consumes.
 const int kTagPacketLen = 1;
-// Max # of bytes a length packet consumes.
+// Max # of bytes a length packet consumes. A Varint32 can consume up to 5 bytes
+// (the MSB in each byte is reserved for denoting whether more bytes follow).
+// But, the protocol only allows for 4KiB payloads, and the socket stream buffer
+// is only of size 8KiB. As such we should never need more than 2 bytes (max
+// value of 16KiB). Anything higher than that will result in an error, either
+// because the socket stream buffer overflowed or too many bytes were required
+// in the size packet.
 const int kSizePacketLenMin = 1;
 const int kSizePacketLenMax = 2;
 
 // The current MCS protocol version.
 const int kMCSVersion = 41;
 
 }  // namespace
 
 ConnectionHandlerImpl::ConnectionHandlerImpl(
     base::TimeDelta read_timeout,
@@ skipping 278 matching lines @@
 }
 
 void ConnectionHandlerImpl::OnGotMessageSize() {
   if (input_stream_->GetState() != SocketInputStream::READY) {
     LOG(ERROR) << "Failed to receive message size.";
     read_callback_.Run(scoped_ptr<google::protobuf::MessageLite>());
     return;
   }
 
   bool need_another_byte = false;
-  int prev_byte_count = input_stream_->ByteCount();
+  int prev_byte_count = input_stream_->UnreadByteCount();

[fgorski, 2014/10/20 22:11:54]

please explain why you are switching to UnreadByteCount()

[Nicolas Zea, 2014/10/21 00:02:54]

I've updated the commit comment to explain why, but in general it was just a
mistake to use ByteCount in the first place (UnreadByteCount is whats used
everywhere else in this file and elsewhere).

   {
     CodedInputStream coded_input_stream(input_stream_.get());
     if (!coded_input_stream.ReadVarint32(&message_size_))
       need_another_byte = true;
   }
 
   if (need_another_byte) {
     DVLOG(1) << "Expecting another message size byte.";
     if (prev_byte_count >= kSizePacketLenMax) {
       // Already had enough bytes, something else went wrong.
-      LOG(ERROR) << "Failed to process message size.";
-      read_callback_.Run(scoped_ptr<google::protobuf::MessageLite>());
+      LOG(ERROR) << "Failed to process message size, too many bytes needed.";
+      connection_callback_.Run(net::ERR_FILE_TOO_BIG);
       return;
     }
     // Back up by the amount read (should always be 1 byte).
-    int bytes_read = prev_byte_count - input_stream_->ByteCount();
+    int bytes_read = prev_byte_count - input_stream_->UnreadByteCount();
     DCHECK_EQ(bytes_read, 1);
     input_stream_->BackUp(bytes_read);
     WaitForData(MCS_FULL_SIZE);
     return;
   }
 
   DVLOG(1) << "Proto size: " << message_size_;
 
   if (message_size_ > 0)
     WaitForData(MCS_PROTO_BYTES);
   else
     OnGotMessageBytes();
 }
 
 void ConnectionHandlerImpl::OnGotMessageBytes() {
   read_timeout_timer_.Stop();
   scoped_ptr<google::protobuf::MessageLite> protobuf(
       BuildProtobufFromTag(message_tag_));
   // Messages with no content are valid; just use the default protobuf for
   // that tag.
   if (protobuf.get() && message_size_ == 0) {
     base::MessageLoop::current()->PostTask(
         FROM_HERE,
         base::Bind(&ConnectionHandlerImpl::GetNextMessage,
                    weak_ptr_factory_.GetWeakPtr()));
     read_callback_.Run(protobuf.Pass());
     return;
   }
 
-  if (!protobuf.get() ||
-      input_stream_->GetState() != SocketInputStream::READY) {
+  if (input_stream_->GetState() != SocketInputStream::READY) {
     LOG(ERROR) << "Failed to extract protobuf bytes of type "
                << static_cast<unsigned int>(message_tag_);
     // Reset the connection.
     connection_callback_.Run(net::ERR_FAILED);
     return;
   }
 
-  {
+  if (protobuf.get()) {

[fgorski, 2014/10/20 22:11:54]

nit: did you consider putting all of the failure conditions first and leave that
portion at the end?

You won't need else and it will look closer to what was there before.

[Nicolas Zea, 2014/10/21 00:02:54]

Done.

     CodedInputStream coded_input_stream(input_stream_.get());
     if (!protobuf->ParsePartialFromCodedStream(&coded_input_stream)) {
       LOG(ERROR) << "Unable to parse GCM message of type "
                  << static_cast<unsigned int>(message_tag_);
       // Reset the connection.
       connection_callback_.Run(net::ERR_FAILED);
       return;
     }
+  } else {
+    LOG(ERROR) << "Received message of invalid type "
+               << static_cast<unsigned int>(message_tag_);
+    connection_callback_.Run(net::ERR_INVALID_ARGUMENT);
+    return;
   }
 
   input_stream_->RebuildBuffer();
   base::MessageLoop::current()->PostTask(
       FROM_HERE,
       base::Bind(&ConnectionHandlerImpl::GetNextMessage,
                  weak_ptr_factory_.GetWeakPtr()));
   if (message_tag_ == kLoginResponseTag) {
     if (handshake_complete_) {
       LOG(ERROR) << "Unexpected login response.";
@@ skipping 20 matching lines @@
   socket_ = NULL;
   handshake_complete_ = false;
   message_tag_ = 0;
   message_size_ = 0;
   input_stream_.reset();
   output_stream_.reset();
   weak_ptr_factory_.InvalidateWeakPtrs();
 }
 
 }  // namespace gcm