Fix UAF in new Mojo EDK.

mojo/edk/system/routed_raw_channel.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/routed_raw_channel.h"
 
 #include "base/bind.h"
 #include "base/logging.h"
 #include "mojo/edk/embedder/embedder_internal.h"
 
@@ skipping 41 matching lines @@
       ++i;
     }
   }
 
   if (close_routes_.find(route_id) != close_routes_.end())
     delegate->OnError(ERROR_READ_SHUTDOWN);
 }
 
 void RoutedRawChannel::RemoveRoute(uint64_t route_id) {
   DCHECK(internal::g_io_thread_task_runner->RunsTasksOnCurrentThread());
-  CHECK(routes_.find(route_id) != routes_.end());
+  // We don't check that routes_ contains route_id because it's possible for it
+  // to not have been added yet (i.e. it's waiting to be added later down the
+  // call stack).
   routes_.erase(route_id);
 
   // Only send a message to the other side to close the route if we hadn't
   // received a close route message. Otherwise they would keep going back and
   // forth.
   if (close_routes_.find(route_id) != close_routes_.end()) {
     close_routes_.erase(route_id);
   } else if (channel_) {
     // Default route id of 0 to reach the other side's RoutedRawChannel.
     char message_data[sizeof(uint64_t)];
@@ skipping 53 matching lines @@
     memcpy(&msg->message[0], message_view.main_buffer(),
            message_view.total_size());
     msg->handles = platform_handles.Pass();
     pending_messages_.push_back(msg.Pass());
   }
 }
 
 void RoutedRawChannel::OnError(Error error) {
   DCHECK(internal::g_io_thread_task_runner->RunsTasksOnCurrentThread());
 
-  // This needs to match non-multiplexed MessagePipeDispatcher's destruction of
-  // the channel only when read errors occur.
-  if (error != ERROR_WRITE || routes_.empty()) {
-    channel_->Shutdown();
-    channel_ = nullptr;
-  }
-
-  if (routes_.empty()) {
-    delete this;
-    return;
-  }
-
+  // Note: we must ensure we don't call RawChannel::Shutdown until after we've
+  // called OnError on each route's delegate.
   for (auto it = routes_.begin(); it != routes_.end();) {

[Fady Samuel, 2015/12/17 05:35:02]

I don't really understand this loop Is the intention to skip the first time?
what if the it++ == routes_.end()? Wouldn't following second fail?

[Fady Samuel, 2015/12/17 05:38:33]

nm, it's late and I'm not thinking straight. It's still weird here. Why do you
need cur_it?

[jam, 2015/12/17 05:45:16]

improved comment

     // Handle the delegate calling RemoveRoute in this call.
     auto cur_it = it++;
     cur_it->second->OnError(error);
   }
+
+  if (routes_.empty()) {
+    channel_->Shutdown();
+    channel_ = nullptr;
+    delete this;
+    return;
+  }
 }
 
 }  // namespace edk
 }  // namespace mojo