From f8bef1c6b2f9b3585af2dc619246359bba8cfbd1 Mon Sep 17 00:00:00 2001 From: zea Date: Thu, 25 Sep 2014 10:58:32 -0700 Subject: [PATCH] [GCM] Investigatory CHECKs for crash in parsing stream The various places where size_t and uint64 were being used have been converted to int, so that we can better check to make sure they're non-negative (they're actually consumed as ints by the protobuf parsing code). Various CHECKS have therefore been added to verify assumptions. BUG=409985 Review URL: https://codereview.chromium.org/600223003 Cr-Commit-Position: refs/heads/master@{#296741} --- google_apis/gcm/base/socket_stream.cc | 11 +++++-- google_apis/gcm/base/socket_stream.h | 2 +- google_apis/gcm/base/socket_stream_unittest.cc | 36 +++++++++++------------ google_apis/gcm/engine/connection_handler_impl.cc | 6 ++-- 4 files changed, 30 insertions(+), 25 deletions(-) diff --git a/google_apis/gcm/base/socket_stream.cc b/google_apis/gcm/base/socket_stream.cc index 14889e1f1180..edf723cdabda 100644 --- a/google_apis/gcm/base/socket_stream.cc +++ b/google_apis/gcm/base/socket_stream.cc @@ -56,8 +56,9 @@ bool SocketInputStream::Next(const void** data, int* size) { void SocketInputStream::BackUp(int count) { DCHECK(GetState() == READY || GetState() == EMPTY); - DCHECK_GT(count, 0); - DCHECK_LE(count, next_pos_); + // TODO(zea): investigating crbug.com/409985 + CHECK_GT(count, 0); + CHECK_LE(count, next_pos_); next_pos_ -= count; DVLOG(1) << "Backing up " << count << " bytes in input buffer. " @@ -76,7 +77,7 @@ int64 SocketInputStream::ByteCount() const { return next_pos_; } -size_t SocketInputStream::UnreadByteCount() const { +int SocketInputStream::UnreadByteCount() const { DCHECK_NE(GetState(), CLOSED); DCHECK_NE(GetState(), READING); return read_buffer_->BytesConsumed() - next_pos_; @@ -137,6 +138,8 @@ void SocketInputStream::RebuildBuffer() { DVLOG(1) << "Have " << unread_data_size << " unread bytes remaining."; } read_buffer_->DidConsume(unread_data_size); + // TODO(zea): investigating crbug.com/409985 + CHECK_GE(UnreadByteCount(), 0); } net::Error SocketInputStream::last_error() const { @@ -179,6 +182,8 @@ void SocketInputStream::RefreshCompletionCallback( DCHECK_GT(result, 0); last_error_ = net::OK; read_buffer_->DidConsume(result); + // TODO(zea): investigating crbug.com/409985 + CHECK_GT(UnreadByteCount(), 0); DVLOG(1) << "Refresh complete with " << result << " new bytes. " << "Current position " << next_pos_ diff --git a/google_apis/gcm/base/socket_stream.h b/google_apis/gcm/base/socket_stream.h index a45842016f6c..61aa1fefd5f3 100644 --- a/google_apis/gcm/base/socket_stream.h +++ b/google_apis/gcm/base/socket_stream.h @@ -72,7 +72,7 @@ class GCM_EXPORT SocketInputStream virtual int64 ByteCount() const OVERRIDE; // The remaining amount of valid data available to be read. - size_t UnreadByteCount() const; + int UnreadByteCount() const; // Reads from the socket, appending a max of |byte_limit| bytes onto the read // buffer. net::ERR_IO_PENDING is returned if the refresh can't complete diff --git a/google_apis/gcm/base/socket_stream_unittest.cc b/google_apis/gcm/base/socket_stream_unittest.cc index d7ba6793bf9c..10c1f8cc19de 100644 --- a/google_apis/gcm/base/socket_stream_unittest.cc +++ b/google_apis/gcm/base/socket_stream_unittest.cc @@ -20,11 +20,11 @@ typedef std::vector ReadList; typedef std::vector WriteList; const char kReadData[] = "read_data"; -const uint64 kReadDataSize = arraysize(kReadData) - 1; +const int kReadDataSize = arraysize(kReadData) - 1; const char kReadData2[] = "read_alternate_data"; -const uint64 kReadData2Size = arraysize(kReadData2) - 1; +const int kReadData2Size = arraysize(kReadData2) - 1; const char kWriteData[] = "write_data"; -const uint64 kWriteDataSize = arraysize(kWriteData) - 1; +const int kWriteDataSize = arraysize(kWriteData) - 1; class GCMSocketStreamTest : public testing::Test { public: @@ -38,12 +38,12 @@ class GCMSocketStreamTest : public testing::Test { void PumpLoop(); // Simulates a google::protobuf::io::CodedInputStream read. - base::StringPiece DoInputStreamRead(uint64 bytes); + base::StringPiece DoInputStreamRead(int bytes); // Simulates a google::protobuf::io::CodedOutputStream write. - uint64 DoOutputStreamWrite(const base::StringPiece& write_src); + int DoOutputStreamWrite(const base::StringPiece& write_src); // Synchronous Refresh wrapper. - void WaitForData(size_t msg_size); + void WaitForData(int msg_size); base::MessageLoop* message_loop() { return &message_loop_; }; net::DelayedSocketData* data_provider() { return data_provider_.get(); } @@ -101,8 +101,8 @@ void GCMSocketStreamTest::PumpLoop() { run_loop.RunUntilIdle(); } -base::StringPiece GCMSocketStreamTest::DoInputStreamRead(uint64 bytes) { - uint64 total_bytes_read = 0; +base::StringPiece GCMSocketStreamTest::DoInputStreamRead(int bytes) { + int total_bytes_read = 0; const void* initial_buffer = NULL; const void* buffer = NULL; int size = 0; @@ -130,22 +130,22 @@ base::StringPiece GCMSocketStreamTest::DoInputStreamRead(uint64 bytes) { total_bytes_read); } -uint64 GCMSocketStreamTest::DoOutputStreamWrite( +int GCMSocketStreamTest::DoOutputStreamWrite( const base::StringPiece& write_src) { DCHECK_EQ(socket_output_stream_->GetState(), SocketOutputStream::EMPTY); - uint64 total_bytes_written = 0; + int total_bytes_written = 0; void* buffer = NULL; int size = 0; - size_t bytes = write_src.size(); + int bytes = write_src.size(); do { if (!socket_output_stream_->Next(&buffer, &size)) break; - uint64 bytes_to_write = (static_cast(size) < bytes ? size : bytes); + int bytes_to_write = (size < bytes ? size : bytes); memcpy(buffer, write_src.data() + total_bytes_written, bytes_to_write); - if (bytes_to_write < static_cast(size)) + if (bytes_to_write < size) socket_output_stream_->BackUp(size - bytes_to_write); total_bytes_written += bytes_to_write; } while (total_bytes_written < bytes); @@ -159,7 +159,7 @@ uint64 GCMSocketStreamTest::DoOutputStreamWrite( return total_bytes_written; } -void GCMSocketStreamTest::WaitForData(size_t msg_size) { +void GCMSocketStreamTest::WaitForData(int msg_size) { while (input_stream()->UnreadByteCount() < msg_size) { base::RunLoop run_loop; if (input_stream()->Refresh(run_loop.QuitClosure(), @@ -207,8 +207,8 @@ TEST_F(GCMSocketStreamTest, ReadDataSync) { // A read that comes in two parts. TEST_F(GCMSocketStreamTest, ReadPartialDataSync) { - size_t first_read_len = kReadDataSize / 2; - size_t second_read_len = kReadDataSize - first_read_len; + int first_read_len = kReadDataSize / 2; + int second_read_len = kReadDataSize - first_read_len; ReadList read_list; read_list.push_back( net::MockRead(net::SYNCHRONOUS, @@ -227,8 +227,8 @@ TEST_F(GCMSocketStreamTest, ReadPartialDataSync) { // A read where no data is available at first (IO_PENDING will be returned). TEST_F(GCMSocketStreamTest, ReadAsync) { - size_t first_read_len = kReadDataSize / 2; - size_t second_read_len = kReadDataSize - first_read_len; + int first_read_len = kReadDataSize / 2; + int second_read_len = kReadDataSize - first_read_len; ReadList read_list; read_list.push_back( net::MockRead(net::SYNCHRONOUS, net::ERR_IO_PENDING)); diff --git a/google_apis/gcm/engine/connection_handler_impl.cc b/google_apis/gcm/engine/connection_handler_impl.cc index d47e0f25b792..751b1cd1daa5 100644 --- a/google_apis/gcm/engine/connection_handler_impl.cc +++ b/google_apis/gcm/engine/connection_handler_impl.cc @@ -184,9 +184,9 @@ void ConnectionHandlerImpl::WaitForData(ProcessingState state) { } // Used to determine whether a Socket::Read is necessary. - size_t min_bytes_needed = 0; + int min_bytes_needed = 0; // Used to limit the size of the Socket::Read. - size_t max_bytes_needed = 0; + int max_bytes_needed = 0; switch(state) { case MCS_VERSION_TAG_AND_SIZE: @@ -214,7 +214,7 @@ void ConnectionHandlerImpl::WaitForData(ProcessingState state) { } DCHECK_GE(max_bytes_needed, min_bytes_needed); - size_t unread_byte_count = input_stream_->UnreadByteCount(); + int unread_byte_count = input_stream_->UnreadByteCount(); if (min_bytes_needed > unread_byte_count && input_stream_->Refresh( base::Bind(&ConnectionHandlerImpl::WaitForData, -- 2.11.4.GIT