Fix shutdown UAF in MessagePipeDispatcher in new Mojo EDK.

mojo/edk/system/message_pipe_dispatcher.cc

 // Copyright 2015 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 "mojo/edk/system/message_pipe_dispatcher.h"
 
 #include "base/bind.h"
 #include "base/debug/stack_trace.h"
 #include "base/logging.h"
 #include "base/message_loop/message_loop.h"
@@ skipping 136 matching lines @@
   base::AutoLock locker(lock());
   CHECK(transferable_);
   calling_init_ = true;
   if (channel_)
     channel_->Init(this);
   calling_init_ = false;
 }
 
 void MessagePipeDispatcher::CloseOnIO() {
   base::AutoLock locker(lock());
+  Release();  // To match CloseImplNoLock.
   if (transferable_) {
     if (channel_) {
       channel_->Shutdown();
       channel_ = nullptr;
     }
   } else {
     if (non_transferable_state_ == CONNECT_CALLED ||
         non_transferable_state_ == WAITING_FOR_READ_OR_WRITE) {
       if (non_transferable_state_ == WAITING_FOR_READ_OR_WRITE)
         RequestNontransferableChannel();
@@ skipping 261 matching lines @@
 #endif
 }
 
 void MessagePipeDispatcher::CancelAllAwakablesNoLock() {
   lock().AssertAcquired();
   awakable_list_.CancelAll();
 }
 
 void MessagePipeDispatcher::CloseImplNoLock() {
   lock().AssertAcquired();
+  // We take a manual refcount because at shutdown, the task below might not get
+  // a chance to execute. If that happens, the RawChannel's will still call our

[msw, 2015/12/17 18:49:59]

nit: "The RawChannel will" or "The RawChannel's ____ will"

[jam, 2015/12/17 18:54:36]

will fix in a followup, thanks

+  // OnError method because it always runs (since it watches thread

[msw, 2015/12/17 18:49:59]

q: Should this try to release in OnError?

[jam, 2015/12/17 18:54:36]

(per in-person discussion)
We could add extra logic, although we'd have to handle the case when CloseOnIO
runs, and when OnError is called before either of these. I'm hesitant to add
even more state & complexity to avoid a shutdown leak, since in general shutdown
leaks are ok.

+  // destruction). So to avoid UAF, manually add a reference and only release it
+  // if the task runs.
+  AddRef();
   internal::g_io_thread_task_runner->PostTask(
       FROM_HERE, base::Bind(&MessagePipeDispatcher::CloseOnIO, this));
 }
 
 void MessagePipeDispatcher::SerializeInternal() {
   serialized_ = true;
   if (!transferable_) {
     CHECK(non_transferable_state_ == WAITING_FOR_READ_OR_WRITE)
         << "Non transferable message pipe being sent after read/write/waited. "
         << "MOJO_CREATE_MESSAGE_PIPE_OPTIONS_FLAG_TRANSFERABLE must be used if "
@@ skipping 542 matching lines @@
   // PostTask since the broker can call us back synchronously.
   internal::g_io_thread_task_runner->PostTask(
       FROM_HERE,
       base::Bind(&Broker::ConnectMessagePipe,
                  base::Unretained(internal::g_broker), pipe_id_,
                  base::Unretained(this)));
 }
 
 }  // namespace edk
 }  // namespace mojo