chore: cherry-pick 3 changes from Release-1-M117 (#40078)

* chore: [25-x-y] cherry-pick 3 changes from Release-1-M117

* b0ad701a609a from v8
* b11e7d07a6f4 from chromium
* 309b604c4e88 from chromium

* chore: update patches

---------

Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>
This commit is contained in:
Pedro Pontes
2023-10-06 14:54:50 +01:00
committed by GitHub
parent bb01e52fb8
commit bd43e652f6
5 changed files with 250 additions and 0 deletions

View File

@@ -141,3 +141,5 @@ revert_remove_the_allowaggressivethrottlingwithwebsocket_feature.patch
cherry-pick-74a2eb9c8cb2.patch
cherry-pick-26175b0903d8.patch
fix_use_delegated_generic_capturer_when_available.patch
cherry-pick-b11e7d07a6f4.patch
cherry-pick-309b604c4e88.patch

View File

@@ -0,0 +1,151 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Tsuyoshi Horo <horo@chromium.org>
Date: Fri, 29 Sep 2023 08:13:50 +0000
Subject: Fix DataPipeDrainer usage in ExtensionLocalizationURLLoader
There is a bug that when ExtensionLocalizationURLLoader is destructed
by canceling the CSS requests from extensions, DataPipeProducer may
cause UAF.
This is because DataPipeProducer is not correctly used in
ExtensionLocalizationURLLoader. DataPipeProducer and the data must be
kept alive until notified of completion.
This CL fix this by changing ExtensionLocalizationURLLoader to keep
DataPipeProducer and the data even if ExtensionLocalizationURLLoader
itself is destructed.
(cherry picked from commit b6e060e17ed9e46b3043a3c369fc10cbbe2245d8)
Bug: 1475798
Change-Id: I013396f2c49f4712914b917c3330b99a1be791b8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4821086
Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1191115}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4872577
Commit-Queue: Zakhar Voit <voit@google.com>
Owners-Override: Victor Gabriel Savu <vsavu@google.com>
Reviewed-by: Victor Gabriel Savu <vsavu@google.com>
Cr-Commit-Position: refs/branch-heads/5735@{#1611}
Cr-Branched-From: 2f562e4ddbaf79a3f3cb338b4d1bd4398d49eb67-refs/heads/main@{#1135570}
diff --git a/extensions/renderer/extension_localization_throttle.cc b/extensions/renderer/extension_localization_throttle.cc
index 382283cea8a8a061f769dc234b67ef8b53b47517..65bbaf5a990fc86105048492e36813645a6a788c 100644
--- a/extensions/renderer/extension_localization_throttle.cc
+++ b/extensions/renderer/extension_localization_throttle.cc
@@ -5,6 +5,7 @@
#include "extensions/renderer/extension_localization_throttle.h"
#include "base/memory/ptr_util.h"
+#include "base/memory/weak_ptr.h"
#include "base/strings/string_piece.h"
#include "base/strings/string_util.h"
#include "content/public/renderer/render_thread.h"
@@ -51,8 +52,7 @@ class ExtensionLocalizationURLLoader : public network::mojom::URLLoaderClient,
data_drainer_ =
std::make_unique<mojo::DataPipeDrainer>(this, std::move(body));
- data_producer_ =
- std::make_unique<mojo::DataPipeProducer>(std::move(producer_handle));
+ producer_handle_ = std::move(producer_handle);
return true;
}
@@ -134,18 +134,31 @@ class ExtensionLocalizationURLLoader : public network::mojom::URLLoaderClient,
ReplaceMessages();
}
- // Safe to use Unretained(this) because `this` owns `data_producer_`.
- data_producer_->Write(
- std::make_unique<mojo::StringDataSource>(
- base::StringPiece(data_), mojo::StringDataSource::AsyncWritingMode::
- STRING_STAYS_VALID_UNTIL_COMPLETION),
- base::BindOnce(&ExtensionLocalizationURLLoader::OnDataWritten,
- base::Unretained(this)));
+ auto data_producer =
+ std::make_unique<mojo::DataPipeProducer>(std::move(producer_handle_));
+ auto data = std::make_unique<std::string>(std::move(data_));
+ // To avoid unnecessary string copy, use STRING_STAYS_VALID_UNTIL_COMPLETION
+ // here, and keep the original data hold in the closure below.
+ auto source = std::make_unique<mojo::StringDataSource>(
+ *data, mojo::StringDataSource::AsyncWritingMode::
+ STRING_STAYS_VALID_UNTIL_COMPLETION);
+ mojo::DataPipeProducer* data_producer_ptr = data_producer.get();
+ data_producer_ptr->Write(
+ std::move(source),
+ base::BindOnce(
+ [](std::unique_ptr<mojo::DataPipeProducer> data_producer,
+ std::unique_ptr<std::string> data,
+ base::OnceCallback<void(MojoResult)> on_data_written_callback,
+ MojoResult result) {
+ std::move(on_data_written_callback).Run(result);
+ },
+ std::move(data_producer), std::move(data),
+ base::BindOnce(&ExtensionLocalizationURLLoader::OnDataWritten,
+ weak_factory_.GetWeakPtr())));
}
private:
void OnDataWritten(MojoResult result) {
- data_producer_.reset();
data_write_result_ = result;
MaybeSendOnComplete();
}
@@ -170,7 +183,7 @@ class ExtensionLocalizationURLLoader : public network::mojom::URLLoaderClient,
const std::string extension_id_;
std::unique_ptr<mojo::DataPipeDrainer> data_drainer_;
- std::unique_ptr<mojo::DataPipeProducer> data_producer_;
+ mojo::ScopedDataPipeProducerHandle producer_handle_;
std::string data_;
absl::optional<network::URLLoaderCompletionStatus> original_complete_status_;
@@ -180,6 +193,7 @@ class ExtensionLocalizationURLLoader : public network::mojom::URLLoaderClient,
this};
mojo::Remote<network::mojom::URLLoader> source_url_loader_;
mojo::Remote<network::mojom::URLLoaderClient> destination_url_loader_client_;
+ base::WeakPtrFactory<ExtensionLocalizationURLLoader> weak_factory_{this};
};
} // namespace
diff --git a/extensions/renderer/extension_localization_throttle_unittest.cc b/extensions/renderer/extension_localization_throttle_unittest.cc
index 732402366b697e3e982fe5feb3d25cb2e726abdd..221ee9062da9ea4262a4903cf815abf82cfb5d99 100644
--- a/extensions/renderer/extension_localization_throttle_unittest.cc
+++ b/extensions/renderer/extension_localization_throttle_unittest.cc
@@ -306,6 +306,37 @@ TEST_F(ExtensionLocalizationThrottleTest, EmptyData) {
delegate->destination_loader_client()->completion_status().error_code);
}
+// Regression test for https://crbug.com/1475798
+TEST_F(ExtensionLocalizationThrottleTest, Cancel) {
+ const GURL url("chrome-extension://some_id/test.css");
+ auto throttle =
+ ExtensionLocalizationThrottle::MaybeCreate(blink::WebURL(url));
+ ASSERT_TRUE(throttle);
+
+ auto delegate = std::make_unique<FakeDelegate>();
+ throttle->set_delegate(delegate.get());
+
+ auto response_head = network::mojom::URLResponseHead::New();
+ response_head->mime_type = "text/css";
+ bool defer = false;
+ throttle->WillProcessResponse(url, response_head.get(), &defer);
+ EXPECT_FALSE(defer);
+ EXPECT_TRUE(delegate->is_intercepted());
+ delegate->LoadResponseBody("__MSG_hello__!");
+ delegate->CompleteResponse();
+ // Run all tasks in the main thread to make DataPipeProducer::SequenceState
+ // call PostTask(&SequenceState::StartOnSequence) to a background thread.
+ base::RunLoop().RunUntilIdle();
+ // Resetting `destination_loader_remote` triggers
+ // ExtensionLocalizationURLLoader destruction.
+ delegate->destination_loader_remote().reset();
+ // Run all tasks in the main thread to destroy the
+ // ExtensionLocalizationURLLoader.
+ base::RunLoop().RunUntilIdle();
+ // Runs SequenceState::StartOnSequence in the background thread.
+ task_environment_.RunUntilIdle();
+}
+
TEST_F(ExtensionLocalizationThrottleTest, SourceSideError) {
const GURL url("chrome-extension://some_id/test.css");
auto throttle =

View File

@@ -0,0 +1,40 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Lei Zhang <thestig@chromium.org>
Date: Wed, 13 Sep 2023 23:32:40 +0000
Subject: M117: Check for object destruction in PdfViewWebPlugin::UpdateFocus()
PdfViewWebPlugin::UpdateFocus() can potentially triggers its own
destruction. Add a check for this and bail out.
(cherry picked from commit cacf485a202b342526374d444375b80a044add76)
Bug: 1480184
Change-Id: I5e7760ed541a2bffb9dd1ebeb522f10650049033
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4852346
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com>
Commit-Queue: Lei Zhang <thestig@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1194210}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4863395
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/branch-heads/5938@{#1286}
Cr-Branched-From: 2b50cb4bcc2318034581a816714d9535dc38966d-refs/heads/main@{#1181205}
diff --git a/pdf/pdf_view_web_plugin.cc b/pdf/pdf_view_web_plugin.cc
index 1abf1f5df13bb2f41862300adb166691e9625ab7..f9fbb3d718b885874c1def1c005d7cc710591fae 100644
--- a/pdf/pdf_view_web_plugin.cc
+++ b/pdf/pdf_view_web_plugin.cc
@@ -515,7 +515,13 @@ void PdfViewWebPlugin::UpdateFocus(bool focused,
if (has_focus_ != focused) {
engine_->UpdateFocus(focused);
client_->UpdateTextInputState();
+
+ // Make sure `this` is still alive after the UpdateSelectionBounds() call.
+ auto weak_this = weak_factory_.GetWeakPtr();
client_->UpdateSelectionBounds();
+ if (!weak_this) {
+ return;
+ }
}
has_focus_ = focused;

View File

@@ -13,3 +13,4 @@ cherry-pick-8ff63d378f2c.patch
merged_squashed_multiple_commits.patch
cherry-pick-038530c94a06.patch
cherry-pick-cf1d4d3c0b6e.patch
cherry-pick-b0ad701a609a.patch

View File

@@ -0,0 +1,56 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Shu-yu Guo <syg@chromium.org>
Date: Wed, 6 Sep 2023 17:36:38 -0700
Subject: Merged: [builtins] Clear FixedArray slot in Promise builtins
(cherry picked from commit f1884222ad56734e56d80f9707e0e8279af9049e)
Bug: chromium:1479104
Change-Id: Iddc16d8add4dc6bf6f55f537da44770bea6f4bc3
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4862980
Auto-Submit: Shu-yu Guo <syg@chromium.org>
Reviewed-by: Adam Klein <adamk@chromium.org>
Commit-Queue: Adam Klein <adamk@chromium.org>
Cr-Commit-Position: refs/branch-heads/11.6@{#36}
Cr-Branched-From: e29c028f391389a7a60ee37097e3ca9e396d6fa4-refs/heads/11.6.189@{#3}
Cr-Branched-From: 95cbef20e2aa556a1ea75431a48b36c4de6b9934-refs/heads/main@{#88340}
diff --git a/src/builtins/promise-any.tq b/src/builtins/promise-any.tq
index 45bafac0e6b09143b69b21a7292f9ed6b9c46239..d531d57a375ba33bf11ccf698da5918f1e25f38c 100644
--- a/src/builtins/promise-any.tq
+++ b/src/builtins/promise-any.tq
@@ -106,9 +106,10 @@ PromiseAnyRejectElementClosure(
const index = identityHash - 1;
// 6. Let errors be F.[[Errors]].
- let errors = *ContextSlot(
+ let errorsRef:&FixedArray = ContextSlot(
context,
PromiseAnyRejectElementContextSlots::kPromiseAnyRejectElementErrorsSlot);
+ let errors = *errorsRef;
// 7. Let promiseCapability be F.[[Capability]].
@@ -134,10 +135,7 @@ PromiseAnyRejectElementClosure(
IntPtrMax(SmiUntag(remainingElementsCount) - 1, index + 1);
if (newCapacity > errors.length_intptr) deferred {
errors = ExtractFixedArray(errors, 0, errors.length_intptr, newCapacity);
- *ContextSlot(
- context,
- PromiseAnyRejectElementContextSlots::
- kPromiseAnyRejectElementErrorsSlot) = errors;
+ *errorsRef = errors;
}
errors.objects[index] = value;
@@ -155,6 +153,10 @@ PromiseAnyRejectElementClosure(
// b. Set error.[[AggregateErrors]] to errors.
const error = ConstructAggregateError(errors);
+
+ // After this point, errors escapes to user code. Clear the slot.
+ *errorsRef = kEmptyFixedArray;
+
// c. Return ? Call(promiseCapability.[[Reject]], undefined, « error »).
const capability = *ContextSlot(
context,