Fix UAF in new Mojo EDK.

Fix UAF in new Mojo EDK.

The problem was that MessagePipeDispatcher was calling Broker::CloseMessagePipe asynchronously, and in between the time that the posted task was run the MPD could be deleted and called back by the channel for another error.

Since all these methods are now called on the IO thread only, fix this by allowing reentrancy for CloseMessagePipe.

BUG=561803
TEST= linux_chromeos browser_tests pass with new EDK

Committed: https://crrev.com/7f48f0a4073d7ec74eea40aa85a9ebfa506236ce
Cr-Commit-Position: refs/heads/master@{#365782}

[jam, 2015-12-17 05:10:35]

Description was changed from

==========
Fix UAF in new Mojo EDK.
The problem was that MessagePipeDispatcher was calling Broker::CloseMessagePipe
asynchronously, and in between the time that the posted task was run the MPD
could be deleted and called back by the channel for another error.

Since all these methods are now called on the IO thread only, fix this by
allowing reentrancy for CloseMessagePipe.

BUG=561803
TEST= linux_chromeos browser_tests pass with new EDK
==========

to

==========
Fix UAF in new Mojo EDK.

The problem was that MessagePipeDispatcher was calling Broker::CloseMessagePipe
asynchronously, and in between the time that the posted task was run the MPD
could be deleted and called back by the channel for another error.

Since all these methods are now called on the IO thread only, fix this by
allowing reentrancy for CloseMessagePipe.

BUG=561803
TEST= linux_chromeos browser_tests pass with new EDK
==========

[jam, 2015-12-17 05:32:48]

jam@chromium.org changed reviewers:
+ fsamuel@chromium.org

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

https://codereview.chromium.org/1537593002/diff/1/mojo/edk/system/routed_raw_...
File mojo/edk/system/routed_raw_channel.cc (right):

https://codereview.chromium.org/1537593002/diff/1/mojo/edk/system/routed_raw_...
mojo/edk/system/routed_raw_channel.cc:140: for (auto it = routes_.begin(); it !=
routes_.end();) {
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]

https://codereview.chromium.org/1537593002/diff/1/mojo/edk/system/routed_raw_...
File mojo/edk/system/routed_raw_channel.cc (right):

https://codereview.chromium.org/1537593002/diff/1/mojo/edk/system/routed_raw_...
mojo/edk/system/routed_raw_channel.cc:140: for (auto it = routes_.begin(); it !=
routes_.end();) {
On 2015/12/17 05:35:02, Fady Samuel wrote:
> 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?

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

[Fady Samuel, 2015-12-17 05:44:55]

On 2015/12/17 05:38:33, Fady Samuel wrote:
>
https://codereview.chromium.org/1537593002/diff/1/mojo/edk/system/routed_raw_...
> File mojo/edk/system/routed_raw_channel.cc (right):
> 
>
https://codereview.chromium.org/1537593002/diff/1/mojo/edk/system/routed_raw_...
> mojo/edk/system/routed_raw_channel.cc:140: for (auto it = routes_.begin(); it
!=
> routes_.end();) {
> On 2015/12/17 05:35:02, Fady Samuel wrote:
> > 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?
> 
> nm, it's late and I'm not thinking straight. It's still weird here. Why do you
> need cur_it?

After chatting offline, I now understand that OnError may delete the current
item. A clearer comment would be nice. LGTM.

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

https://codereview.chromium.org/1537593002/diff/1/mojo/edk/system/routed_raw_...
File mojo/edk/system/routed_raw_channel.cc (right):

https://codereview.chromium.org/1537593002/diff/1/mojo/edk/system/routed_raw_...
mojo/edk/system/routed_raw_channel.cc:140: for (auto it = routes_.begin(); it !=
routes_.end();) {
On 2015/12/17 05:38:33, Fady Samuel wrote:
> On 2015/12/17 05:35:02, Fady Samuel wrote:
> > 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?
> 
> nm, it's late and I'm not thinking straight. It's still weird here. Why do you
> need cur_it?

improved comment

[jam, 2015-12-17 05:45:24]

The CQ bit was checked by jam@chromium.org

[jam, 2015-12-17 05:45:25]

The patchset sent to the CQ was uploaded after l-g-t-m from fsamuel@chromium.org
Link to the patchset: https://codereview.chromium.org/1537593002/#ps40001
(title: "improve comment")

[commit-bot: I haz the power, 2015-12-17 05:45:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1537593002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1537593002/40001

[commit-bot: I haz the power, 2015-12-17 08:18:41]

Description was changed from

==========
Fix UAF in new Mojo EDK.

The problem was that MessagePipeDispatcher was calling Broker::CloseMessagePipe
asynchronously, and in between the time that the posted task was run the MPD
could be deleted and called back by the channel for another error.

Since all these methods are now called on the IO thread only, fix this by
allowing reentrancy for CloseMessagePipe.

BUG=561803
TEST= linux_chromeos browser_tests pass with new EDK
==========

to

==========
Fix UAF in new Mojo EDK.

The problem was that MessagePipeDispatcher was calling Broker::CloseMessagePipe
asynchronously, and in between the time that the posted task was run the MPD
could be deleted and called back by the channel for another error.

Since all these methods are now called on the IO thread only, fix this by
allowing reentrancy for CloseMessagePipe.

BUG=561803
TEST= linux_chromeos browser_tests pass with new EDK
==========

[commit-bot: I haz the power, 2015-12-17 08:18:42]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-12-17 08:19:28]

Description was changed from

==========
Fix UAF in new Mojo EDK.

The problem was that MessagePipeDispatcher was calling Broker::CloseMessagePipe
asynchronously, and in between the time that the posted task was run the MPD
could be deleted and called back by the channel for another error.

Since all these methods are now called on the IO thread only, fix this by
allowing reentrancy for CloseMessagePipe.

BUG=561803
TEST= linux_chromeos browser_tests pass with new EDK
==========

to

==========
Fix UAF in new Mojo EDK.

The problem was that MessagePipeDispatcher was calling Broker::CloseMessagePipe
asynchronously, and in between the time that the posted task was run the MPD
could be deleted and called back by the channel for another error.

Since all these methods are now called on the IO thread only, fix this by
allowing reentrancy for CloseMessagePipe.

BUG=561803
TEST= linux_chromeos browser_tests pass with new EDK

Committed: https://crrev.com/7f48f0a4073d7ec74eea40aa85a9ebfa506236ce
Cr-Commit-Position: refs/heads/master@{#365782}
==========

[commit-bot: I haz the power, 2015-12-17 08:19:29]

Patchset 3 (id:??) landed as
https://crrev.com/7f48f0a4073d7ec74eea40aa85a9ebfa506236ce
Cr-Commit-Position: refs/heads/master@{#365782}