Validate bitstream_data_size in AudioSyncReader.

In case the compressed data that is sent to the browser is larger than the
allocated buffer, we shouldn't try to use the buffer. We should also be a bit
careful with the bitstream_frames field, since it comes from an IPC call and
thus isn't trustworthy.

Bug: 792422
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: I1f6c02a4947b593375575da659573b1a93b93bd3
Reviewed-on: https://chromium-review.googlesource.com/810604Reviewed-by: Olga Sharonova <olka@chromium.org>
Commit-Queue: Max Morin <maxmorin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522107}

content/browser/renderer_host/media/audio_sync_reader.cc

@@ -14,6 +14,7 @@
 #include "base/memory/ptr_util.h"
 #include "base/memory/shared_memory.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/numerics/safe_conversions.h"
 #include "base/strings/stringprintf.h"
 #include "base/trace_event/trace_event.h"
 #include "build/build_config.h"
@@ -54,6 +55,10 @@ AudioSyncReader::AudioSyncReader(
           switches::kMuteAudio)),
       had_socket_error_(false),
       socket_(std::move(socket)),
+      // Validated for reasonable size in Create.
+      output_bus_buffer_size_(
+          AudioBus::CalculateMemorySize(params.channels(),
+                                        params.frames_per_buffer())),
       renderer_callback_count_(0),
       renderer_missed_callback_count_(0),
       trailing_renderer_missed_callback_count_(0),
@@ -209,8 +214,17 @@ void AudioSyncReader::Read(AudioBus* dest) {
     // For bitstream formats, we need the real data size and PCM frame count.
     AudioOutputBuffer* buffer =
         reinterpret_cast<AudioOutputBuffer*>(shared_memory_->memory());
-    output_bus_->SetBitstreamDataSize(buffer->params.bitstream_data_size);
-    output_bus_->SetBitstreamFrames(buffer->params.bitstream_frames);
+    uint32_t data_size = buffer->params.bitstream_data_size;
+    uint32_t bitstream_frames = buffer->params.bitstream_frames;
+    // |bitstream_frames| is cast to int below, so it must fit.
+    if (data_size > output_bus_buffer_size_ ||
+        !base::IsValueInRangeForNumericType<int>(bitstream_frames)) {
+      // Received data doesn't fit in the buffer, shouldn't happen.
+      dest->Zero();
+      return;
+    }
+    output_bus_->SetBitstreamDataSize(data_size);
+    output_bus_->SetBitstreamFrames(bitstream_frames);
   }
   output_bus_->CopyTo(dest);
 }

content/browser/renderer_host/media/audio_sync_reader.h

@@ -79,6 +79,8 @@ class CONTENT_EXPORT AudioSyncReader
   // Socket for transmitting audio data.
   std::unique_ptr<base::CancelableSyncSocket> socket_;
 
+  const uint32_t output_bus_buffer_size_;
+
   // Shared memory wrapper used for transferring audio data to Read() callers.
   std::unique_ptr<media::AudioBus> output_bus_;
 

content/browser/renderer_host/media/audio_sync_reader_unittest.cc

+// Copyright 2017 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 "content/browser/renderer_host/media/audio_sync_reader.h"
+
+#include <limits>
+#include <memory>
+#include <type_traits>
+#include <utility>
+
+#include "base/memory/shared_memory.h"
+#include "base/sync_socket.h"
+#include "base/time/time.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "media/base/audio_bus.h"
+#include "media/base/audio_parameters.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using ::testing::TestWithParam;
+
+namespace content {
+
+static_assert(
+    std::is_unsigned<decltype(
+        media::AudioOutputBufferParameters::bitstream_data_size)>::value,
+    "If |bitstream_data_size| is ever made signed, add tests for negative "
+    "buffer sizes.");
+enum OverflowTestCase {
+  kZero,
+  kNoOverflow,
+  kOverflowByOne,
+  kOverflowByOneThousand,
+  kOverflowByMax
+};
+
+static const OverflowTestCase overflow_test_case_values[]{
+    kZero, kNoOverflow, kOverflowByOne, kOverflowByOneThousand, kOverflowByMax};
+
+class AudioSyncReaderBitstreamTest : public TestWithParam<OverflowTestCase> {
+ public:
+  AudioSyncReaderBitstreamTest() {}
+  ~AudioSyncReaderBitstreamTest() override {}
+
+ private:
+  TestBrowserThreadBundle threads;
+};
+
+TEST_P(AudioSyncReaderBitstreamTest, BitstreamBufferOverflow_DoesNotWriteOOB) {
+  const int kSampleRate = 44100;
+  const int kBitsPerSample = 32;
+  const int kFramesPerBuffer = 1;
+  media::AudioParameters params(media::AudioParameters::AUDIO_BITSTREAM_AC3,
+                                media::CHANNEL_LAYOUT_STEREO, kSampleRate,
+                                kBitsPerSample, kFramesPerBuffer);
+
+  auto socket = std::make_unique<base::CancelableSyncSocket>();
+  std::unique_ptr<media::AudioBus> output_bus = media::AudioBus::Create(params);
+  std::unique_ptr<AudioSyncReader> reader =
+      AudioSyncReader::Create(params, socket.get());
+  const base::SharedMemory* shmem = reader->shared_memory();
+  media::AudioOutputBuffer* buffer =
+      reinterpret_cast<media::AudioOutputBuffer*>(shmem->memory());
+  reader->RequestMoreData(base::TimeDelta(), base::TimeTicks(), 0);
+
+  uint32_t signal;
+  EXPECT_EQ(socket->Receive(&signal, sizeof(signal)), sizeof(signal));
+
+  // So far, this is an ordinary stream.
+  // Now |reader| expects data to be writted to the shared memory. The renderer
+  // says how much data was written.
+  switch (GetParam()) {
+    case kZero:
+      buffer->params.bitstream_data_size = 0;
+      break;
+    case kNoOverflow:
+      buffer->params.bitstream_data_size =
+          shmem->mapped_size() - sizeof(media::AudioOutputBufferParameters);
+      break;
+    case kOverflowByOne:
+      buffer->params.bitstream_data_size =
+          shmem->mapped_size() - sizeof(media::AudioOutputBufferParameters) + 1;
+      break;
+    case kOverflowByOneThousand:
+      buffer->params.bitstream_data_size =
+          shmem->mapped_size() - sizeof(media::AudioOutputBufferParameters) +
+          1000;
+      break;
+    case kOverflowByMax:
+      buffer->params.bitstream_data_size = std::numeric_limits<decltype(
+          buffer->params.bitstream_data_size)>::max();
+      break;
+  }
+
+  ++signal;
+  EXPECT_EQ(socket->Send(&signal, sizeof(signal)), sizeof(signal));
+
+  // The purpose of the test is to ensure this call doesn't result in undefined
+  // behavior, which should be verified by sanitizers.
+  reader->Read(output_bus.get());
+}
+
+INSTANTIATE_TEST_CASE_P(AudioSyncReaderTest,
+                        AudioSyncReaderBitstreamTest,
+                        ::testing::ValuesIn(overflow_test_case_values));
+
+}  // namespace content

content/test/BUILD.gn

@@ -1366,6 +1366,7 @@ test("content_unittests") {
     "../browser/renderer_host/media/audio_output_authorization_handler_unittest.cc",
     "../browser/renderer_host/media/audio_output_delegate_impl_unittest.cc",
     "../browser/renderer_host/media/audio_renderer_host_unittest.cc",
+    "../browser/renderer_host/media/audio_sync_reader_unittest.cc",
     "../browser/renderer_host/media/fake_video_capture_device_launcher.cc",
     "../browser/renderer_host/media/fake_video_capture_device_launcher.h",
     "../browser/renderer_host/media/fake_video_capture_provider.cc",

media/audio/audio_output_controller.cc

@@ -350,6 +350,10 @@ int AudioOutputController::OnMoreData(base::TimeDelta delay,
   }
 
   if (will_monitor_audio_levels()) {
+    // Note: this code path should never be hit when using bitstream streams.
+    // Scan doesn't expect compressed audio, so it may go out of bounds trying
+    // to read |frames| frames of PCM data.
+    CHECK(!params_.IsBitstreamFormat());
     power_monitor_.Scan(*dest, frames);
 
     const auto now = base::TimeTicks::Now();