github/electron
1
1
Fork 0
mirror of https://github.com/electron/electron.git synced 2026-04-10 03:01:51 -04:00
Code Issues Packages Projects Releases Wiki Activity

chore: cherry-pick 4 changes from Release-1-M121 (#41176)

Browse Source 
* chore: [27-x-y] cherry-pick 4 changes from Release-1-M121

* d4a197e4913f from chromium
* 8755f76bec32 from chromium
* e321f354a613 from chromium
* 4a98f9e304be from chromium

* chore: update patches

---------

Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>
This commit is contained in:
Shelley Vohr
2024-02-02 12:33:18 +01:00
committed by GitHub
parent ca92ebff25
commit b1377d42d9
5 changed files with 431 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
patches/chromium/.patches
View File

@@ -150,3 +150,7 @@ reland_mojom_ts_generator_handle_empty_module_path_identically_to.patch
cherry-pick-c1cda70a433a.patch
cherry-pick-cc07a95bc309.patch
safely_crash_on_dangling_profile.patch
cherry-pick-91a02d67a83d.patch
cherry-pick-8755f76bec32.patch
cherry-pick-1f8bec968902.patch
cherry-pick-4a98f9e304be.patch

125
patches/chromium/cherry-pick-1f8bec968902.patch Normal file
View File

@@ -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

67
patches/chromium/cherry-pick-4a98f9e304be.patch Normal file
View File

@@ -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;

110
patches/chromium/cherry-pick-8755f76bec32.patch Normal file
View File

@@ -0,0 +1,110 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Guido Urdaneta <guidou@chromium.org>
Date: Thu, 18 Jan 2024 16:47:18 +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.
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-Commit-Position: refs/heads/main@{#1248826}
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 473eb82685f004d960f3d488e3976a2305eda248..b6d893cc0f93d5bbb12b07734c63b31a52d662f1 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
@@ -1056,15 +1056,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_.Get();
+ 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_.Get();
- 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>(
diff --git a/third_party/blink/renderer/modules/peerconnection/rtc_peer_connection_handler_test.cc b/third_party/blink/renderer/modules/peerconnection/rtc_peer_connection_handler_test.cc
index f8838ab0c5a31182e210cc6f42b9f489c7dd5365..f689a679a7589f16839349189cbfdf84efd14367 100644
--- a/third_party/blink/renderer/modules/peerconnection/rtc_peer_connection_handler_test.cc
+++ b/third_party/blink/renderer/modules/peerconnection/rtc_peer_connection_handler_test.cc
@@ -601,7 +601,7 @@ class RTCPeerConnectionHandlerTest : public SimTest {
waitable_event->Signal();
}
- public:
+ protected:
ScopedTestingPlatformSupport<AudioCapturerSourceTestingPlatformSupport>
webrtc_audio_device_platform_support_;
Persistent<MockRTCPeerConnectionHandlerClient> mock_client_;
@@ -1168,4 +1168,32 @@ TEST_F(RTCPeerConnectionHandlerTest,
observer->OnIceCandidate(native_candidate.get());
}
+TEST_F(RTCPeerConnectionHandlerTest,
+ OnIceCandidateAfterClientGarbageCollectionDoesNothing) {
+ testing::InSequence sequence;
+ EXPECT_CALL(*mock_tracker_.Get(),
+ TrackAddIceCandidate(pc_handler_.get(), _,
+ PeerConnectionTracker::kSourceLocal, true))
+ .Times(0);
+
+ std::unique_ptr<webrtc::IceCandidateInterface> native_candidate(
+ mock_dependency_factory_->CreateIceCandidate("sdpMid", 1, kDummySdp));
+ mock_client_ = nullptr;
+ WebHeap::CollectAllGarbageForTesting();
+ pc_handler_->observer()->OnIceCandidate(native_candidate.get());
+ RunMessageLoopsUntilIdle();
+}
+
+TEST_F(RTCPeerConnectionHandlerTest,
+ OnIceCandidateAfterClientGarbageCollectionFails) {
+ DummyExceptionStateForTesting exception_state;
+ auto pc_handler = CreateRTCPeerConnectionHandlerUnderTest();
+ mock_client_ = nullptr;
+ WebHeap::CollectAllGarbageForTesting();
+ EXPECT_FALSE(pc_handler->Initialize(
+ /*context=*/nullptr, webrtc::PeerConnectionInterface::RTCConfiguration(),
+ /*media_constraints=*/nullptr,
+ /*frame=*/nullptr, exception_state));
+}
+
} // namespace blink

125
patches/chromium/cherry-pick-91a02d67a83d.patch Normal file
View File

@@ -0,0 +1,125 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Jean-Philippe Gravel <jpgravel@chromium.org>
Date: Tue, 23 Jan 2024 21:37:50 +0000
Subject: Fix use-after-free in DrawTextInternal
DrawTextInternal was calling GetOrCreatePaintCanvas multiple times,
once at the start of the function, once inside of the
BaseRenderingContext2DAutoRestoreSkCanvas helper class and once in the
Draw call. GetOrCreatePaintCanvas destroys the canvas resource provider
if the GPU context is lost. If this happens on the second call to
GetOrCreatePaintCanvas, destroying the resource provider will
invalidate the cc::PaintCanvas returned by the first call to
GetOrCreatePaintCanvas.
The GPU process can technically crash at any point during the renderer
process execution (perhaps because of something another renderer
process did). We therefore have to assume that any call to
GetOrCreatePaintCanvas can invalidate previously returned
cc::PaintCanvas.
(cherry picked from commit d4a197e4913f8e5072263b59aedc29f2b5af3e93)
Change-Id: Ifa77735ab1b2b55b3d494f886b8566299937f6fe
Fixed: 1511567
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5198419
Reviewed-by: Fernando Serboncini <fserb@chromium.org>
Commit-Queue: Jean-Philippe Gravel <jpgravel@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1248204}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5230237
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/branch-heads/6099@{#1859}
Cr-Branched-From: e6ee4500f7d6549a9ac1354f8d056da49ef406be-refs/heads/main@{#1217362}
diff --git a/third_party/blink/renderer/modules/canvas/canvas2d/base_rendering_context_2d.cc b/third_party/blink/renderer/modules/canvas/canvas2d/base_rendering_context_2d.cc
index f09459b0b67c37d307c70a516731d05db49f49b8..89cb2a41b3eef8bb359984b0d14bfa0e4e19cfdc 100644
--- a/third_party/blink/renderer/modules/canvas/canvas2d/base_rendering_context_2d.cc
+++ b/third_party/blink/renderer/modules/canvas/canvas2d/base_rendering_context_2d.cc
@@ -2682,40 +2682,6 @@ const Font& BaseRenderingContext2D::AccessFont(HTMLCanvasElement* canvas) {
return GetState().GetFont();
}
-namespace {
-
-// Drawing methods need to use this instead of SkAutoCanvasRestore in case
-// overdraw detection substitutes the recording canvas (to discard overdrawn
-// draw calls).
-class BaseRenderingContext2DAutoRestoreSkCanvas {
- STACK_ALLOCATED();
-
- public:
- explicit BaseRenderingContext2DAutoRestoreSkCanvas(
- BaseRenderingContext2D* context)
- : context_(context) {
- DCHECK(context_);
- cc::PaintCanvas* c = context_->GetOrCreatePaintCanvas();
- if (c) {
- save_count_ = c->getSaveCount();
- }
- }
-
- ~BaseRenderingContext2DAutoRestoreSkCanvas() {
- cc::PaintCanvas* c = context_->GetOrCreatePaintCanvas();
- if (c) {
- c->restoreToCount(save_count_);
- }
- context_->ValidateStateStack();
- }
-
- private:
- BaseRenderingContext2D* context_;
- int save_count_ = 0;
-};
-
-} // namespace
-
void BaseRenderingContext2D::DrawTextInternal(
const String& text,
double x,
@@ -2736,8 +2702,10 @@ void BaseRenderingContext2D::DrawTextInternal(
canvas->GetDocument().UpdateStyleAndLayoutTreeForNode(
canvas, DocumentUpdateReason::kCanvas);
}
- cc::PaintCanvas* c = GetOrCreatePaintCanvas();
- if (!c) {
+
+ // Abort if we don't have a paint canvas (e.g. the context was lost).
+ cc::PaintCanvas* paint_canvas = GetOrCreatePaintCanvas();
+ if (!paint_canvas) {
return;
}
@@ -2808,14 +2776,13 @@ void BaseRenderingContext2D::DrawTextInternal(
InflateStrokeRect(bounds);
}
- BaseRenderingContext2DAutoRestoreSkCanvas state_restorer(this);
if (use_max_width) {
- c->save();
+ paint_canvas->save();
// We draw when fontWidth is 0 so compositing operations (eg, a "copy" op)
// still work. As the width of canvas is scaled, so text can be scaled to
// match the given maxwidth, update text location so it appears on desired
// place.
- c->scale(ClampTo<float>(width / font_width), 1);
+ paint_canvas->scale(ClampTo<float>(width / font_width), 1);
location.set_x(location.x() / ClampTo<float>(width / font_width));
}
@@ -2846,6 +2813,16 @@ void BaseRenderingContext2D::DrawTextInternal(
{ return false; },
bounds, paint_type, CanvasRenderingContext2DState::kNoImage,
CanvasPerformanceMonitor::DrawType::kText);
+
+ if (use_max_width) {
+ // Cannot use `paint_canvas` in case recording canvas was substituted or
+ // destroyed during draw call.
+ cc::PaintCanvas* c = GetPaintCanvas();
+ if (c) {
+ c->restore();
+ }
+ }
+ ValidateStateStack();
}
TextMetrics* BaseRenderingContext2D::measureText(const String& text) {