refactor: type-safe module imports / requires (#41236)

* refactor: type-safe module imports / requires

Co-authored-by: Milan Burda <milan.burda@gmail.com>

* chore: update patches

---------

Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com>
Co-authored-by: Milan Burda <milan.burda@gmail.com>
Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>
This commit is contained in:
trop[bot]
2024-02-05 16:32:50 -05:00
committed by GitHub
parent ace362c4c4
commit d6221eaf2d
9 changed files with 11 additions and 437 deletions

View File

@@ -3,6 +3,8 @@ import * as fs from 'fs';
import * as path from 'path';
import type * as defaultMenuModule from '@electron/internal/browser/default-menu';
import type * as url from 'url';
import type * as v8 from 'v8';
const Module = require('module') as NodeJS.ModuleInternal;
@@ -132,7 +134,7 @@ if (packageJson.desktopName != null) {
// Set v8 flags, deliberately lazy load so that apps that do not use this
// feature do not pay the price
if (packageJson.v8Flags != null) {
require('v8').setFlagsFromString(packageJson.v8Flags);
(require('v8') as typeof v8).setFlagsFromString(packageJson.v8Flags);
}
app.setAppPath(packagePath);
@@ -195,7 +197,7 @@ if (packagePath) {
// Finally load app's main.js and transfer control to C++.
if ((packageJson.type === 'module' && !mainStartupScript.endsWith('.cjs')) || mainStartupScript.endsWith('.mjs')) {
const { loadESM } = __non_webpack_require__('internal/process/esm_loader');
const main = require('url').pathToFileURL(path.join(packagePath, mainStartupScript));
const main = (require('url') as typeof url).pathToFileURL(path.join(packagePath, mainStartupScript));
loadESM(async (esmLoader: any) => {
try {
await esmLoader.import(main.toString(), undefined, Object.create(null));

View File

@@ -1,6 +1,7 @@
import * as util from 'util';
import type * as stream from 'stream';
const timers = require('timers');
import timers = require('timers');
type AnyFn = (...args: any[]) => any
@@ -62,7 +63,7 @@ if (process.type === 'browser' ||
if (process.platform === 'win32') {
// Always returns EOF for stdin stream.
const { Readable } = require('stream');
const { Readable } = require('stream') as typeof stream;
const stdin = new Readable();
stdin.push(null);
Object.defineProperty(process, 'stdin', {

View File

@@ -2,7 +2,9 @@ import { Buffer } from 'buffer';
import { constants } from 'fs';
import * as path from 'path';
import * as util from 'util';
import type * as Crypto from 'crypto';
import type * as os from 'os';
const asar = process._linkedBinding('electron_common_asar');
@@ -255,7 +257,7 @@ export const wrapFsWithAsar = (fs: Record<string, any>) => {
if (!process.env.ELECTRON_LOG_ASAR_READS) return;
if (!logFDs.has(asarPath)) {
const logFilename = `${path.basename(asarPath, '.asar')}-access-log.txt`;
const logPath = path.join(require('os').tmpdir(), logFilename);
const logPath = path.join((require('os') as typeof os).tmpdir(), logFilename);
logFDs.set(asarPath, fs.openSync(logPath, 'a'));
}
fs.writeSync(logFDs.get(asarPath), `${offset}: ${filePath}\n`);

View File

@@ -1,4 +1,5 @@
import * as events from 'events';
import { setImmediate, clearImmediate } from 'timers';
import { IPC_MESSAGES } from '@electron/internal/common/ipc-messages';
import type * as ipcRendererUtilsModule from '@electron/internal/renderer/ipc-renderer-internal-utils';
@@ -126,7 +127,6 @@ function runPreloadScript (preloadSrc: string) {
// eval in window scope
const preloadFn = binding.createPreloadScript(preloadWrapperSrc);
const { setImmediate, clearImmediate } = require('timers');
const exports = {};
preloadFn(preloadRequire, preloadProcess, Buffer, global, setImmediate, clearImmediate, exports, { exports });

View File

@@ -144,7 +144,3 @@ feat_allow_code_cache_in_custom_schemes.patch
enable_partition_alloc_ref_count_size.patch
ensure_an_axcontext_before_painting.patch
safely_crash_on_dangling_profile.patch
cherry-pick-91a02d67a83d.patch
cherry-pick-8755f76bec32.patch
cherry-pick-1f8bec968902.patch
cherry-pick-4a98f9e304be.patch

View File

@@ -1,125 +0,0 @@
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 6be121003a1044ea55e086e7b0853589ded52732..62ad4f9fd58f29fae844cde5bbdb8273fdb40ae4 100644
--- a/services/network/public/cpp/source_stream_to_data_pipe.cc
+++ b/services/network/public/cpp/source_stream_to_data_pipe.cc
@@ -56,9 +56,9 @@ void SourceStreamToDataPipe::ReadMore() {
int num_bytes = base::checked_cast<int>(pending_write_->size());
scoped_refptr<net::IOBuffer> buffer(
new network::NetToMojoIOBuffer(pending_write_.get()));
- int result = source_->Read(
- buffer.get(), num_bytes,
- base::BindOnce(&SourceStreamToDataPipe::DidRead, base::Unretained(this)));
+ int result = source_->Read(buffer.get(), 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

View File

@@ -1,67 +0,0 @@
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;

View File

@@ -1,110 +0,0 @@
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 015c5abec1facd3fe9b427b197a137682b11f526..3afc7074a5775542ba2393aa007ff7c5abb3f728 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;
@@ -2321,10 +2325,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 eb44aabb481a66e08d432a9ed4f202d5ca27ee90..ee703c4317f57483a67ea0b67a19137cc859ecc6 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
@@ -602,7 +602,7 @@ class RTCPeerConnectionHandlerTest : public SimTest {
waitable_event->Signal();
}
- public:
+ protected:
ScopedTestingPlatformSupport<AudioCapturerSourceTestingPlatformSupport>
webrtc_audio_device_platform_support_;
Persistent<MockRTCPeerConnectionHandlerClient> mock_client_;
@@ -1170,4 +1170,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

View File

@@ -1,125 +0,0 @@
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 cf74c803a0d4b5a3229f2ebc9d8637ba4294f6a9..aa61546c345d0de41f36f372fb9981a8881269d6 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
@@ -2709,40 +2709,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,
@@ -2763,8 +2729,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;
}
@@ -2835,14 +2803,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));
}
@@ -2873,6 +2840,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) {