Fix error handling for message parse errors that occur during connection setup.

chrome/browser/extensions/api/cast_channel/cast_socket.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 "chrome/browser/extensions/api/cast_channel/cast_socket.h"
 
 #include <stdlib.h>
 #include <string.h>
 
 #include "base/bind.h"
@@ skipping 140 matching lines @@
     return false;
   bool result = net::X509Certificate::GetDEREncoded(
      ssl_info.cert->os_cert_handle(), cert);
   if (result)
     VLOG_WITH_CONNECTION(1) << "Successfully extracted peer certificate: "
                             << *cert;
   return result;
 }
 
 bool CastSocket::VerifyChallengeReply() {
-  return AuthenticateChallengeReply(*challenge_reply_.get(), peer_cert_);
+  return AuthenticateChallengeReply(*challenge_reply_, peer_cert_);
 }
 
 void CastSocket::Connect(const net::CompletionCallback& callback) {
   DCHECK(CalledOnValidThread());
   VLOG_WITH_CONNECTION(1) << "Connect readyState = " << ready_state_;
   if (ready_state_ != READY_STATE_NONE) {
     callback.Run(net::ERR_CONNECTION_FAILED);
     return;
   }
   ready_state_ = READY_STATE_CONNECTING;
@@ skipping 353 matching lines @@
         rv = DoRead();
         break;
       case READ_STATE_READ_COMPLETE:
         rv = DoReadComplete(rv);
         break;
       case READ_STATE_DO_CALLBACK:
         rv = DoReadCallback();
         break;
       case READ_STATE_ERROR:
         rv = DoReadError(rv);
+        DCHECK_EQ(read_state_, READ_STATE_NONE);
         break;
       default:
         NOTREACHED() << "BUG in read flow. Unknown state: " << state;
         break;
     }
   } while (rv != net::ERR_IO_PENDING && read_state_ != READ_STATE_NONE);
 
-  // Read loop is done - If the result is ERR_FAILED then close with error.
-  if (rv == net::ERR_FAILED)
-    CloseWithError(error_state_);
+  if (rv == net::ERR_FAILED) {
+    if (ready_state_ == READY_STATE_CONNECTING) {
+      // Read errors during the handshake should notify the caller via
+      // the connect callback, rather than the message event delegate.
+      PostTaskToStartConnectLoop(net::ERR_FAILED);
+    } else {
+      // Connection is already established.
+      // Close and send error status via the message event delegate.
+      CloseWithError(error_state_);
+    }
+  }
 }
 
 int CastSocket::DoRead() {
   read_state_ = READ_STATE_READ_COMPLETE;
   // Figure out whether to read header or body, and the remaining bytes.
   uint32 num_bytes_to_read = 0;
   if (header_read_buffer_->RemainingCapacity() > 0) {
     current_read_buffer_ = header_read_buffer_;
     num_bytes_to_read = header_read_buffer_->RemainingCapacity();
-    DCHECK_LE(num_bytes_to_read, MessageHeader::header_size());
+    if (num_bytes_to_read > MessageHeader::header_size()) {
+      error_state_ = CHANNEL_ERROR_INVALID_MESSAGE;

[mark a. foltz, 2014/07/24 00:32:42]

This means that header_read_buffer_ was originally sized incorrectly, which is a
bug.  If that happens we will simply error out all messages, but not know why. 
Add a LOG(ERROR) explaining why we are closing the channel.

[Wez, 2014/07/24 00:55:55]

If this really indicates a logic error then it should be CHECK_LE(), as in an
earlier revision, since it should never happen but if it does we mustn't
continue processing the header.

[Kevin M, 2014/07/24 17:45:07]

Acknowledged.

[Kevin M, 2014/07/24 17:45:07]

Done.

+      return net::ERR_FAILED;
+    }
   } else {
     DCHECK_GT(current_message_size_, 0U);
     num_bytes_to_read = current_message_size_ - body_read_buffer_->offset();
     current_read_buffer_ = body_read_buffer_;
-    DCHECK_LE(num_bytes_to_read, MessageHeader::max_message_size());
+    if (num_bytes_to_read > MessageHeader::max_message_size()) {
+      error_state_ = CHANNEL_ERROR_INVALID_MESSAGE;

[mark a. foltz, 2014/07/24 00:32:42]

This is also indicative of a bug, as current_message_size_ is constrained to be
between 0 and max_message_size(), and body_read_buffer_->offset() is an offset
into an IOBuffer of capacity max_message_size().  I would like a LOG(ERROR)
explaining the condition.

[Wez, 2014/07/24 00:55:55]

Again, given that this situation in principle can never arise, let's CHECK();
that way we will get crash reports if the logic breaks.

[Kevin M, 2014/07/24 17:45:07]

Acknowledged.

[Kevin M, 2014/07/24 17:45:07]

Done.

+      return net::ERR_FAILED;
+    }
   }
-  DCHECK_GT(num_bytes_to_read, 0U);
+  if (num_bytes_to_read == 0) {
+    error_state_ = CHANNEL_ERROR_INVALID_MESSAGE;

[mark a. foltz, 2014/07/24 00:32:42]

Ditto.

[Kevin M, 2014/07/24 17:45:07]

Done.

+    return net::ERR_FAILED;
+  }
 
   // Read up to num_bytes_to_read into |current_read_buffer_|.
   return socket_->Read(
       current_read_buffer_.get(),
       num_bytes_to_read,
       base::Bind(&CastSocket::DoReadLoop, AsWeakPtr()));
 }
 
 int CastSocket::DoReadComplete(int result) {
   VLOG_WITH_CONNECTION(2) << "DoReadComplete result = " << result
                           << " header offset = "
                           << header_read_buffer_->offset()
                           << " body offset = " << body_read_buffer_->offset();
   if (result <= 0) {  // 0 means EOF: the peer closed the socket
     VLOG_WITH_CONNECTION(1) << "Read error, peer closed the socket";
     error_state_ = CHANNEL_ERROR_SOCKET_ERROR;
     read_state_ = READ_STATE_ERROR;
     return result == 0 ? net::ERR_FAILED : result;
   }
 
   // Some data was read.  Move the offset in the current buffer forward.
-  DCHECK_LE(current_read_buffer_->offset() + result,
-            current_read_buffer_->capacity());
+  CHECK_LE(current_read_buffer_->offset() + result,
+           current_read_buffer_->capacity());
   current_read_buffer_->set_offset(current_read_buffer_->offset() + result);
   read_state_ = READ_STATE_READ;
 
   if (current_read_buffer_.get() == header_read_buffer_.get() &&
       current_read_buffer_->RemainingCapacity() == 0) {
     // A full header is read, process the contents.
     if (!ProcessHeader()) {
       error_state_ = cast_channel::CHANNEL_ERROR_INVALID_MESSAGE;
       read_state_ = READ_STATE_ERROR;
     }
   } else if (current_read_buffer_.get() == body_read_buffer_.get() &&
              static_cast<uint32>(current_read_buffer_->offset()) ==
              current_message_size_) {
     // Full body is read, process the contents.
     if (ProcessBody()) {
       read_state_ = READ_STATE_DO_CALLBACK;
     } else {
       error_state_ = cast_channel::CHANNEL_ERROR_INVALID_MESSAGE;
       read_state_ = READ_STATE_ERROR;
     }
   }
 
   return net::OK;
 }
 
 int CastSocket::DoReadCallback() {
   read_state_ = READ_STATE_READ;
-  const CastMessage& message = *(current_message_.get());
-  if (IsAuthMessage(message)) {
-    // An auth message is received, check that connect flow is running.
-    if (ready_state_ == READY_STATE_CONNECTING) {
+  const CastMessage& message = *current_message_;
+  if (ready_state_ == READY_STATE_CONNECTING) {
+    if (IsAuthMessage(message)) {
       challenge_reply_.reset(new CastMessage(message));
       PostTaskToStartConnectLoop(net::OK);
+      return net::OK;
     } else {
+      // Expected an auth message, got something else instead. Handle as error.
       read_state_ = READ_STATE_ERROR;
+      return net::ERR_INVALID_RESPONSE;
     }
-  } else if (delegate_) {
-    MessageInfo message_info;
-    if (CastMessageToMessageInfo(message, &message_info))
-      delegate_->OnMessage(this, message_info);
-    else
-      read_state_ = READ_STATE_ERROR;
   }
+
+  MessageInfo message_info;
+  if (!CastMessageToMessageInfo(message, &message_info)) {
+    current_message_->Clear();
+    read_state_ = READ_STATE_ERROR;
+    return net::ERR_INVALID_RESPONSE;
+  }
+  delegate_->OnMessage(this, message_info);
   current_message_->Clear();
   return net::OK;
 }
 
 int CastSocket::DoReadError(int result) {
   DCHECK_LE(result, 0);
-  // If inside connection flow, then get back to connect loop.
-  if (ready_state_ == READY_STATE_CONNECTING) {
-    PostTaskToStartConnectLoop(result);
-    // does not try to report error also.
-    return net::OK;
-  }
   return net::ERR_FAILED;
 }
 
 bool CastSocket::ProcessHeader() {
-  DCHECK_EQ(static_cast<uint32>(header_read_buffer_->offset()),
-            MessageHeader::header_size());
+  CHECK_EQ(static_cast<uint32>(header_read_buffer_->offset()),
+           MessageHeader::header_size());
   MessageHeader header;
   MessageHeader::ReadFromIOBuffer(header_read_buffer_.get(), &header);
   if (header.message_size > MessageHeader::max_message_size())
     return false;
 
   VLOG_WITH_CONNECTION(2) << "Parsed header { message_size: "
                           << header.message_size << " }";
   current_message_size_ = header.message_size;
   return true;
 }
 
 bool CastSocket::ProcessBody() {
-  DCHECK_EQ(static_cast<uint32>(body_read_buffer_->offset()),
-            current_message_size_);
+  if (static_cast<uint32>(body_read_buffer_->offset()) !=
+           current_message_size_) {

[Wez, 2014/07/24 00:55:55]

This also indicates a logic error; I can't see a way in which we can get here
without this being true - just CHECK_EQ()?

[Kevin M, 2014/07/24 17:45:07]

Done.

+    return false;
+  }
   if (!current_message_->ParseFromArray(
       body_read_buffer_->StartOfBuffer(), current_message_size_)) {
     return false;
   }
   current_message_size_ = 0;
   header_read_buffer_->set_offset(0);
   body_read_buffer_->set_offset(0);
   current_read_buffer_ = header_read_buffer_;
   return true;
 }
 
 // static
 bool CastSocket::Serialize(const CastMessage& message_proto,
                            std::string* message_data) {
   DCHECK(message_data);
   message_proto.SerializeToString(message_data);
   size_t message_size = message_data->size();
   if (message_size > MessageHeader::max_message_size()) {
     message_data->clear();
     return false;
   }
   CastSocket::MessageHeader header;
-  header.SetMessageSize(message_size);
+  if (!header.SetMessageSize(message_size)) {
+    return false;
+  }
   header.PrependToString(message_data);
   return true;
-};
+}
 
 void CastSocket::CloseWithError(ChannelError error) {
   DCHECK(CalledOnValidThread());
   socket_.reset(NULL);
   ready_state_ = READY_STATE_CLOSED;
   error_state_ = error;
   if (delegate_)
     delegate_->OnError(this, error);
 }
 
 std::string CastSocket::CastUrl() const {
   return ((channel_auth_ == CHANNEL_AUTH_TYPE_SSL_VERIFIED) ?
           "casts://" : "cast://") + ip_endpoint_.ToString();
 }
 
 bool CastSocket::CalledOnValidThread() const {
   return thread_checker_.CalledOnValidThread();
 }
 
 CastSocket::MessageHeader::MessageHeader() : message_size(0) { }
 
-void CastSocket::MessageHeader::SetMessageSize(size_t size) {
-  DCHECK(size < static_cast<size_t>(kuint32max));
-  DCHECK(size > 0);
-  message_size = static_cast<size_t>(size);
+bool CastSocket::MessageHeader::SetMessageSize(size_t size) {
+  message_size = size;
+  return true;

[mark a. foltz, 2014/07/24 00:32:42]

Why return a boolean if it's always true?

[Kevin M, 2014/07/24 17:45:07]

Vestiges from validation in a previous patch. Thanks, fixed.

 }
 
 // TODO(mfoltz): Investigate replacing header serialization with base::Pickle,
 // if bit-for-bit compatible.
 void CastSocket::MessageHeader::PrependToString(std::string* str) {
   MessageHeader output = *this;
   output.message_size = base::HostToNet32(message_size);
   size_t header_size = base::checked_cast<size_t,uint32>(
       MessageHeader::header_size());
   scoped_ptr<char, base::FreeDeleter> char_array(
@@ skipping 30 matching lines @@
   return true;
 }
 
 CastSocket::WriteRequest::~WriteRequest() { }
 
 }  // namespace cast_channel
 }  // namespace api
 }  // namespace extensions
 
 #undef VLOG_WITH_CONNECTION