Fix use after free in DeviceMediaToMojoAdapter

In Issue 807887, ClusterFuzz provided a very useful stack trace demonstrating a
use after free, which is likely the same as Issue 777608. Root cause was a
Mojo connection error getting invoked on a base::Unretained() pointer to a
deleted object.

I added a unit test case the reproduced the issue before the fix. There are two
possible fixes.
1. In ~DeviceMediaToMojoAdapter() call Stop() in order to reset the connection
error handler before it gets invoked.
2. Use base::WeakPtr.
I am opting for option 2. because seeing/proving that solution 1. is effective
is unreasonably complex and also requires more code.

Test: services_unittests --gtest_filter="DeviceMediaToMojoAdapterTest.*"
Bug: 807887, 777608
Change-Id: If42094796fbb095caccad7af9f72263b1d5f3ed6
Reviewed-on: https://chromium-review.googlesource.com/898256
Commit-Queue: Christian Fremerey <chfremer@chromium.org>
Reviewed-by: Emircan Uysaler <emircan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#534199}

services/video_capture/BUILD.gn

@@ -66,6 +66,7 @@ source_set("tests") {
   testonly = true
 
   sources = [
+    "device_media_to_mojo_adapter_unittest.cc",
     "test/device_factory_provider_connectortest.cc",
     "test/device_factory_provider_test.cc",
     "test/device_factory_provider_test.h",
@@ -76,6 +77,8 @@ source_set("tests") {
     "test/fake_device_test.cc",
     "test/fake_device_test.h",
     "test/fake_device_unittest.cc",
+    "test/mock_device.cc",
+    "test/mock_device.h",
     "test/mock_device_factory.cc",
     "test/mock_device_factory.h",
     "test/mock_device_test.cc",

services/video_capture/device_media_to_mojo_adapter.cc

@@ -23,7 +23,8 @@ DeviceMediaToMojoAdapter::DeviceMediaToMojoAdapter(
     : service_ref_(std::move(service_ref)),
       device_(std::move(device)),
       jpeg_decoder_factory_callback_(jpeg_decoder_factory_callback),
-      device_started_(false) {}
+      device_started_(false),
+      weak_factory_(this) {}
 
 DeviceMediaToMojoAdapter::~DeviceMediaToMojoAdapter() {
   DCHECK(thread_checker_.CalledOnValidThread());
@@ -37,17 +38,10 @@ void DeviceMediaToMojoAdapter::Start(
   DCHECK(thread_checker_.CalledOnValidThread());
   receiver.set_connection_error_handler(
       base::Bind(&DeviceMediaToMojoAdapter::OnClientConnectionErrorOrClose,
-                 base::Unretained(this)));
+                 weak_factory_.GetWeakPtr()));
 
   auto receiver_adapter =
       std::make_unique<ReceiverMojoToMediaAdapter>(std::move(receiver));
-  // We must hold on something that allows us to unsubscribe from
-  // receiver.set_connection_error_handler() when we stop the device. Otherwise,
-  // we may receive a corresponding callback after having been destroyed.
-  // This happens when the deletion of |receiver| is delayed (scheduled to a
-  // task runner) when we release |device_|, as is the case when using
-  // ReceiverOnTaskRunner.
-  receiver_adapter_ptr_ = receiver_adapter.get();
   auto media_receiver = std::make_unique<ReceiverOnTaskRunner>(
       std::move(receiver_adapter), base::ThreadTaskRunnerHandle::Get());
 
@@ -118,9 +112,7 @@ void DeviceMediaToMojoAdapter::Stop() {
   if (device_started_ == false)
     return;
   device_started_ = false;
-  // Unsubscribe from connection error callbacks.
-  receiver_adapter_ptr_->ResetConnectionErrorHandler();
-  receiver_adapter_ptr_ = nullptr;
+  weak_factory_.InvalidateWeakPtrs();
   device_->StopAndDeAllocate();
 }
 

services/video_capture/device_media_to_mojo_adapter.h

@@ -14,8 +14,6 @@
 
 namespace video_capture {
 
-class ReceiverMojoToMediaAdapter;
-
 // Implementation of mojom::Device backed by a given instance of
 // media::VideoCaptureDevice.
 class DeviceMediaToMojoAdapter : public mojom::Device {
@@ -52,8 +50,8 @@ class DeviceMediaToMojoAdapter : public mojom::Device {
   const std::unique_ptr<media::VideoCaptureDevice> device_;
   media::VideoCaptureJpegDecoderFactoryCB jpeg_decoder_factory_callback_;
   bool device_started_;
-  ReceiverMojoToMediaAdapter* receiver_adapter_ptr_ = nullptr;
   base::ThreadChecker thread_checker_;
+  base::WeakPtrFactory<DeviceMediaToMojoAdapter> weak_factory_;
 };
 
 }  // namespace video_capture

services/video_capture/device_media_to_mojo_adapter_unittest.cc

+// Copyright 2018 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 "services/video_capture/device_media_to_mojo_adapter.h"
+
+#include "base/run_loop.h"
+#include "base/test/scoped_task_environment.h"
+#include "services/video_capture/test/mock_device.h"
+#include "services/video_capture/test/mock_receiver.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using testing::Invoke;
+using testing::_;
+
+namespace video_capture {
+
+class DeviceMediaToMojoAdapterTest : public ::testing::Test {
+ public:
+  DeviceMediaToMojoAdapterTest() = default;
+  ~DeviceMediaToMojoAdapterTest() override = default;
+
+  void SetUp() override {
+    mock_receiver_ =
+        std::make_unique<MockReceiver>(mojo::MakeRequest(&receiver_));
+    auto mock_device = std::make_unique<MockDevice>();
+    mock_device_ptr_ = mock_device.get();
+    adapter_ = std::make_unique<DeviceMediaToMojoAdapter>(
+        std::unique_ptr<service_manager::ServiceContextRef>(),
+        std::move(mock_device), media::VideoCaptureJpegDecoderFactoryCB());
+  }
+
+  void TearDown() override {
+    // The internals of ReceiverOnTaskRunner perform a DeleteSoon().
+    adapter_.reset();
+    base::RunLoop wait_loop;
+    wait_loop.RunUntilIdle();
+  }
+
+ protected:
+  MockDevice* mock_device_ptr_;
+  std::unique_ptr<DeviceMediaToMojoAdapter> adapter_;
+  std::unique_ptr<MockReceiver> mock_receiver_;
+  mojom::ReceiverPtr receiver_;
+  base::test::ScopedTaskEnvironment task_environment_;
+};
+
+TEST_F(DeviceMediaToMojoAdapterTest,
+       DeviceIsStoppedWhenReceiverClosesConnection) {
+  {
+    base::RunLoop run_loop;
+    EXPECT_CALL(*mock_device_ptr_, DoAllocateAndStart(_, _))
+        .WillOnce(Invoke(
+            [](const media::VideoCaptureParams& params,
+               std::unique_ptr<media::VideoCaptureDevice::Client>* client) {
+              (*client)->OnStarted();
+            }));
+    EXPECT_CALL(*mock_receiver_, OnStarted()).WillOnce(Invoke([&run_loop]() {
+      run_loop.Quit();
+    }));
+
+    const media::VideoCaptureParams kArbitrarySettings;
+    adapter_->Start(kArbitrarySettings, std::move(receiver_));
+    run_loop.Run();
+  }
+  {
+    base::RunLoop run_loop;
+    EXPECT_CALL(*mock_device_ptr_, DoStopAndDeAllocate())
+        .WillOnce(Invoke([&run_loop]() { run_loop.Quit(); }));
+    mock_receiver_.reset();
+    run_loop.Run();
+  }
+}
+
+// Triggers a condition that caused a use-after-free reported in
+// https://crbug.com/807887. The use-after-free happened because the connection
+// lost event handler got invoked on a base::Unretained() pointer to |adapter_|
+// after |adapter_| was released.
+TEST_F(DeviceMediaToMojoAdapterTest,
+       ReleaseInstanceSynchronouslyAfterReceiverClosedConnection) {
+  {
+    base::RunLoop run_loop;
+    EXPECT_CALL(*mock_device_ptr_, DoAllocateAndStart(_, _))
+        .WillOnce(Invoke(
+            [](const media::VideoCaptureParams& params,
+               std::unique_ptr<media::VideoCaptureDevice::Client>* client) {
+              (*client)->OnStarted();
+            }));
+    EXPECT_CALL(*mock_receiver_, OnStarted()).WillOnce(Invoke([&run_loop]() {
+      run_loop.Quit();
+    }));
+
+    const media::VideoCaptureParams kArbitrarySettings;
+    adapter_->Start(kArbitrarySettings, std::move(receiver_));
+    run_loop.Run();
+  }
+
+  {
+    base::RunLoop run_loop;
+
+    // This posts invocation of the error event handler to the end of the
+    // current sequence.
+    mock_receiver_.reset();
+
+    // This destroys the DeviceMediaToMojoAdapter, which in turn posts a
+    // DeleteSoon in ~ReceiverOnTaskRunner() to the end of the current sequence.
+    adapter_.reset();
+
+    // Give error handle chance to get invoked
+    run_loop.RunUntilIdle();
+  }
+}
+
+}  // namespace video_capture

services/video_capture/receiver_mojo_to_media_adapter.cc

@@ -80,10 +80,6 @@ ReceiverMojoToMediaAdapter::ReceiverMojoToMediaAdapter(
 
 ReceiverMojoToMediaAdapter::~ReceiverMojoToMediaAdapter() = default;
 
-void ReceiverMojoToMediaAdapter::ResetConnectionErrorHandler() {
-  receiver_.set_connection_error_handler(base::Closure());
-}
-
 void ReceiverMojoToMediaAdapter::OnNewBufferHandle(
     int buffer_id,
     std::unique_ptr<media::VideoCaptureDevice::Client::Buffer::HandleProvider>

services/video_capture/receiver_mojo_to_media_adapter.h

@@ -47,8 +47,6 @@ class ReceiverMojoToMediaAdapter : public media::VideoFrameReceiver {
   ReceiverMojoToMediaAdapter(mojom::ReceiverPtr receiver);
   ~ReceiverMojoToMediaAdapter() override;
 
-  void ResetConnectionErrorHandler();
-
   // media::VideoFrameReceiver implementation.
   void OnNewBufferHandle(
       int buffer_id,

services/video_capture/test/mock_device.cc

+// Copyright 2018 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 "services/video_capture/test/mock_device.h"
+
+namespace video_capture {
+
+MockDevice::MockDevice() = default;
+
+MockDevice::~MockDevice() = default;
+
+void MockDevice::SendStubFrame(const media::VideoCaptureFormat& format,
+                               int rotation,
+                               int frame_feedback_id) {
+  auto stub_frame = media::VideoFrame::CreateZeroInitializedFrame(
+      format.pixel_format, format.frame_size,
+      gfx::Rect(format.frame_size.width(), format.frame_size.height()),
+      format.frame_size, base::TimeDelta());
+  client_->OnIncomingCapturedData(
+      stub_frame->data(0),
+      static_cast<int>(media::VideoFrame::AllocationSize(
+          stub_frame->format(), stub_frame->coded_size())),
+      format, rotation, base::TimeTicks(), base::TimeDelta(),
+      frame_feedback_id);
+}
+
+void MockDevice::AllocateAndStart(const media::VideoCaptureParams& params,
+                                  std::unique_ptr<Client> client) {
+  client_ = std::move(client);
+  DoAllocateAndStart(params, &client_);
+}
+
+void MockDevice::StopAndDeAllocate() {
+  DoStopAndDeAllocate();
+  client_.reset();
+}
+
+void MockDevice::GetPhotoState(GetPhotoStateCallback callback) {
+  DoGetPhotoState(&callback);
+}
+
+void MockDevice::SetPhotoOptions(media::mojom::PhotoSettingsPtr settings,
+                                 SetPhotoOptionsCallback callback) {
+  DoSetPhotoOptions(&settings, &callback);
+}
+
+void MockDevice::TakePhoto(TakePhotoCallback callback) {
+  DoTakePhoto(&callback);
+}
+
+}  // namespace video_capture

services/video_capture/test/mock_device.h

+// Copyright 2018 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.
+
+#ifndef SERVICES_VIDEO_CAPTURE_TEST_MOCK_DEVICE_H_
+#define SERVICES_VIDEO_CAPTURE_TEST_MOCK_DEVICE_H_
+
+#include "media/capture/video/video_capture_device.h"
+#include "testing/gmock/include/gmock/gmock.h"
+
+namespace video_capture {
+
+// To ensure correct operation, this mock device holds on to the |client|
+// that is passed to it in AllocateAndStart() and releases it on
+// StopAndDeAllocate().
+class MockDevice : public media::VideoCaptureDevice {
+ public:
+  MockDevice();
+  ~MockDevice() override;
+
+  void SendStubFrame(const media::VideoCaptureFormat& format,
+                     int rotation,
+                     int frame_feedback_id);
+
+  // media::VideoCaptureDevice implementation.
+  MOCK_METHOD2(DoAllocateAndStart,
+               void(const media::VideoCaptureParams& params,
+                    std::unique_ptr<Client>* client));
+  MOCK_METHOD0(RequestRefreshFrame, void());
+  MOCK_METHOD0(DoStopAndDeAllocate, void());
+  MOCK_METHOD1(DoGetPhotoState, void(GetPhotoStateCallback* callback));
+  MOCK_METHOD2(DoSetPhotoOptions,
+               void(media::mojom::PhotoSettingsPtr* settings,
+                    SetPhotoOptionsCallback* callback));
+  MOCK_METHOD1(DoTakePhoto, void(TakePhotoCallback* callback));
+  MOCK_METHOD2(OnUtilizationReport,
+               void(int frame_feedback_id, double utilization));
+
+  void AllocateAndStart(const media::VideoCaptureParams& params,
+                        std::unique_ptr<Client> client) override;
+  void StopAndDeAllocate() override;
+  void GetPhotoState(GetPhotoStateCallback callback) override;
+  void SetPhotoOptions(media::mojom::PhotoSettingsPtr settings,
+                       SetPhotoOptionsCallback callback) override;
+  void TakePhoto(TakePhotoCallback callback) override;
+
+  std::unique_ptr<Client> TakeOutClient() { return std::move(client_); }
+
+ private:
+  std::unique_ptr<Client> client_;
+};
+
+}  // namespace video_capture
+
+#endif  // SERVICES_VIDEO_CAPTURE_TEST_MOCK_DEVICE_H_

services/video_capture/test/mock_device_test.cc

@@ -24,49 +24,6 @@ std::unique_ptr<media::VideoCaptureJpegDecoder> CreateJpegDecoder() {
 
 namespace video_capture {
 
-MockDevice::MockDevice() = default;
-
-MockDevice::~MockDevice() = default;
-
-void MockDevice::SendStubFrame(const media::VideoCaptureFormat& format,
-                               int rotation,
-                               int frame_feedback_id) {
-  auto stub_frame = media::VideoFrame::CreateZeroInitializedFrame(
-      format.pixel_format, format.frame_size,
-      gfx::Rect(format.frame_size.width(), format.frame_size.height()),
-      format.frame_size, base::TimeDelta());
-  client_->OnIncomingCapturedData(
-      stub_frame->data(0),
-      static_cast<int>(media::VideoFrame::AllocationSize(
-          stub_frame->format(), stub_frame->coded_size())),
-      format, rotation, base::TimeTicks(), base::TimeDelta(),
-      frame_feedback_id);
-}
-
-void MockDevice::AllocateAndStart(const media::VideoCaptureParams& params,
-                                  std::unique_ptr<Client> client) {
-  client_ = std::move(client);
-  DoAllocateAndStart(params, &client);
-}
-
-void MockDevice::StopAndDeAllocate() {
-  DoStopAndDeAllocate();
-  client_.reset();
-}
-
-void MockDevice::GetPhotoState(GetPhotoStateCallback callback) {
-  DoGetPhotoState(&callback);
-}
-
-void MockDevice::SetPhotoOptions(media::mojom::PhotoSettingsPtr settings,
-                                 SetPhotoOptionsCallback callback) {
-  DoSetPhotoOptions(&settings, &callback);
-}
-
-void MockDevice::TakePhoto(TakePhotoCallback callback) {
-  DoTakePhoto(&callback);
-}
-
 MockDeviceTest::MockDeviceTest() : ref_factory_(base::Bind(&base::DoNothing)) {}
 
 MockDeviceTest::~MockDeviceTest() = default;

services/video_capture/test/mock_device_test.h

@@ -10,6 +10,7 @@
 #include "services/service_manager/public/cpp/service_context_ref.h"
 #include "services/video_capture/device_factory_media_to_mojo_adapter.h"
 #include "services/video_capture/public/interfaces/device_factory_provider.mojom.h"
+#include "services/video_capture/test/mock_device.h"
 #include "services/video_capture/test/mock_device_factory.h"
 #include "services/video_capture/test/mock_receiver.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -20,44 +21,6 @@ class MessageLoop;
 
 namespace video_capture {
 
-// To ensure correct operation, this mock device holds on to the |client|
-// that is passed to it in AllocateAndStart() and releases it on
-// StopAndDeAllocate().
-class MockDevice : public media::VideoCaptureDevice {
- public:
-  MockDevice();
-  ~MockDevice() override;
-
-  void SendStubFrame(const media::VideoCaptureFormat& format,
-                     int rotation,
-                     int frame_feedback_id);
-
-  // media::VideoCaptureDevice implementation.
-  MOCK_METHOD2(DoAllocateAndStart,
-               void(const media::VideoCaptureParams& params,
-                    std::unique_ptr<Client>* client));
-  MOCK_METHOD0(RequestRefreshFrame, void());
-  MOCK_METHOD0(DoStopAndDeAllocate, void());
-  MOCK_METHOD1(DoGetPhotoState, void(GetPhotoStateCallback* callback));
-  MOCK_METHOD2(DoSetPhotoOptions,
-               void(media::mojom::PhotoSettingsPtr* settings,
-                    SetPhotoOptionsCallback* callback));
-  MOCK_METHOD1(DoTakePhoto, void(TakePhotoCallback* callback));
-  MOCK_METHOD2(OnUtilizationReport,
-               void(int frame_feedback_id, double utilization));
-
-  void AllocateAndStart(const media::VideoCaptureParams& params,
-                        std::unique_ptr<Client> client) override;
-  void StopAndDeAllocate() override;
-  void GetPhotoState(GetPhotoStateCallback callback) override;
-  void SetPhotoOptions(media::mojom::PhotoSettingsPtr settings,
-                       SetPhotoOptionsCallback callback) override;
-  void TakePhoto(TakePhotoCallback callback) override;
-
- private:
-  std::unique_ptr<Client> client_;
-};
-
 // Reusable test setup for testing with a single mock device.
 class MockDeviceTest : public ::testing::Test {
  public: