Skip to content

Commit

Permalink
chore: cherry-pick d727013bb543 from chromium (#30816)
Browse files Browse the repository at this point in the history
* chore: cherry-pick d727013bb543 from chromium

* chore: update patches

Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>
Co-authored-by: Electron Bot <electron@github.com>
  • Loading branch information
3 people committed Sep 6, 2021
1 parent da5a627 commit 87b55ce
Show file tree
Hide file tree
Showing 2 changed files with 86 additions and 0 deletions.
1 change: 1 addition & 0 deletions patches/chromium/.patches
Original file line number Diff line number Diff line change
Expand Up @@ -147,4 +147,5 @@ cherry-pick-ac9dc1235e28.patch
cherry-pick-4ce2abc17078.patch
cherry-pick-e2123a8e0943.patch
cherry-pick-1227933.patch
cherry-pick-d727013bb543.patch
cherry-pick-1231134.patch
85 changes: 85 additions & 0 deletions patches/chromium/cherry-pick-d727013bb543.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Henrik=20Bostr=C3=B6m?= <hbos@chromium.org>
Date: Thu, 8 Jul 2021 12:16:10 +0000
Subject: Fix UAF in VideoCaptureDeviceAVFoundation's dealloc.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Despite dealloc performing stopCapture prior to clearing variables like
_sampleBufferTransformer, it appears possible for callbacks that are
already running concurrently to be using these variables, resulting in
rare use-after-free races. By grabbing the _lock, we avoid this issue.

We also have to introduce a new lock, _destructionLock, to ensure |this|
is not destroyed while -captureOutput is still running.

Bug: chromium:1227228
Change-Id: I8c2c4d9834ee995d3f4154fae13e262398e6f2e2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3013796
Reviewed-by: Evan Shrubsole <eshr@google.com>
Reviewed-by: Ilya Nikolaevskiy <ilnik@chromium.org>
Commit-Queue: Henrik Boström <hbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#899503}

diff --git a/media/capture/video/mac/video_capture_device_avfoundation_mac.h b/media/capture/video/mac/video_capture_device_avfoundation_mac.h
index 4ecd25491967d001f71e01f88ca415c5bbe788c7..134423d6bee933ab4bebcd7bb99b874b93eb847c 100644
--- a/media/capture/video/mac/video_capture_device_avfoundation_mac.h
+++ b/media/capture/video/mac/video_capture_device_avfoundation_mac.h
@@ -52,6 +52,8 @@ CAPTURE_EXPORT
// Protects concurrent setting and using |frameReceiver_|. Note that the
// GUARDED_BY decoration below does not have any effect.
base::Lock _lock;
+ // Used to avoid UAF in -captureOutput.
+ base::Lock _destructionLock;
media::VideoCaptureDeviceAVFoundationFrameReceiver* _frameReceiver
GUARDED_BY(_lock); // weak.

diff --git a/media/capture/video/mac/video_capture_device_avfoundation_mac.mm b/media/capture/video/mac/video_capture_device_avfoundation_mac.mm
index 7367fbdb4b9216f012c263007170fbc70b1ad823..d1a7997a08ec9ce87278822c8f4c6156db489e7c 100644
--- a/media/capture/video/mac/video_capture_device_avfoundation_mac.mm
+++ b/media/capture/video/mac/video_capture_device_avfoundation_mac.mm
@@ -180,12 +180,26 @@ - (id)initWithFrameReceiver:
}

- (void)dealloc {
- [self stopStillImageOutput];
- [self stopCapture];
- _sampleBufferTransformer.reset();
- _weakPtrFactoryForTakePhoto = nullptr;
- _mainThreadTaskRunner = nullptr;
- _sampleQueue.reset();
+ {
+ // To avoid races with concurrent callbacks, grab the lock before stopping
+ // capture and clearing all the variables.
+ base::AutoLock lock(_lock);
+ [self stopStillImageOutput];
+ [self stopCapture];
+ _frameReceiver = nullptr;
+ _sampleBufferTransformer.reset();
+ _weakPtrFactoryForTakePhoto = nullptr;
+ _mainThreadTaskRunner = nullptr;
+ _sampleQueue.reset();
+ }
+ {
+ // Ensures -captureOutput has finished before we continue the destruction
+ // steps. If -captureOutput grabbed the destruction lock before us this
+ // prevents UAF. If -captureOutput grabbed the destruction lock after us
+ // it will exit early because |_frameReceiver| is already null at this
+ // point.
+ base::AutoLock destructionLock(_destructionLock);
+ }
[super dealloc];
}

@@ -709,7 +723,9 @@ - (void)captureOutput:(AVCaptureOutput*)captureOutput
VLOG(3) << __func__;

// Concurrent calls into |_frameReceiver| are not supported, so take |_lock|
- // before any of the subsequent paths.
+ // before any of the subsequent paths. The |_destructionLock| must be grabbed
+ // first to avoid races with -dealloc.
+ base::AutoLock destructionLock(_destructionLock);
base::AutoLock lock(_lock);
if (!_frameReceiver)
return;

0 comments on commit 87b55ce

Please sign in to comment.