Handle unexpected MojoWait() result in MojoAudioDecoderService

media/mojo/services/mojo_audio_decoder_service.cc

 // Copyright 2016 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 "media/mojo/services/mojo_audio_decoder_service.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/logging.h"
 #include "media/base/cdm_context.h"
@@ skipping 135 matching lines @@
 scoped_refptr<DecoderBuffer> MojoAudioDecoderService::ReadDecoderBuffer(
     mojom::DecoderBufferPtr buffer) {
   scoped_refptr<DecoderBuffer> media_buffer(
       buffer.To<scoped_refptr<DecoderBuffer>>());
 
   if (media_buffer->end_of_stream())
     return media_buffer;
 
   // Wait for the data to become available in the DataPipe.
   MojoHandleSignalsState state;
-  CHECK_EQ(MOJO_RESULT_OK,
-           MojoWait(consumer_handle_.get().value(), MOJO_HANDLE_SIGNAL_READABLE,
-                    MOJO_DEADLINE_INDEFINITE, &state));
+  MojoResult wait_result =
+      MojoWait(consumer_handle_.get().value(), MOJO_HANDLE_SIGNAL_READABLE,
+               MOJO_DEADLINE_INDEFINITE, &state);
 
-  if (state.satisfied_signals & MOJO_HANDLE_SIGNAL_PEER_CLOSED) {
+  if (wait_result != MOJO_RESULT_OK ||
+      state.satisfied_signals & MOJO_HANDLE_SIGNAL_PEER_CLOSED) {

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

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.

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

Done.

     DVLOG(1) << __FUNCTION__ << ": Peer closed the data pipe";
     return scoped_refptr<DecoderBuffer>();
   }
 
   CHECK_EQ(MOJO_HANDLE_SIGNAL_READABLE,

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

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.

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

Removed this check.

            state.satisfied_signals & MOJO_HANDLE_SIGNAL_READABLE);
 
   // Read the inner data for the DecoderBuffer from our DataPipe.
   uint32_t bytes_to_read =
       base::checked_cast<uint32_t>(media_buffer->data_size());
   DCHECK_GT(bytes_to_read, 0u);
   uint32_t bytes_read = bytes_to_read;
   CHECK_EQ(ReadDataRaw(consumer_handle_.get(), media_buffer->writable_data(),
                        &bytes_read, MOJO_READ_DATA_FLAG_ALL_OR_NONE),
            MOJO_RESULT_OK);
   CHECK_EQ(bytes_to_read, bytes_read);

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

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/18 01:17:16]

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]

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.

 
   return media_buffer;
 }
 
 }  // namespace media