From 41b696164b7398f99ccddb39997a8e24d20fdeba Mon Sep 17 00:00:00 2001 From: Zakhar Voit Date: Mon, 21 Nov 2022 04:11:38 +0000 Subject: [Backport] CVE-2022-4179: Use after free in Audio Manual cherry-pick of patch originally reviewed on https://chromium-review.googlesource.com/c/chromium/src/+/4024547: Replace raw pointer to LocalMuter with weak ptr This CL replaces a raw pointer to LocalMuter with a weak ptr. Additional info about this bug here: http://crbug/1377783 (cherry picked from commit 9989b93eb12c93b9351d5bf2872c1069ef5f7d01) Bug: 1377783 Change-Id: Id821ea800ba12f1cfae4677fc591c12dec112852 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3997421 Commit-Queue: Evan Liu Cr-Original-Commit-Position: refs/heads/main@{#1068776} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4024547 Auto-Submit: Evan Liu Owners-Override: Srinivas Sista Bot-Commit: Rubber Stamper Commit-Queue: Rubber Stamper Reviewed-by: Evan Liu Cr-Commit-Position: refs/branch-heads/5359@{#824} Cr-Branched-From: 27d3765d341b09369006d030f83f582a29eb57ae-refs/heads/main@{#1058933} (cherry picked from commit 65d46507a0c9e88b407060d0b8b7d9f0897d09e2) Reviewed-on: https://codereview.qt-project.org/c/qt/qtwebengine-chromium/+/446484 Reviewed-by: Michal Klocek Reviewed-on: https://codereview.qt-project.org/c/qt/qtwebengine-chromium/+/450108 --- chromium/services/audio/local_muter.h | 5 +++++ chromium/services/audio/stream_factory.cc | 18 +++++++++--------- chromium/services/audio/stream_factory.h | 2 +- 3 files changed, 15 insertions(+), 10 deletions(-) diff --git a/chromium/services/audio/local_muter.h b/chromium/services/audio/local_muter.h index 00316b005f5..a67a7ba8d90 100644 --- a/chromium/services/audio/local_muter.h +++ b/chromium/services/audio/local_muter.h @@ -7,6 +7,7 @@ #include "base/callback.h" #include "base/macros.h" +#include "base/memory/weak_ptr.h" #include "base/sequence_checker.h" #include "base/unguessable_token.h" #include "mojo/public/cpp/bindings/associated_receiver_set.h" @@ -40,6 +41,8 @@ class LocalMuter : public mojom::LocalMuter, void OnMemberJoinedGroup(LoopbackGroupMember* member) final; void OnMemberLeftGroup(LoopbackGroupMember* member) final; + base::WeakPtr GetWeakPtr() { return weak_factory_.GetWeakPtr(); } + private: // Runs the |all_bindings_lost_callback_| when |bindings_| becomes empty. void OnBindingLost(); @@ -52,6 +55,8 @@ class LocalMuter : public mojom::LocalMuter, SEQUENCE_CHECKER(sequence_checker_); + base::WeakPtrFactory weak_factory_{this}; + DISALLOW_COPY_AND_ASSIGN(LocalMuter); }; diff --git a/chromium/services/audio/stream_factory.cc b/chromium/services/audio/stream_factory.cc index ff64baf61b7..8e6d662a79a 100644 --- a/chromium/services/audio/stream_factory.cc +++ b/chromium/services/audio/stream_factory.cc @@ -127,8 +127,9 @@ void StreamFactory::BindMuter( if (it == muters_.end()) { auto muter_ptr = std::make_unique(&coordinator_, group_id); muter = muter_ptr.get(); - muter->SetAllBindingsLostCallback(base::BindOnce( - &StreamFactory::DestroyMuter, base::Unretained(this), muter)); + muter->SetAllBindingsLostCallback( + base::BindOnce(&StreamFactory::DestroyMuter, + base::Unretained(this), muter_ptr->GetWeakPtr())); muters_.emplace_back(std::move(muter_ptr)); } else { muter = it->get(); @@ -201,9 +202,10 @@ void StreamFactory::DestroyOutputStream(OutputStream* stream) { DCHECK_EQ(1u, erased); } -void StreamFactory::DestroyMuter(LocalMuter* muter) { +void StreamFactory::DestroyMuter(base::WeakPtr muter) { DCHECK_CALLED_ON_VALID_SEQUENCE(owning_sequence_); - DCHECK(muter); + if (!muter) + return; // Output streams have a task posting before destruction (see the OnError // function in output_stream.cc). To ensure that stream destruction and @@ -212,13 +214,11 @@ void StreamFactory::DestroyMuter(LocalMuter* muter) { // Otherwise, a "destroy all streams, then destroy the muter" sequence may // result in a brief blip of audio. auto do_destroy = [](base::WeakPtr weak_this, - LocalMuter* muter) { - if (weak_this) { - + base::WeakPtr muter) { + if (weak_this && muter) { const auto it = std::find_if(weak_this->muters_.begin(), weak_this->muters_.end(), - base::MatchesUniquePtr(muter)); - DCHECK(it != weak_this->muters_.end()); + base::MatchesUniquePtr(muter.get())); weak_this->muters_.erase(it); } }; diff --git a/chromium/services/audio/stream_factory.h b/chromium/services/audio/stream_factory.h index 747da1e447a..0020a159127 100644 --- a/chromium/services/audio/stream_factory.h +++ b/chromium/services/audio/stream_factory.h @@ -94,7 +94,7 @@ class StreamFactory final : public mojom::StreamFactory { void DestroyInputStream(InputStream* stream); void DestroyOutputStream(OutputStream* stream); - void DestroyMuter(LocalMuter* muter); + void DestroyMuter(base::WeakPtr muter); void DestroyLoopbackStream(LoopbackStream* stream); SEQUENCE_CHECKER(owning_sequence_); -- cgit v1.2.1