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_);

[mark a. foltz, 2014/07/21 17:38:52]

Thank you for fixing this, in my email reply I forgot scoped_ptr overloads * and
does the right thing: CHECK-fails on NULL dereference.

[Kevin M, 2014/07/21 20:48:58]

Acknowledged.

 }
 
 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 372 matching lines @@
     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());

[mark a. foltz, 2014/07/21 17:38:52]

We should audit the use of DCHECKs in the read and connect loop and ensure that
they shouldn't be converted to CHECKs.  It would be better to CHECK-fail the
browser than leave a potental buffer overflow.

[Kevin M, 2014/07/21 20:48:58]

Done here and elsewhere in places that deal with sizes and offsets.

[Wez, 2014/07/22 22:33:09]

Rather than [D]CHECK(), we could do the following:

if (unexpected_condition) {
  NOTREACHED();
  CloseWithError(GENERIC_FAILURE);
  ... return or whatever ...
}

This gets us graceful handling of failures in Release builds, but logged, and
crashes in Debug builds. Downside is no visibility via go/crash into these
issues occurring in the wild.

[Kevin M, 2014/07/23 18:57:33]

Done.

   } 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());
   }
   DCHECK_GT(num_bytes_to_read, 0U);
 
   // Read up to num_bytes_to_read into |current_read_buffer_|.
   return socket_->Read(
@@ skipping 19 matching lines @@
             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;
+      return net::ERR_FAILED;

[Wez, 2014/07/22 22:33:09]

IIUC this change will mean we get read error notifications for CastChannels that
have neither succeeded nor failed connecting yet, which probably isn't what we
want?

[Kevin M, 2014/07/23 18:57:33]

This was removed.

     }
   } 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::ERR_FAILED;
     }
   }
 
   return net::OK;
 }
 
 int CastSocket::DoReadCallback() {
   read_state_ = READ_STATE_READ;
-  const CastMessage& message = *(current_message_.get());
+  const CastMessage& message = *current_message_;
   if (IsAuthMessage(message)) {
     // An auth message is received, check that connect flow is running.
     if (ready_state_ == READY_STATE_CONNECTING) {
       challenge_reply_.reset(new CastMessage(message));
       PostTaskToStartConnectLoop(net::OK);
     } else {
       read_state_ = READ_STATE_ERROR;

[mark a. foltz, 2014/07/21 17:38:52]

This is another place we need to abort the read loop.

[Kevin M, 2014/07/21 20:48:57]

Already done - setting read_state_ to READ_STATE_ERROR has the effect of
finishing the read event loop (indirectly: DoReadError leaves the read state as
READ_STATE_NONE which is an exit condition for the do-while loop).

     }
   } else if (delegate_) {
     MessageInfo message_info;
     if (CastMessageToMessageInfo(message, &message_info))
       delegate_->OnMessage(this, message_info);
     else
       read_state_ = READ_STATE_ERROR;

[mark a. foltz, 2014/07/21 17:38:52]

Ditto

[Kevin M, 2014/07/21 20:48:57]

Acknowledged.

   }
   current_message_->Clear();
   return net::OK;
 }
 
 int CastSocket::DoReadError(int result) {
+  VLOG_WITH_CONNECTION(1) << "DoReadError: " << result;
   DCHECK_LE(result, 0);
+  read_state_ = READ_STATE_NONE;
   // If inside connection flow, then get back to connect loop.

[mark a. foltz, 2014/07/21 17:38:52]

I don't fully understand this piece of logic. A read error should immediately
abort the connection attempt and not restart the connect loop.  I think this is
trying to guarantee that we don't invoke the connect callback (to signal the
error) from a different callback passed to net::socket.  However this is
indirect and I'm not really convinced it's correct (what's the invariant on 
connect_state_ here)?

At the least, make sure that there is a test case that a read error during
connect does the right thing, and possibly add a CHECK-guard to guarantee that
the connect loop exits on a read error.  When Munjal is back, we can maybe think
about if there is a cleaner way to handle the control flow - I believe he has
more context for why we are posting tasks to hop between loops.

[Kevin M, 2014/07/21 20:48:58]

Changed the flow to call the completion callback directly instead of via the
messageloop. PTAL. Given the simplistic nature of the revised function (<any
error>=><ERR_FAILED>) we could eliminate this node from the state machine
entirely...

[mark a. foltz, 2014/07/21 21:23:00]

I'm not 100% comfortable changing the control flow in this way without review by
Munjal.  The issue is that the completion callback may delete the CastSocket on
the API side, and we don't want to do that inside a callback on the net:: side.

Let's do that cleanup in a separate patch.

[Kevin M, 2014/07/21 23:50:31]

Done. OK, back to the previous method.

   if (ready_state_ == READY_STATE_CONNECTING) {
     PostTaskToStartConnectLoop(result);
-    // does not try to report error also.

[Wez, 2014/07/22 22:33:09]

This comment implies that if we return something other than net::OK then we're
going to get a read-failure notification sent to the caller, even though we
haven't yet informed them of the connection succeeding, i.e. when they won't be
expecting one?

-    return net::OK;
   }
+  // Allow the read loop error handler to close the connection.
   return net::ERR_FAILED;
 }
 
 bool CastSocket::ProcessHeader() {
   DCHECK_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;
@@ skipping 101 matching lines @@
   return true;
 }
 
 CastSocket::WriteRequest::~WriteRequest() { }
 
 }  // namespace cast_channel
 }  // namespace api
 }  // namespace extensions
 
 #undef VLOG_WITH_CONNECTION