IPC::ChannelMojo: Make IPC handling robust against bad messages.

IPC::ChannelMojo: Make IPC handling robust against bad messages.

This change replaces some DCHECK()s with real error handling.
They happen when it receives broken messages.

BUG=428800
R=viettrungluu@chromium.org

Committed: https://crrev.com/185ffdf58f85e47db9666d2ad9b80ff9bd7fa54b
Cr-Commit-Position: refs/heads/master@{#304102}

[Hajime Morrita, 2014-11-13 20:24:17]

This is a speculative fix for ipz_fuzzer based crash

[viettrungluu, 2014-11-13 20:27:59]

https://codereview.chromium.org/725733002/diff/1/ipc/mojo/ipc_mojo_bootstrap.cc
File ipc/mojo/ipc_mojo_bootstrap.cc (right):

https://codereview.chromium.org/725733002/diff/1/ipc/mojo/ipc_mojo_bootstrap....
ipc/mojo/ipc_mojo_bootstrap.cc:103: if (state() != STATE_WAITING_ACK) {
You should probably document in the header what the valid state transitions are.

https://codereview.chromium.org/725733002/diff/1/ipc/mojo/ipc_mojo_bootstrap....
ipc/mojo/ipc_mojo_bootstrap.cc:104: DLOG(WARNING) << "Got inconsistent message
from client.";
LOG(ERROR)?

This seems pretty serious.

https://codereview.chromium.org/725733002/diff/1/ipc/mojo/ipc_mojo_bootstrap....
ipc/mojo/ipc_mojo_bootstrap.cc:109: DCHECK(server_pipe_.is_valid());
CHECK?

https://codereview.chromium.org/725733002/diff/1/ipc/mojo/ipc_mojo_bootstrap....
ipc/mojo/ipc_mojo_bootstrap.cc:136: if (STATE_INITIALIZED != state()) {
state() != STATE_INITIALIZED

https://codereview.chromium.org/725733002/diff/1/ipc/mojo/ipc_mojo_bootstrap....
ipc/mojo/ipc_mojo_bootstrap.cc:137: DLOG(WARNING) << "Got inconsistent message
from server.";
LOG(ERROR)?

[Hajime Morrita, 2014-11-13 21:01:19]

Thanks for the review Trung! PTAL?

https://codereview.chromium.org/725733002/diff/1/ipc/mojo/ipc_mojo_bootstrap.cc
File ipc/mojo/ipc_mojo_bootstrap.cc (right):

https://codereview.chromium.org/725733002/diff/1/ipc/mojo/ipc_mojo_bootstrap....
ipc/mojo/ipc_mojo_bootstrap.cc:103: if (state() != STATE_WAITING_ACK) {
On 2014/11/13 20:27:57, viettrungluu wrote:
> You should probably document in the header what the valid state transitions
are.

Done.

https://codereview.chromium.org/725733002/diff/1/ipc/mojo/ipc_mojo_bootstrap....
ipc/mojo/ipc_mojo_bootstrap.cc:104: DLOG(WARNING) << "Got inconsistent message
from client.";
On 2014/11/13 20:27:57, viettrungluu wrote:
> LOG(ERROR)?
> 
> This seems pretty serious.

Done.

https://codereview.chromium.org/725733002/diff/1/ipc/mojo/ipc_mojo_bootstrap....
ipc/mojo/ipc_mojo_bootstrap.cc:109: DCHECK(server_pipe_.is_valid());
On 2014/11/13 20:27:57, viettrungluu wrote:
> CHECK?

Done.

https://codereview.chromium.org/725733002/diff/1/ipc/mojo/ipc_mojo_bootstrap....
ipc/mojo/ipc_mojo_bootstrap.cc:136: if (STATE_INITIALIZED != state()) {
On 2014/11/13 20:27:57, viettrungluu wrote:
> state() != STATE_INITIALIZED

Done.

https://codereview.chromium.org/725733002/diff/1/ipc/mojo/ipc_mojo_bootstrap....
ipc/mojo/ipc_mojo_bootstrap.cc:137: DLOG(WARNING) << "Got inconsistent message
from server.";
On 2014/11/13 20:27:57, viettrungluu wrote:
> LOG(ERROR)?

Done.

[viettrungluu, 2014-11-13 21:15:26]

LGTM

https://codereview.chromium.org/725733002/diff/20001/ipc/mojo/ipc_mojo_bootst...
File ipc/mojo/ipc_mojo_bootstrap.cc (right):

https://codereview.chromium.org/725733002/diff/20001/ipc/mojo/ipc_mojo_bootst...
ipc/mojo/ipc_mojo_bootstrap.cc:105: DLOG(ERROR) << "Got inconsistent message
from client.";
I actually suggested LOG, not DLOG, but I'll leave that up to you.

(The problem is that DLOG isn't likely to actually give any meaningful output in
the field, e.g., if a user encounters malicious content or when being fuzzed.)

[Hajime Morrita, 2014-11-13 21:18:26]

On 2014/11/13 21:15:26, viettrungluu wrote:
> LGTM
> 
>
https://codereview.chromium.org/725733002/diff/20001/ipc/mojo/ipc_mojo_bootst...
> File ipc/mojo/ipc_mojo_bootstrap.cc (right):
> 
>
https://codereview.chromium.org/725733002/diff/20001/ipc/mojo/ipc_mojo_bootst...
> ipc/mojo/ipc_mojo_bootstrap.cc:105: DLOG(ERROR) << "Got inconsistent message
> from client.";
> I actually suggested LOG, not DLOG, but I'll leave that up to you.
> 
> (The problem is that DLOG isn't likely to actually give any meaningful output
in
> the field, e.g., if a user encounters malicious content or when being fuzzed.)
That is right. will LOG() there.

[Hajime Morrita, 2014-11-13 21:18:34]

The CQ bit was checked by morrita@chromium.org

[commit-bot: I haz the power, 2014-11-13 21:19:48]

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

[commit-bot: I haz the power, 2014-11-13 22:54:22]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-11-13 22:55:11]

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

[inferno, 2015-01-29 20:34:22]

On 2014/11/13 22:55:11, I haz the power (commit-bot) wrote:
> Patchset 3 (id:??) landed as
> https://crrev.com/185ffdf58f85e47db9666d2ad9b80ff9bd7fa54b
> Cr-Commit-Position: refs/heads/master@{#304102}

This broke ipc fuzzer {https://code.google.com/p/chromium/wiki/IPCFuzzer} which
we want to continue using to find ipc bugs. reverting for now, lets do the
discussion in the bug itself.

[inferno, 2015-01-29 20:34:48]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/883373002/ by inferno@chromium.org.

The reason for reverting is: Broke ipc fuzzer.