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

google_apis/gcm/engine/connection_handler_impl_unittest.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/bind.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/run_loop.h"
 #include "base/strings/string_number_conversions.h"
@@ skipping 118 matching lines @@
 
  private:
   void ReadContinuation(ScopedMessage* dst_proto, ScopedMessage new_proto);
   void WriteContinuation();
   void ConnectionContinuation(int error);
 
   // SocketStreams and their data provider.
   ReadList mock_reads_;
   WriteList mock_writes_;
   scoped_ptr<net::DelayedSocketData> data_provider_;
-  scoped_ptr<SocketInputStream> socket_input_stream_;
-  scoped_ptr<SocketOutputStream> socket_output_stream_;
 
   // The connection handler being tested.
   scoped_ptr<ConnectionHandlerImpl> connection_handler_;
 
   // The last connection error received.
   int last_error_;
 
   // net:: components.
   scoped_ptr<net::StreamSocket> socket_;
   net::MockClientSocketFactory socket_factory_;
@@ skipping 516 matching lines @@
   data_message.set_from(kDataMsgFrom);
   data_message.set_category(kDataMsgCategory);
   connection_handler()->SendMessage(data_message);
   EXPECT_FALSE(connection_handler()->CanSendMessage());
   WaitForMessage();  // The message send. Should result in an error
   EXPECT_FALSE(connection_handler()->CanSendMessage());
   EXPECT_EQ(net::ERR_CONNECTION_CLOSED, last_error());
 }
 
 // Receive a message whose size field was corrupted and is larger than the
-// socket's buffer. Should fail gracefully.
-TEST_F(GCMConnectionHandlerImplTest, CorruptedSize) {
+// socket's buffer. Should fail gracefully with a size error.
+TEST_F(GCMConnectionHandlerImplTest, OutOfBuffer) {
   std::string handshake_request = EncodeHandshakeRequest();
   WriteList write_list(1, net::MockWrite(net::ASYNC,
                                          handshake_request.c_str(),
                                          handshake_request.size()));
   std::string handshake_response = EncodeHandshakeResponse();
 
   // Fill a string with 9000 character zero.
   std::string data_message_proto(9000, '0');
   std::string data_message_pkt =
       EncodePacket(kDataMessageStanzaTag, data_message_proto);
   ReadList read_list;
   read_list.push_back(net::MockRead(net::ASYNC,
                                     handshake_response.c_str(),
                                     handshake_response.size()));
   read_list.push_back(net::MockRead(net::ASYNC,
                                     data_message_pkt.c_str(),
                                     data_message_pkt.size()));
   BuildSocket(read_list, write_list);
 
   ScopedMessage received_message;
   Connect(&received_message);
   WaitForMessage();  // The login send.
   WaitForMessage();  // The login response.
   received_message.reset();
   WaitForMessage();  // The data message.
   EXPECT_FALSE(received_message.get());
   EXPECT_EQ(net::ERR_FILE_TOO_BIG, last_error());
 }
 
+// Receive a message whose size field was corrupted and takes more than two
+// bytes to encode. Should fail gracefully with a size error.
+TEST_F(GCMConnectionHandlerImplTest, InvalidSizePacket) {
+  std::string handshake_request = EncodeHandshakeRequest();
+  WriteList write_list(1, net::MockWrite(net::ASYNC,
+                                         handshake_request.c_str(),
+                                         handshake_request.size()));
+  std::string handshake_response = EncodeHandshakeResponse();
+
+  // Fill a string with 20000 character zero (which uses more than 2 bytes to
+  // encode the size packet).
+  std::string data_message_proto(20000, '0');
+  std::string data_message_pkt =
+      EncodePacket(kDataMessageStanzaTag, data_message_proto);
+  ReadList read_list;
+  read_list.push_back(net::MockRead(net::ASYNC,
+                                    handshake_response.c_str(),
+                                    handshake_response.size()));
+  read_list.push_back(net::MockRead(net::ASYNC,
+                                    data_message_pkt.c_str(),
+                                    data_message_pkt.size()));
+  BuildSocket(read_list, write_list);
+
+  ScopedMessage received_message;
+  Connect(&received_message);
+  WaitForMessage();  // The login send.
+  WaitForMessage();  // The login response.
+  received_message.reset();
+  WaitForMessage();  // The data message.
+  EXPECT_FALSE(received_message.get());
+  EXPECT_EQ(net::ERR_FILE_TOO_BIG, last_error());
+}
+
+// Make sure a message with an invalid tag is handled gracefully and resets
+// the connection with an invalid argument error.
+TEST_F(GCMConnectionHandlerImplTest, InvalidTag) {
+  std::string handshake_request = EncodeHandshakeRequest();
+  WriteList write_list(1, net::MockWrite(net::ASYNC,
+                                         handshake_request.c_str(),
+                                         handshake_request.size()));
+  std::string handshake_response = EncodeHandshakeResponse();
+
+  std::string invalid_message = "0";
+  std::string invalid_message_pkt =
+      EncodePacket(100, invalid_message);

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

nit: could you name that value? could be a const internal to the test. That is
to call it out, and perhaps fix it in distant future whenever 100 is a correct
message tag. 

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

Done.

+  ReadList read_list;
+  read_list.push_back(net::MockRead(net::ASYNC,
+                                    handshake_response.c_str(),
+                                    handshake_response.size()));
+  read_list.push_back(net::MockRead(net::ASYNC,
+                                    invalid_message_pkt.c_str(),
+                                    invalid_message_pkt.size()));
+  BuildSocket(read_list, write_list);
+
+  ScopedMessage received_message;
+  Connect(&received_message);
+  WaitForMessage();  // The login send.
+  WaitForMessage();  // The login response.
+  received_message.reset();
+  WaitForMessage();  // The invalid message.
+  EXPECT_FALSE(received_message.get());
+  EXPECT_EQ(net::ERR_INVALID_ARGUMENT, last_error());
+}
+
+// Receive a message where the size field spans two socket reads.

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

nit: pleas call out that it is byte 2 and 3 explicitly (that is what I am
reading from the test assuming that the first byte is the tag). It will make it
more obvious why you picked 2 as a split point.

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

Done.

+TEST_F(GCMConnectionHandlerImplTest, RecvMsgSplitSize) {
+  std::string handshake_request = EncodeHandshakeRequest();
+  WriteList write_list(1, net::MockWrite(net::ASYNC,
+                                         handshake_request.c_str(),
+                                         handshake_request.size()));
+  std::string handshake_response = EncodeHandshakeResponse();
+
+  std::string data_message_proto =
+      BuildDataMessage(kDataMsgFromLong, kDataMsgCategoryLong);
+  std::string data_message_pkt =
+      EncodePacket(kDataMessageStanzaTag, data_message_proto);
+  DCHECK_GT(data_message_pkt.size(), 128U);
+  ReadList read_list;
+  read_list.push_back(net::MockRead(net::ASYNC,
+                                    handshake_response.c_str(),
+                                    handshake_response.size()));
+  read_list.push_back(net::MockRead(net::ASYNC,
+                                    data_message_pkt.c_str(),
+                                    2));
+  read_list.push_back(net::MockRead(net::ASYNC,
+                                    data_message_pkt.c_str() + 2,
+                                    data_message_pkt.size() - 2));
+  BuildSocket(read_list, write_list);
+
+  ScopedMessage received_message;
+  Connect(&received_message);
+  WaitForMessage();  // The login send.
+  WaitForMessage();  // The login response.
+  WaitForMessage();  // The data message.
+  ASSERT_TRUE(received_message.get());
+  EXPECT_EQ(data_message_proto, received_message->SerializeAsString());
+  EXPECT_EQ(net::OK, last_error());
+}
+
 }  // namespace
 }  // namespace gcm