IndexedDB: Reject bad OnDestroyed messages

IndexedDB: Reject bad OnDestroyed messages

Reject a message with a bogus object id, rather than null crashing.

BUG=451752
R=cmumford@chromium.org

Committed: https://crrev.com/aa968c24ec0a90856c6269332756e0c974e3b848
Cr-Commit-Position: refs/heads/master@{#313145}

[jsbell, 2015-01-26 19:11:49]

cmumford@ - please take a look?

[cmumford, 2015-01-26 19:52:15]

https://codereview.chromium.org/874983003/diff/1/content/browser/indexed_db/i...
File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right):

https://codereview.chromium.org/874983003/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_dispatcher_host.cc:618:
parent_->GetOrTerminateProcess(&map_, ipc_object_id);
This still dies on non-official builds right? Will that satisfy ASan if it's
still a failed DCHECK?

[jsbell, 2015-01-26 19:57:21]

jsbell@chromium.org changed reviewers:
+ inferno@chromium.org

[jsbell, 2015-01-26 19:57:22]

inferno@ - can you sanity check that the goal of the bug is to replace a null
crash with BadMessageReceived() call in release builds?

https://codereview.chromium.org/874983003/diff/1/content/browser/indexed_db/i...
File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right):

https://codereview.chromium.org/874983003/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_dispatcher_host.cc:618:
parent_->GetOrTerminateProcess(&map_, ipc_object_id);
On 2015/01/26 19:52:15, cmumford wrote:
> This still dies on non-official builds right? Will that satisfy ASan if it's
> still a failed DCHECK?

Given that we do this for the other IPCs, but *this* one was called out, I'm
guessing the fuzzer is looking for unexpected exits and the other ones are doing
the right thing (i.e. in release builds, BadMessageReceived()).

I'll ask inferno@

[inferno, 2015-01-26 20:00:11]

On 2015/01/26 19:57:22, jsbell wrote:
> inferno@ - can you sanity check that the goal of the bug is to replace a null
> crash with BadMessageReceived() call in release builds?
> 
>
https://codereview.chromium.org/874983003/diff/1/content/browser/indexed_db/i...
> File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right):
> 
>
https://codereview.chromium.org/874983003/diff/1/content/browser/indexed_db/i...
> content/browser/indexed_db/indexed_db_dispatcher_host.cc:618:
> parent_->GetOrTerminateProcess(&map_, ipc_object_id);
> On 2015/01/26 19:52:15, cmumford wrote:
> > This still dies on non-official builds right? Will that satisfy ASan if it's
> > still a failed DCHECK?
> 
> Given that we do this for the other IPCs, but *this* one was called out, I'm
> guessing the fuzzer is looking for unexpected exits and the other ones are
doing
> the right thing (i.e. in release builds, BadMessageReceived()).
> 
> I'll ask inferno@

For bad ipcs, we just ignore them in ipc fuzzing build. ipc build have this
command line set

// Don't kill a child process when it sends a bad IPC message.  Apart
// from testing, it is a bad idea from a security perspective to enable
// this switch.
const char kDisableKillAfterBadIPC[]        = "disable-kill-after-bad-ipc";

[cmumford, 2015-01-26 20:00:46]

lgtm

[jsbell, 2015-01-26 21:22:22]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2015-01-26 21:23:21]

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

[commit-bot: I haz the power, 2015-01-26 21:52:03]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-01-26 21:52:51]

Patchset 1 (id:??) landed as
https://crrev.com/aa968c24ec0a90856c6269332756e0c974e3b848
Cr-Commit-Position: refs/heads/master@{#313145}