Handle unexpected MojoWait() result in MojoAudioDecoderService

Handle unexpected MojoWait() result in MojoAudioDecoderService

Report an error to the client if MojoWait() failed to get
to the readable state instead of crashing.

BUG=610489
Committed: https://crrev.com/90b79c7d69f6b25d3a8c44875cfe6331c6da320a
Cr-Commit-Position: refs/heads/master@{#394941}

[Tima Vaisburd, 2016-05-17 21:39:12]

timav@chromium.org changed reviewers:
+ rockot@chromium.org, xhwang@chromium.org

[Tima Vaisburd, 2016-05-17 21:39:13]

PTAL.

[sandersd (OOO until July 31), 2016-05-17 23:45:09]

On 2016/05/17 21:39:13, Tima Vaisburd wrote:
> PTAL.

FYI, I am doing similar work:
https://codereview.chromium.org/1899363002/diff/200001/media/mojo/services/mo...

Similar changes were required in MojoVideoDecoderService to pass security
review, and you'll probably need the same for this CL.

[Ken Rockot(use gerrit already), 2016-05-17 23:54:53]

https://codereview.chromium.org/1982883003/diff/1/media/mojo/services/mojo_au...
File media/mojo/services/mojo_audio_decoder_service.cc (right):

https://codereview.chromium.org/1982883003/diff/1/media/mojo/services/mojo_au...
media/mojo/services/mojo_audio_decoder_service.cc:161: state.satisfied_signals &
MOJO_HANDLE_SIGNAL_PEER_CLOSED) {
It's possible for PEER_CLOSED to be set while READABLE is also set (data may
have been written and then the producer closed, in which case the consumer can
still read the written data).

This can result in data being left unread. Pretty sure you only want to exit
early if wait_result != MOJO_RESULT_OK.

https://codereview.chromium.org/1982883003/diff/1/media/mojo/services/mojo_au...
media/mojo/services/mojo_audio_decoder_service.cc:166:
CHECK_EQ(MOJO_HANDLE_SIGNAL_READABLE,
Please reserve CHECK usage for exceptional conditions that could actually lead
to security bugs and use DCHECK here instead (or just remove it altogether.)

The API makes this guarantee explicitly: if MojoWait returned MOJO_RESULT_OK,
then READABLE is set.

https://codereview.chromium.org/1982883003/diff/1/media/mojo/services/mojo_au...
media/mojo/services/mojo_audio_decoder_service.cc:177: CHECK_EQ(bytes_to_read,
bytes_read);
What guarantee do you have that the sender sent the right number of bytes? This
should probably not be a CHECK.

[Tima Vaisburd, 2016-05-17 23:56:15]

On 2016/05/17 23:45:09, sandersd wrote:
> On 2016/05/17 21:39:13, Tima Vaisburd wrote:
> > PTAL.
> 
> FYI, I am doing similar work:
>
https://codereview.chromium.org/1899363002/diff/200001/media/mojo/services/mo...
> 
> Similar changes were required in MojoVideoDecoderService to pass security
> review, and you'll probably need the same for this CL.

Thank you for letting me know and I did not realize I need to include you in the
review list, sorry.
You've done a nice cleanup, I need a small CL for an easy merge to M51. After
that, I believe, your CL will just override everything - or do you plan to merge
it in M51?

[Tima Vaisburd, 2016-05-17 23:56:32]

timav@chromium.org changed reviewers:
+ sandersd@chromium.org

[sandersd (OOO until July 31), 2016-05-18 00:00:15]

On 2016/05/17 23:56:15, Tima Vaisburd wrote:
> On 2016/05/17 23:45:09, sandersd wrote:
> > On 2016/05/17 21:39:13, Tima Vaisburd wrote:
> > > PTAL.
> > 
> > FYI, I am doing similar work:
> >
>
https://codereview.chromium.org/1899363002/diff/200001/media/mojo/services/mo...
> > 
> > Similar changes were required in MojoVideoDecoderService to pass security
> > review, and you'll probably need the same for this CL.
> 
> Thank you for letting me know and I did not realize I need to include you in
the
> review list, sorry.
> You've done a nice cleanup, I need a small CL for an easy merge to M51. After
> that, I believe, your CL will just override everything - or do you plan to
merge
> it in M51?

I don't plan to merge for M51, you are free to take whatever you want from my CL
and I will handle any merging.

I didn't change MojoAudioDecoder quite as much as I did MojoVideoDecoder, to
preserve the existing behavior more closely. However, feel free to copy from
MojoVideoDecoder instead (% review comments from above), and I'll merge in the
other direction. (Eventually we will share this code, so identical code would be
helpful.)

[sandersd (OOO until July 31), 2016-05-18 00:00:56]

On 2016/05/18 00:00:15, sandersd wrote:
> On 2016/05/17 23:56:15, Tima Vaisburd wrote:
> > On 2016/05/17 23:45:09, sandersd wrote:
> > > On 2016/05/17 21:39:13, Tima Vaisburd wrote:
> > > > PTAL.
> > > 
> > > FYI, I am doing similar work:
> > >
> >
>
https://codereview.chromium.org/1899363002/diff/200001/media/mojo/services/mo...
> > > 
> > > Similar changes were required in MojoVideoDecoderService to pass security
> > > review, and you'll probably need the same for this CL.
> > 
> > Thank you for letting me know and I did not realize I need to include you in
> the
> > review list, sorry.
> > You've done a nice cleanup, I need a small CL for an easy merge to M51.
After
> > that, I believe, your CL will just override everything - or do you plan to
> merge
> > it in M51?
> 
> I don't plan to merge for M51, you are free to take whatever you want from my
CL
> and I will handle any merging.
> 
> I didn't change MojoAudioDecoder quite as much as I did MojoVideoDecoder, to
> preserve the existing behavior more closely. However, feel free to copy from
> MojoVideoDecoder instead (% review comments from above), and I'll merge in the
> other direction. (Eventually we will share this code, so identical code would
be
> helpful.)

Sorry for using "merge" to mean two things above, let's say instead that I will
handle rebasing.

[Tima Vaisburd, 2016-05-18 01:17:16]

https://codereview.chromium.org/1982883003/diff/1/media/mojo/services/mojo_au...
File media/mojo/services/mojo_audio_decoder_service.cc (right):

https://codereview.chromium.org/1982883003/diff/1/media/mojo/services/mojo_au...
media/mojo/services/mojo_audio_decoder_service.cc:161: state.satisfied_signals &
MOJO_HANDLE_SIGNAL_PEER_CLOSED) {
On 2016/05/17 23:54:53, Ken Rockot wrote:
> It's possible for PEER_CLOSED to be set while READABLE is also set (data may
> have been written and then the producer closed, in which case the consumer can
> still read the written data).
> 
> This can result in data being left unread. Pretty sure you only want to exit
> early if wait_result != MOJO_RESULT_OK.

Done.

https://codereview.chromium.org/1982883003/diff/1/media/mojo/services/mojo_au...
media/mojo/services/mojo_audio_decoder_service.cc:166:
CHECK_EQ(MOJO_HANDLE_SIGNAL_READABLE,
On 2016/05/17 23:54:53, Ken Rockot wrote:
> Please reserve CHECK usage for exceptional conditions that could actually lead
> to security bugs and use DCHECK here instead (or just remove it altogether.)
> 
> The API makes this guarantee explicitly: if MojoWait returned MOJO_RESULT_OK,
> then READABLE is set.

Removed this check.

https://codereview.chromium.org/1982883003/diff/1/media/mojo/services/mojo_au...
media/mojo/services/mojo_audio_decoder_service.cc:177: CHECK_EQ(bytes_to_read,
bytes_read);
On 2016/05/17 23:54:53, Ken Rockot wrote:
> What guarantee do you have that the sender sent the right number of bytes?
This
> should probably not be a CHECK.

The sender calls WriteDataRaw() with ALL_OR_NONE. Now I return an error if the
result is not OK, assuming that for OK the equality in the last check is
guaranteed.

I also changed the sender side correspondingly.

[Ken Rockot(use gerrit already), 2016-05-18 03:10:47]

https://codereview.chromium.org/1982883003/diff/1/media/mojo/services/mojo_au...
File media/mojo/services/mojo_audio_decoder_service.cc (right):

https://codereview.chromium.org/1982883003/diff/1/media/mojo/services/mojo_au...
media/mojo/services/mojo_audio_decoder_service.cc:177: CHECK_EQ(bytes_to_read,
bytes_read);
On 2016/05/18 at 01:17:16, Tima Vaisburd wrote:
> On 2016/05/17 23:54:53, Ken Rockot wrote:
> > What guarantee do you have that the sender sent the right number of bytes?
This
> > should probably not be a CHECK.
> 
> The sender calls WriteDataRaw() with ALL_OR_NONE. Now I return an error if the
result is not OK, assuming that for OK the equality in the last check is
guaranteed.
> 
> I also changed the sender side correspondingly.

If the sender is not always running in a trusted process then the guarantee will
not necessarily be met. I'm not sure what our security policy is in situations
like this, but it seems like CHECKing untrusted inputs in the browser process is
not good if not necessary. You could just return an error if the size is
unexpected.

[Tima Vaisburd, 2016-05-18 17:55:42]

On 2016/05/18 03:10:47, Ken Rockot wrote:
>
https://codereview.chromium.org/1982883003/diff/1/media/mojo/services/mojo_au...
> File media/mojo/services/mojo_audio_decoder_service.cc (right):
> 
>
https://codereview.chromium.org/1982883003/diff/1/media/mojo/services/mojo_au...
> media/mojo/services/mojo_audio_decoder_service.cc:177: CHECK_EQ(bytes_to_read,
> bytes_read);
> On 2016/05/18 at 01:17:16, Tima Vaisburd wrote:
> > On 2016/05/17 23:54:53, Ken Rockot wrote:
> > > What guarantee do you have that the sender sent the right number of bytes?
> This
> > > should probably not be a CHECK.
> > 
> > The sender calls WriteDataRaw() with ALL_OR_NONE. Now I return an error if
the
> result is not OK, assuming that for OK the equality in the last check is
> guaranteed.
> > 
> > I also changed the sender side correspondingly.
> 
> If the sender is not always running in a trusted process then the guarantee
will
> not necessarily be met. I'm not sure what our security policy is in situations
> like this, but it seems like CHECKing untrusted inputs in the browser process
is
> not good if not necessary. You could just return an error if the size is
> unexpected.

Added size check in the sender. The sender is in the renderer process, is it
considered trusted?

[Ken Rockot(use gerrit already), 2016-05-18 18:21:16]

On 2016/05/18 at 17:55:42, timav wrote:
> On 2016/05/18 03:10:47, Ken Rockot wrote:
> >
https://codereview.chromium.org/1982883003/diff/1/media/mojo/services/mojo_au...
> > File media/mojo/services/mojo_audio_decoder_service.cc (right):
> > 
> >
https://codereview.chromium.org/1982883003/diff/1/media/mojo/services/mojo_au...
> > media/mojo/services/mojo_audio_decoder_service.cc:177:
CHECK_EQ(bytes_to_read,
> > bytes_read);
> > On 2016/05/18 at 01:17:16, Tima Vaisburd wrote:
> > > On 2016/05/17 23:54:53, Ken Rockot wrote:
> > > > What guarantee do you have that the sender sent the right number of
bytes?
> > This
> > > > should probably not be a CHECK.
> > > 
> > > The sender calls WriteDataRaw() with ALL_OR_NONE. Now I return an error if
the
> > result is not OK, assuming that for OK the equality in the last check is
> > guaranteed.
> > > 
> > > I also changed the sender side correspondingly.
> > 
> > If the sender is not always running in a trusted process then the guarantee
will
> > not necessarily be met. I'm not sure what our security policy is in
situations
> > like this, but it seems like CHECKing untrusted inputs in the browser
process is
> > not good if not necessary. You could just return an error if the size is
> > unexpected.
> 
> Added size check in the sender. The sender is in the renderer process, is it
considered trusted?

No - renderers are sandboxed because they are inherently untrustworthy

[Ken Rockot(use gerrit already), 2016-05-18 18:22:09]

On 2016/05/18 at 18:21:16, Ken Rockot wrote:
> On 2016/05/18 at 17:55:42, timav wrote:
> > On 2016/05/18 03:10:47, Ken Rockot wrote:
> > >
https://codereview.chromium.org/1982883003/diff/1/media/mojo/services/mojo_au...
> > > File media/mojo/services/mojo_audio_decoder_service.cc (right):
> > > 
> > >
https://codereview.chromium.org/1982883003/diff/1/media/mojo/services/mojo_au...
> > > media/mojo/services/mojo_audio_decoder_service.cc:177:
CHECK_EQ(bytes_to_read,
> > > bytes_read);
> > > On 2016/05/18 at 01:17:16, Tima Vaisburd wrote:
> > > > On 2016/05/17 23:54:53, Ken Rockot wrote:
> > > > > What guarantee do you have that the sender sent the right number of
bytes?
> > > This
> > > > > should probably not be a CHECK.
> > > > 
> > > > The sender calls WriteDataRaw() with ALL_OR_NONE. Now I return an error
if the
> > > result is not OK, assuming that for OK the equality in the last check is
> > > guaranteed.
> > > > 
> > > > I also changed the sender side correspondingly.
> > > 
> > > If the sender is not always running in a trusted process then the
guarantee will
> > > not necessarily be met. I'm not sure what our security policy is in
situations
> > > like this, but it seems like CHECKing untrusted inputs in the browser
process is
> > > not good if not necessary. You could just return an error if the size is
> > > unexpected.
> > 
> > Added size check in the sender. The sender is in the renderer process, is it
considered trusted?
> 
> No - renderers are sandboxed because they are inherently untrustworthy

i.e. a renderer is much more likely to have exploitable vulnerabilities and thus
be compromised. We can never trust that a renderer is going to do what we've
programmed it to do.

[Tima Vaisburd, 2016-05-18 18:26:33]

On 2016/05/18 18:22:09, Ken Rockot wrote:
> On 2016/05/18 at 18:21:16, Ken Rockot wrote:
> > On 2016/05/18 at 17:55:42, timav wrote:
> > > On 2016/05/18 03:10:47, Ken Rockot wrote:
> > > >
>
https://codereview.chromium.org/1982883003/diff/1/media/mojo/services/mojo_au...
> > > > File media/mojo/services/mojo_audio_decoder_service.cc (right):
> > > > 
> > > >
>
https://codereview.chromium.org/1982883003/diff/1/media/mojo/services/mojo_au...
> > > > media/mojo/services/mojo_audio_decoder_service.cc:177:
> CHECK_EQ(bytes_to_read,
> > > > bytes_read);
> > > > On 2016/05/18 at 01:17:16, Tima Vaisburd wrote:
> > > > > On 2016/05/17 23:54:53, Ken Rockot wrote:
> > > > > > What guarantee do you have that the sender sent the right number of
> bytes?
> > > > This
> > > > > > should probably not be a CHECK.
> > > > > 
> > > > > The sender calls WriteDataRaw() with ALL_OR_NONE. Now I return an
error
> if the
> > > > result is not OK, assuming that for OK the equality in the last check is
> > > > guaranteed.
> > > > > 
> > > > > I also changed the sender side correspondingly.
> > > > 
> > > > If the sender is not always running in a trusted process then the
> guarantee will
> > > > not necessarily be met. I'm not sure what our security policy is in
> situations
> > > > like this, but it seems like CHECKing untrusted inputs in the browser
> process is
> > > > not good if not necessary. You could just return an error if the size is
> > > > unexpected.
> > > 
> > > Added size check in the sender. The sender is in the renderer process, is
it
> considered trusted?
> > 
> > No - renderers are sandboxed because they are inherently untrustworthy
> 
> i.e. a renderer is much more likely to have exploitable vulnerabilities and
thus
> be compromised. We can never trust that a renderer is going to do what we've
> programmed it to do.

I see.
Regarding this CL: is there anything else you would recommend to improve?

[Ken Rockot(use gerrit already), 2016-05-18 19:12:46]

Nope, lgtm - thanks!

[xhwang, 2016-05-19 18:59:59]

lgtm % nit

https://codereview.chromium.org/1982883003/diff/40001/media/mojo/services/moj...
File media/mojo/services/mojo_audio_decoder.cc (right):

https://codereview.chromium.org/1982883003/diff/40001/media/mojo/services/moj...
media/mojo/services/mojo_audio_decoder.cc:237: return nullptr;
Can you put a DVLOG(1) here?

[sandersd (OOO until July 31), 2016-05-19 20:22:59]

https://codereview.chromium.org/1982883003/diff/40001/media/mojo/services/moj...
File media/mojo/services/mojo_audio_decoder_service.cc (right):

https://codereview.chromium.org/1982883003/diff/40001/media/mojo/services/moj...
media/mojo/services/mojo_audio_decoder_service.cc:166: uint32_t data_size =
base::checked_cast<uint32_t>(media_buffer->data_size());
This should not be a checked cast.

[Tima Vaisburd, 2016-05-19 22:00:57]

https://codereview.chromium.org/1982883003/diff/40001/media/mojo/services/moj...
File media/mojo/services/mojo_audio_decoder.cc (right):

https://codereview.chromium.org/1982883003/diff/40001/media/mojo/services/moj...
media/mojo/services/mojo_audio_decoder.cc:237: return nullptr;
On 2016/05/19 18:59:59, xhwang wrote:
> Can you put a DVLOG(1) here?

Done.

https://codereview.chromium.org/1982883003/diff/40001/media/mojo/services/moj...
File media/mojo/services/mojo_audio_decoder_service.cc (right):

https://codereview.chromium.org/1982883003/diff/40001/media/mojo/services/moj...
media/mojo/services/mojo_audio_decoder_service.cc:166: uint32_t data_size =
base::checked_cast<uint32_t>(media_buffer->data_size());
On 2016/05/19 20:22:58, sandersd wrote:
> This should not be a checked cast.

Changed to static_cast<>

[sandersd (OOO until July 31), 2016-05-19 23:22:58]

lgtm

[Tima Vaisburd, 2016-05-19 23:26:25]

The CQ bit was checked by timav@chromium.org

[Tima Vaisburd, 2016-05-19 23:26:26]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
xhwang@chromium.org, sandersd@chromium.org
Link to the patchset: https://codereview.chromium.org/1982883003/#ps80001
(title: "Rebase only")

[commit-bot: I haz the power, 2016-05-19 23:26:57]

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

[commit-bot: I haz the power, 2016-05-20 01:00:20]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-20 01:03:25]

Description was changed from

==========
Handle unexpected MojoWait() result in MojoAudioDecoderService

Report an error to the client if MojoWait() failed to get
to the readable state instead of crashing.

BUG=610489
==========

to

==========
Handle unexpected MojoWait() result in MojoAudioDecoderService

Report an error to the client if MojoWait() failed to get
to the readable state instead of crashing.

BUG=610489

Committed: https://crrev.com/90b79c7d69f6b25d3a8c44875cfe6331c6da320a
Cr-Commit-Position: refs/heads/master@{#394941}
==========

[commit-bot: I haz the power, 2016-05-20 01:03:26]

Patchset 5 (id:??) landed as
https://crrev.com/90b79c7d69f6b25d3a8c44875cfe6331c6da320a
Cr-Commit-Position: refs/heads/master@{#394941}

[Will Harris, 2016-07-28 21:41:41]

I have asked a question on issue 610489 about this CL.