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

google_apis/gcm/engine/connection_handler_impl.cc

Index: google_apis/gcm/engine/connection_handler_impl.cc
diff --git a/google_apis/gcm/engine/connection_handler_impl.cc b/google_apis/gcm/engine/connection_handler_impl.cc
index 751b1cd1daa52fabcbcc583cdcc6d61b04e37861..f2eb1122c197b8ac7ac0b97c44133bdf006c86ee 100644
--- a/google_apis/gcm/engine/connection_handler_impl.cc
+++ b/google_apis/gcm/engine/connection_handler_impl.cc
@@ -22,7 +22,13 @@ namespace {
 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;
 
@@ -321,7 +327,7 @@ void ConnectionHandlerImpl::OnGotMessageSize() {
   }
 
   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_))
@@ -332,12 +338,12 @@ void ConnectionHandlerImpl::OnGotMessageSize() {
     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);
@@ -367,8 +373,7 @@ void ConnectionHandlerImpl::OnGotMessageBytes() {
     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.
@@ -376,7 +381,7 @@ void ConnectionHandlerImpl::OnGotMessageBytes() {
     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 "
@@ -385,6 +390,11 @@ void ConnectionHandlerImpl::OnGotMessageBytes() {
       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();