From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Guido Urdaneta <guidou@chromium.org>
Date: Thu, 24 Aug 2023 11:12:43 +0000
Subject: Handle object destruction in MediaStreamDeviceObserver

MSDO executes some callbacks that can result in the destruction of
MSDO upon an external event such as removing a media device or the
user revoking permission.
This CL adds code to detect this condition and prevent further
processing that would result in UAF. It also removes some invalid
DCHECKs.

Drive-by: minor style fixes

(cherry picked from commit 7337133682ab0404b753c563dde2ae2b1dc13171)

Bug: 1472492, b/296997707
Change-Id: I76f019bb110e7d9cca276444bc23a7e43114d2cc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4798398
Reviewed-by: Palak Agarwal <agpalak@chromium.org>
Commit-Queue: Guido Urdaneta <guidou@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1186452}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4810035
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/branch-heads/5845@{#1586}
Cr-Branched-From: 5a5dff63a4a4c63b9b18589819bebb2566c85443-refs/heads/main@{#1160321}

diff --git a/third_party/blink/renderer/modules/mediastream/media_stream_device_observer.cc b/third_party/blink/renderer/modules/mediastream/media_stream_device_observer.cc
index 0715b727f7e2b319243f5ec47ead48c8b1b1f644..7bf375055637d67fa409ce977e103baf4aecdf6b 100644
--- a/third_party/blink/renderer/modules/mediastream/media_stream_device_observer.cc
+++ b/third_party/blink/renderer/modules/mediastream/media_stream_device_observer.cc
@@ -37,11 +37,11 @@ MediaStreamDeviceObserver::MediaStreamDeviceObserver(LocalFrame* frame) {
   if (frame) {
     frame->GetInterfaceRegistry()->AddInterface(WTF::BindRepeating(
         &MediaStreamDeviceObserver::BindMediaStreamDeviceObserverReceiver,
-        WTF::Unretained(this)));
+        weak_factory_.GetWeakPtr()));
   }
 }
 
-MediaStreamDeviceObserver::~MediaStreamDeviceObserver() {}
+MediaStreamDeviceObserver::~MediaStreamDeviceObserver() = default;
 
 MediaStreamDevices MediaStreamDeviceObserver::GetNonScreenCaptureDevices() {
   MediaStreamDevices video_devices;
@@ -70,13 +70,21 @@ void MediaStreamDeviceObserver::OnDeviceStopped(
   }
 
   for (Stream& stream : it->value) {
-    if (IsAudioInputMediaType(device.type))
+    if (IsAudioInputMediaType(device.type)) {
       RemoveStreamDeviceFromArray(device, &stream.audio_devices);
-    else
+    } else {
       RemoveStreamDeviceFromArray(device, &stream.video_devices);
-
-    if (stream.on_device_stopped_cb)
+    }
+    if (stream.on_device_stopped_cb) {
+      // Running `stream.on_device_stopped_cb` can destroy `this`. Use a weak
+      // pointer to detect that condition, and stop processing if it happens.
+      base::WeakPtr<MediaStreamDeviceObserver> weak_this =
+          weak_factory_.GetWeakPtr();
       stream.on_device_stopped_cb.Run(device);
+      if (!weak_this) {
+        return;
+      }
+    }
   }
 
   // |it| could have already been invalidated in the function call above. So we
@@ -85,8 +93,9 @@ void MediaStreamDeviceObserver::OnDeviceStopped(
   // iterator from |label_stream_map_| (https://crbug.com/616884). Future work
   // needs to be done to resolve this re-entrancy issue.
   it = label_stream_map_.find(label);
-  if (it == label_stream_map_.end())
+  if (it == label_stream_map_.end()) {
     return;
+  }
 
   Vector<Stream>& streams = it->value;
   auto* stream_it = streams.begin();
@@ -122,8 +131,16 @@ void MediaStreamDeviceObserver::OnDeviceChanged(
   DCHECK_EQ(1u, it->value.size());
 
   Stream* stream = &it->value[0];
-  if (stream->on_device_changed_cb)
+  if (stream->on_device_changed_cb) {
+    // Running `stream->on_device_changed_cb` can destroy `this`. Use a weak
+    // pointer to detect that condition, and stop processing if it happens.
+    base::WeakPtr<MediaStreamDeviceObserver> weak_this =
+        weak_factory_.GetWeakPtr();
     stream->on_device_changed_cb.Run(old_device, new_device);
+    if (!weak_this) {
+      return;
+    }
+  }
 
   // Update device list only for device changing. Removing device will be
   // handled in its own callback.
@@ -304,9 +321,9 @@ void MediaStreamDeviceObserver::RemoveStreamDevice(
       streams_to_remove.push_back(entry.key);
     }
   }
-  DCHECK(device_found);
-  for (const String& label : streams_to_remove)
+  for (const String& label : streams_to_remove) {
     label_stream_map_.erase(label);
+  }
 }
 
 base::UnguessableToken MediaStreamDeviceObserver::GetAudioSessionId(
diff --git a/third_party/blink/renderer/modules/mediastream/media_stream_device_observer.h b/third_party/blink/renderer/modules/mediastream/media_stream_device_observer.h
index e14c74b7c4ba270efeb433eb0e5d876f6224e60b..a97e39274caa75a321d08afc8703bd9b2c29351d 100644
--- a/third_party/blink/renderer/modules/mediastream/media_stream_device_observer.h
+++ b/third_party/blink/renderer/modules/mediastream/media_stream_device_observer.h
@@ -123,6 +123,7 @@ class MODULES_EXPORT MediaStreamDeviceObserver
 
   using LabelStreamMap = HashMap<String, Vector<Stream>>;
   LabelStreamMap label_stream_map_;
+  base::WeakPtrFactory<MediaStreamDeviceObserver> weak_factory_{this};
 };
 
 }  // namespace blink
diff --git a/third_party/blink/renderer/platform/exported/mediastream/web_platform_media_stream_source.cc b/third_party/blink/renderer/platform/exported/mediastream/web_platform_media_stream_source.cc
index 2cc3a9a7f072dec1aaf44b3e13d4ef6dacf3e943..e3eddc9d5ed12bf9947af12c1054f217c90d40b8 100644
--- a/third_party/blink/renderer/platform/exported/mediastream/web_platform_media_stream_source.cc
+++ b/third_party/blink/renderer/platform/exported/mediastream/web_platform_media_stream_source.cc
@@ -32,10 +32,12 @@ void WebPlatformMediaStreamSource::StopSource() {
 
 void WebPlatformMediaStreamSource::FinalizeStopSource() {
   DCHECK(task_runner_->BelongsToCurrentThread());
-  if (!stop_callback_.is_null())
+  if (!stop_callback_.is_null()) {
     std::move(stop_callback_).Run(Owner());
-  if (Owner())
+  }
+  if (Owner()) {
     Owner().SetReadyState(WebMediaStreamSource::kReadyStateEnded);
+  }
 }
 
 void WebPlatformMediaStreamSource::SetSourceMuted(bool is_muted) {
@@ -43,8 +45,9 @@ void WebPlatformMediaStreamSource::SetSourceMuted(bool is_muted) {
   // Although this change is valid only if the ready state isn't already Ended,
   // there's code further along (like in MediaStreamTrack) which filters
   // that out already.
-  if (!Owner())
+  if (!Owner()) {
     return;
+  }
   Owner().SetReadyState(is_muted ? WebMediaStreamSource::kReadyStateMuted
                                  : WebMediaStreamSource::kReadyStateLive);
 }
@@ -73,7 +76,6 @@ void WebPlatformMediaStreamSource::SetStopCallback(
 
 void WebPlatformMediaStreamSource::ResetSourceStoppedCallback() {
   DCHECK(task_runner_->BelongsToCurrentThread());
-  DCHECK(!stop_callback_.is_null());
   stop_callback_.Reset();
 }
 
