-
Notifications
You must be signed in to change notification settings - Fork 15k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
chore: cherry-pick 3 changes from Release-1-M121 (#41177)
* chore: [26-x-y] cherry-pick 4 changes from Release-1-M121 * ee0b8769f428 from chromium * 1f8bec968902 from chromium * 4a98f9e304be from chromium * chore: update patches --------- Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>
- Loading branch information
1 parent
0ebe403
commit 1390874
Showing
4 changed files
with
267 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,125 @@ | ||
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 | ||
From: Tsuyoshi Horo <horo@chromium.org> | ||
Date: Wed, 24 Jan 2024 02:04:24 +0000 | ||
Subject: Fix UAF in SourceStreamToDataPipe | ||
|
||
SourceStreamToDataPipe::ReadMore() is passing a callback with | ||
Unretained(this) to net::SourceStream::Read(). But this callback may be | ||
called even after the SourceStream is destructed. This is causing UAF | ||
issue (crbug.com/1511085). | ||
|
||
To solve this problem, this CL changes ReadMore() method to pass a | ||
callback with a weak ptr of this. | ||
|
||
(cherry picked from commit 6e36a69da1b73f9aea9c54bfbe6c5b9cb2c672a5) | ||
|
||
Bug: 1511085 | ||
Change-Id: Idd4e34ff300ff5db2de1de7b303841c7db3a964a | ||
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5179746 | ||
Reviewed-by: Adam Rice <ricea@chromium.org> | ||
Commit-Queue: Tsuyoshi Horo <horo@chromium.org> | ||
Cr-Original-Commit-Position: refs/heads/main@{#1244526} | ||
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5231558 | ||
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org> | ||
Cr-Commit-Position: refs/branch-heads/6099@{#1860} | ||
Cr-Branched-From: e6ee4500f7d6549a9ac1354f8d056da49ef406be-refs/heads/main@{#1217362} | ||
|
||
diff --git a/services/network/public/cpp/source_stream_to_data_pipe.cc b/services/network/public/cpp/source_stream_to_data_pipe.cc | ||
index bfd85b1a00b216b52ae816ca29cb66ddabe20b6d..07afd58a40f92485ded07c535092a891c5140c7b 100644 | ||
--- a/services/network/public/cpp/source_stream_to_data_pipe.cc | ||
+++ b/services/network/public/cpp/source_stream_to_data_pipe.cc | ||
@@ -55,9 +55,9 @@ void SourceStreamToDataPipe::ReadMore() { | ||
|
||
scoped_refptr<net::IOBuffer> buffer( | ||
new network::NetToMojoIOBuffer(pending_write_.get())); | ||
- int result = source_->Read( | ||
- buffer.get(), base::checked_cast<int>(num_bytes), | ||
- base::BindOnce(&SourceStreamToDataPipe::DidRead, base::Unretained(this))); | ||
+ int result = source_->Read(buffer.get(), base::checked_cast<int>(num_bytes), | ||
+ base::BindOnce(&SourceStreamToDataPipe::DidRead, | ||
+ weak_factory_.GetWeakPtr())); | ||
|
||
if (result != net::ERR_IO_PENDING) | ||
DidRead(result); | ||
diff --git a/services/network/public/cpp/source_stream_to_data_pipe_unittest.cc b/services/network/public/cpp/source_stream_to_data_pipe_unittest.cc | ||
index 7061418c5141d936f04b1193c98e66efc5e72ac5..54159df39afa7cf6e2faa51da185dc034b923209 100644 | ||
--- a/services/network/public/cpp/source_stream_to_data_pipe_unittest.cc | ||
+++ b/services/network/public/cpp/source_stream_to_data_pipe_unittest.cc | ||
@@ -6,7 +6,9 @@ | ||
|
||
#include "base/functional/bind.h" | ||
#include "base/memory/raw_ptr.h" | ||
+#include "base/test/bind.h" | ||
#include "base/test/task_environment.h" | ||
+#include "net/base/net_errors.h" | ||
#include "net/filter/mock_source_stream.h" | ||
#include "testing/gtest/include/gtest/gtest.h" | ||
#include "third_party/abseil-cpp/absl/types/optional.h" | ||
@@ -42,6 +44,33 @@ struct SourceStreamToDataPipeTestParam { | ||
const ReadResultType read_result_type; | ||
}; | ||
|
||
+class DummyPendingSourceStream : public net::SourceStream { | ||
+ public: | ||
+ DummyPendingSourceStream() : net::SourceStream(SourceStream::TYPE_NONE) {} | ||
+ ~DummyPendingSourceStream() override = default; | ||
+ | ||
+ DummyPendingSourceStream(const DummyPendingSourceStream&) = delete; | ||
+ DummyPendingSourceStream& operator=(const DummyPendingSourceStream&) = delete; | ||
+ | ||
+ // SourceStream implementation | ||
+ int Read(net::IOBuffer* dest_buffer, | ||
+ int buffer_size, | ||
+ net::CompletionOnceCallback callback) override { | ||
+ callback_ = std::move(callback); | ||
+ return net::ERR_IO_PENDING; | ||
+ } | ||
+ std::string Description() const override { return ""; } | ||
+ bool MayHaveMoreBytes() const override { return true; } | ||
+ | ||
+ net::CompletionOnceCallback TakeCompletionCallback() { | ||
+ CHECK(callback_); | ||
+ return std::move(callback_); | ||
+ } | ||
+ | ||
+ private: | ||
+ net::CompletionOnceCallback callback_; | ||
+}; | ||
+ | ||
} // namespace | ||
|
||
class SourceStreamToDataPipeTest | ||
@@ -212,4 +241,33 @@ TEST_P(SourceStreamToDataPipeTest, MayHaveMoreBytes) { | ||
EXPECT_EQ(ReadPipe(&output), net::OK); | ||
EXPECT_EQ(output, message); | ||
} | ||
+ | ||
+TEST(SourceStreamToDataPipeCallbackTest, CompletionCallbackAfterDestructed) { | ||
+ base::test::TaskEnvironment task_environment; | ||
+ | ||
+ std::unique_ptr<DummyPendingSourceStream> source = | ||
+ std::make_unique<DummyPendingSourceStream>(); | ||
+ DummyPendingSourceStream* source_ptr = source.get(); | ||
+ const MojoCreateDataPipeOptions data_pipe_options{ | ||
+ sizeof(MojoCreateDataPipeOptions), MOJO_CREATE_DATA_PIPE_FLAG_NONE, 1, 1}; | ||
+ mojo::ScopedDataPipeProducerHandle producer_end; | ||
+ mojo::ScopedDataPipeConsumerHandle consumer_end; | ||
+ CHECK_EQ(MOJO_RESULT_OK, mojo::CreateDataPipe(&data_pipe_options, | ||
+ producer_end, consumer_end)); | ||
+ | ||
+ std::unique_ptr<SourceStreamToDataPipe> adapter = | ||
+ std::make_unique<SourceStreamToDataPipe>(std::move(source), | ||
+ std::move(producer_end)); | ||
+ bool callback_called = false; | ||
+ adapter->Start( | ||
+ base::BindLambdaForTesting([&](int result) { callback_called = true; })); | ||
+ net::CompletionOnceCallback callback = source_ptr->TakeCompletionCallback(); | ||
+ adapter.reset(); | ||
+ | ||
+ // Test that calling `callback` after deleting `adapter` must not cause UAF | ||
+ // (crbug.com/1511085). | ||
+ std::move(callback).Run(net::ERR_FAILED); | ||
+ EXPECT_FALSE(callback_called); | ||
+} | ||
+ | ||
} // namespace network |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,67 @@ | ||
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 | ||
From: =?UTF-8?q?Peter=20Bostr=C3=B6m?= <pbos@chromium.org> | ||
Date: Fri, 26 Jan 2024 19:37:57 +0000 | ||
Subject: Speculatively fix race in mojo ShutDownOnIOThread | ||
MIME-Version: 1.0 | ||
Content-Type: text/plain; charset=UTF-8 | ||
Content-Transfer-Encoding: 8bit | ||
|
||
This acquires `write_lock_` before resetting handles used by WriteNoLock | ||
(which is called under the same lock in another thread). We also set | ||
`reject_writes_` to prevent future write attempts after shutdown. That | ||
seems strictly more correct. | ||
|
||
We also acquire `fds_to_close_lock_` before clearing the FDs. | ||
|
||
I was unable to repro locally as content_browsertests just times out | ||
in my local setup without reporting anything interesting. This seems | ||
strictly more correct though. | ||
|
||
(cherry picked from commit 9755d9d81e4a8cb5b4f76b23b761457479dbb06b) | ||
|
||
Bug: 1519980 | ||
Change-Id: I96279936ca908ecb98eddd381df20d61597cba43 | ||
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5226127 | ||
Auto-Submit: Peter Boström <pbos@chromium.org> | ||
Reviewed-by: Ken Rockot <rockot@google.com> | ||
Commit-Queue: Ken Rockot <rockot@google.com> | ||
Commit-Queue: Peter Boström <pbos@chromium.org> | ||
Cr-Original-Commit-Position: refs/heads/main@{#1250580} | ||
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5239564 | ||
Cr-Commit-Position: refs/branch-heads/6099@{#1883} | ||
Cr-Branched-From: e6ee4500f7d6549a9ac1354f8d056da49ef406be-refs/heads/main@{#1217362} | ||
|
||
diff --git a/mojo/core/channel_posix.cc b/mojo/core/channel_posix.cc | ||
index 0a3596382d0e9a40c72bfb4ead6f0338a61253d6..eae6b0768463679b5043514dc5745da52b80ae10 100644 | ||
--- a/mojo/core/channel_posix.cc | ||
+++ b/mojo/core/channel_posix.cc | ||
@@ -246,16 +246,21 @@ void ChannelPosix::WaitForWriteOnIOThreadNoLock() { | ||
void ChannelPosix::ShutDownOnIOThread() { | ||
base::CurrentThread::Get()->RemoveDestructionObserver(this); | ||
|
||
- read_watcher_.reset(); | ||
- write_watcher_.reset(); | ||
- if (leak_handle_) { | ||
- std::ignore = socket_.release(); | ||
- } else { | ||
- socket_.reset(); | ||
- } | ||
+ { | ||
+ base::AutoLock lock(write_lock_); | ||
+ reject_writes_ = true; | ||
+ read_watcher_.reset(); | ||
+ write_watcher_.reset(); | ||
+ if (leak_handle_) { | ||
+ std::ignore = socket_.release(); | ||
+ } else { | ||
+ socket_.reset(); | ||
+ } | ||
#if BUILDFLAG(IS_IOS) | ||
- fds_to_close_.clear(); | ||
+ base::AutoLock fd_lock(fds_to_close_lock_); | ||
+ fds_to_close_.clear(); | ||
#endif | ||
+ } | ||
|
||
// May destroy the |this| if it was the last reference. | ||
self_ = nullptr; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,72 @@ | ||
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 | ||
From: Guido Urdaneta <guidou@chromium.org> | ||
Date: Wed, 24 Jan 2024 18:40:01 +0000 | ||
Subject: Exit early from RTCPeerConnectionHandler | ||
|
||
For certain operations that require a live client | ||
(i.e., RTCPeerConnection, which is garbage collected), | ||
PeerConnectionHandler keeps a pointer to the client on the stack | ||
to prevent garbage collection. | ||
|
||
In some cases, the client may have already been garbage collected | ||
(the client is null). In that case, there is no point in doing the | ||
operation and it should exit early to avoid UAF/crashes. | ||
|
||
This CL adds early exit to the cases that do not already have it. | ||
|
||
(cherry picked from commit 8755f76bec326c654370de6dd68eea693df74ede) | ||
|
||
Bug: 1514777 | ||
Change-Id: I27e9541cfaa74d978799c03e2832a0980f9e5710 | ||
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5210359 | ||
Reviewed-by: Tomas Gunnarsson <tommi@chromium.org> | ||
Commit-Queue: Guido Urdaneta <guidou@chromium.org> | ||
Cr-Original-Commit-Position: refs/heads/main@{#1248826} | ||
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5233883 | ||
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com> | ||
Auto-Submit: Guido Urdaneta <guidou@chromium.org> | ||
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com> | ||
Cr-Commit-Position: refs/branch-heads/6099@{#1867} | ||
Cr-Branched-From: e6ee4500f7d6549a9ac1354f8d056da49ef406be-refs/heads/main@{#1217362} | ||
|
||
diff --git a/third_party/blink/renderer/modules/peerconnection/rtc_peer_connection_handler.cc b/third_party/blink/renderer/modules/peerconnection/rtc_peer_connection_handler.cc | ||
index 76fa93800543ff134859c8fc0c0fa63123cf9772..9e5ce0572cfd1d2dd729e5f560b021aba05653f3 100644 | ||
--- a/third_party/blink/renderer/modules/peerconnection/rtc_peer_connection_handler.cc | ||
+++ b/third_party/blink/renderer/modules/peerconnection/rtc_peer_connection_handler.cc | ||
@@ -1057,15 +1057,19 @@ bool RTCPeerConnectionHandler::Initialize( | ||
WebLocalFrame* frame, | ||
ExceptionState& exception_state) { | ||
DCHECK(task_runner_->RunsTasksInCurrentSequence()); | ||
- DCHECK(frame); | ||
DCHECK(dependency_factory_); | ||
- frame_ = frame; | ||
|
||
CHECK(!initialize_called_); | ||
initialize_called_ = true; | ||
|
||
// Prevent garbage collection of client_ during processing. | ||
auto* client_on_stack = client_; | ||
+ if (!client_on_stack) { | ||
+ return false; | ||
+ } | ||
+ | ||
+ DCHECK(frame); | ||
+ frame_ = frame; | ||
peer_connection_tracker_ = PeerConnectionTracker::From(*frame); | ||
|
||
configuration_ = server_configuration; | ||
@@ -2312,10 +2316,13 @@ void RTCPeerConnectionHandler::OnIceCandidate(const String& sdp, | ||
int sdp_mline_index, | ||
int component, | ||
int address_family) { | ||
+ DCHECK(task_runner_->RunsTasksInCurrentSequence()); | ||
// In order to ensure that the RTCPeerConnection is not garbage collected | ||
// from under the function, we keep a pointer to it on the stack. | ||
auto* client_on_stack = client_; | ||
- DCHECK(task_runner_->RunsTasksInCurrentSequence()); | ||
+ if (!client_on_stack) { | ||
+ return; | ||
+ } | ||
TRACE_EVENT0("webrtc", "RTCPeerConnectionHandler::OnIceCandidateImpl"); | ||
// This line can cause garbage collection. | ||
auto* platform_candidate = MakeGarbageCollected<RTCIceCandidatePlatform>( |