electron · deepak1556 · Oct 10, 2021 · Oct 1, 2021 · Oct 1, 2021 · Oct 4, 2021
@@ -108,4 +108,5 @@ cherry-pick-f8a74d72f328.patch
 cherry-pick-f2fd53c6d706.patch
 cherry-pick-096afc1c5428.patch
 cherry-pick-4e528a5a8d83.patch
+m90-lts_prevents_non-browser_processes_from_requesting_memory.patch
 check_direction_of_rtcencodedframes.patch
@@ -0,0 +1,227 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Erik Chen <erikchen@chromium.org>
+Date: Fri, 1 Oct 2021 12:44:24 +0000
+Subject: Prevents non-browser processes from requesting memory dumps.
+
+This CL makes several changes:
+
+(1) Causes the browser to reset non-browser
+mojo::PendingReceiver<Coordinator>. This means that non-browser
+processes will never be able to use the Coordinator interface.
+
+(2) Add CHECKs to existing code to prevent non-browser processes from
+attempting to use the Coordinator interface.
+
+A code audit shows that all Coordinator usages should already only be
+from the browser process.
+
+Note that (2) is important since attempting to use an unbound interface
+will trigger a nullptr dereference, which is undefined behavior.
+
+(cherry picked from commit d9cc471e122e9a2391a68fa7cd72ea50587d8d97)
+
+Bug: 1251787
+Change-Id: Ifbe9610cc0e373edaaa60fad46b447e8bdb3ec04
+Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3174305
+Auto-Submit: Erik Chen <erikchen@chromium.org>
+Commit-Queue: Erik Chen <erikchen@chromium.org>
+Cr-Original-Commit-Position: refs/heads/main@{#923693}
+Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3197813
+Reviewed-by: Achuith Bhandarkar <achuith@chromium.org>
+Owners-Override: Achuith Bhandarkar <achuith@chromium.org>
+Commit-Queue: Roger Felipe Zanoni da Silva <rzanoni@google.com>
+Cr-Commit-Position: refs/branch-heads/4430@{#1631}
+Cr-Branched-From: e5ce7dc4f7518237b3d9bb93cccca35d25216cbe-refs/heads/master@{#857950}
+
+diff --git a/content/browser/browser_child_process_host_impl.cc b/content/browser/browser_child_process_host_impl.cc
+index 9a836207989b19ae041a3ec700b55185d63f9a4c..ed1d10aae4470c9aac28cfce67d97f351b8266d5 100644
+--- a/content/browser/browser_child_process_host_impl.cc
++++ b/content/browser/browser_child_process_host_impl.cc
+@@ -708,6 +708,9 @@ void BrowserChildProcessHostImpl::RegisterCoordinatorClient(
+     mojo::PendingReceiver<memory_instrumentation::mojom::Coordinator> receiver,
+     mojo::PendingRemote<memory_instrumentation::mojom::ClientProcess>
+         client_process) {
++  // Intentionally disallow non-browser processes from getting a Coordinator.
++  receiver.reset();
++
+   // The child process may have already terminated by the time this message is
+   // dispatched. We do nothing in that case.
+   if (!IsProcessLaunched())
+diff --git a/content/browser/renderer_host/render_process_host_impl.cc b/content/browser/renderer_host/render_process_host_impl.cc
+index bd24cb0f6edc37bf23c40f0a7839a2f5d85c9a01..09d883fba5167ea254e8e463bb6e79026bf61f50 100644
+--- a/content/browser/renderer_host/render_process_host_impl.cc
++++ b/content/browser/renderer_host/render_process_host_impl.cc
+@@ -2860,6 +2860,9 @@ void RenderProcessHostImpl::RegisterCoordinatorClient(
+     mojo::PendingReceiver<memory_instrumentation::mojom::Coordinator> receiver,
+     mojo::PendingRemote<memory_instrumentation::mojom::ClientProcess>
+         client_process) {
++  // Intentionally disallow non-browser processes from getting a Coordinator.
++  receiver.reset();
++
+   if (!GetProcess().IsValid()) {
+     // If the process dies before we get this message. we have no valid PID
+     // and there's nothing to register.
+diff --git a/services/resource_coordinator/memory_instrumentation/coordinator_impl.cc b/services/resource_coordinator/memory_instrumentation/coordinator_impl.cc
+index 741e35dc3df4f2e6b73a8873b034bd9fc7fdc6cc..0f4551f6c4f6cc6d3285149411d851936dda3c8c 100644
+--- a/services/resource_coordinator/memory_instrumentation/coordinator_impl.cc
++++ b/services/resource_coordinator/memory_instrumentation/coordinator_impl.cc
+@@ -105,7 +105,8 @@ void CoordinatorImpl::RegisterClientProcess(
+     const absl::optional<std::string>& service_name) {
+   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+   mojo::Remote<mojom::ClientProcess> process(std::move(client_process));
+-  coordinator_receivers_.Add(this, std::move(receiver), process_id);
++  if (receiver.is_valid())
++    coordinator_receivers_.Add(this, std::move(receiver), process_id);
+   process.set_disconnect_handler(
+       base::BindOnce(&CoordinatorImpl::UnregisterClientProcess,
+                      base::Unretained(this), process_id));
+diff --git a/services/resource_coordinator/public/cpp/memory_instrumentation/client_process_impl.cc b/services/resource_coordinator/public/cpp/memory_instrumentation/client_process_impl.cc
+index fea3e999ee0c624948445421829a731879c4fe74..1afe539a08287a81cd2590a2c4901fa528a37df3 100644
+--- a/services/resource_coordinator/public/cpp/memory_instrumentation/client_process_impl.cc
++++ b/services/resource_coordinator/public/cpp/memory_instrumentation/client_process_impl.cc
+@@ -24,6 +24,11 @@ void ClientProcessImpl::CreateInstance(
+     mojo::PendingReceiver<mojom::ClientProcess> receiver,
+     mojo::PendingRemote<mojom::Coordinator> coordinator,
+     bool is_browser_process) {
++  // Intentionally disallow non-browser processes from ever holding a
++  // Coordinator.
++  if (!is_browser_process)
++    coordinator.reset();
++
+   static ClientProcessImpl* instance = nullptr;
+   if (!instance) {
+     instance = new ClientProcessImpl(
+@@ -39,10 +44,12 @@ ClientProcessImpl::ClientProcessImpl(
+     mojo::PendingRemote<mojom::Coordinator> coordinator,
+     bool is_browser_process,
+     bool initialize_memory_instrumentation)
+-    : receiver_(this, std::move(receiver)) {
++    : receiver_(this, std::move(receiver)),
++      is_browser_process_(is_browser_process) {
+   if (initialize_memory_instrumentation) {
+     // Initialize the public-facing MemoryInstrumentation helper.
+-    MemoryInstrumentation::CreateInstance(std::move(coordinator));
++    MemoryInstrumentation::CreateInstance(std::move(coordinator),
++                                          is_browser_process);
+   } else {
+     coordinator_.Bind(std::move(coordinator));
+   }
+@@ -110,6 +117,8 @@ void ClientProcessImpl::OnChromeMemoryDumpDone(
+ void ClientProcessImpl::RequestGlobalMemoryDump_NoCallback(
+     base::trace_event::MemoryDumpType dump_type,
+     base::trace_event::MemoryDumpLevelOfDetail level_of_detail) {
++  CHECK(is_browser_process_);
++
+   if (!task_runner_->RunsTasksInCurrentSequence()) {
+     task_runner_->PostTask(
+         FROM_HERE,
+diff --git a/services/resource_coordinator/public/cpp/memory_instrumentation/client_process_impl.h b/services/resource_coordinator/public/cpp/memory_instrumentation/client_process_impl.h
+index d41651d9bef7bbc8ec1685e68cfd77993f0d4551..aef7f5a03091a0c42168cb2154edabf41d28d829 100644
+--- a/services/resource_coordinator/public/cpp/memory_instrumentation/client_process_impl.h
++++ b/services/resource_coordinator/public/cpp/memory_instrumentation/client_process_impl.h
+@@ -96,6 +96,9 @@ class COMPONENT_EXPORT(RESOURCE_COORDINATOR_PUBLIC_MEMORY_INSTRUMENTATION)
+   mojo::Remote<mojom::Coordinator> coordinator_;
+   scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
+
++  // Only browser process is allowed to request memory dumps.
++  const bool is_browser_process_;
++
+   // TODO(crbug.com/728199): The observer is only used to setup and tear down
+   // MemoryDumpManager in each process. Setting up MemoryDumpManager should
+   // be moved away from TracingObserver.
+diff --git a/services/resource_coordinator/public/cpp/memory_instrumentation/memory_instrumentation.cc b/services/resource_coordinator/public/cpp/memory_instrumentation/memory_instrumentation.cc
+index c81d5f83bf9e1ad5e7a77d7c187fa33bd02812d5..ec90ab9211ede586d441f40e3e2bc2c820658fb1 100644
+--- a/services/resource_coordinator/public/cpp/memory_instrumentation/memory_instrumentation.cc
++++ b/services/resource_coordinator/public/cpp/memory_instrumentation/memory_instrumentation.cc
+@@ -21,10 +21,11 @@ void WrapGlobalMemoryDump(
+
+ // static
+ void MemoryInstrumentation::CreateInstance(
+-    mojo::PendingRemote<memory_instrumentation::mojom::Coordinator>
+-        coordinator) {
++    mojo::PendingRemote<memory_instrumentation::mojom::Coordinator> coordinator,
++    bool is_browser_process) {
+   DCHECK(!g_instance);
+-  g_instance = new MemoryInstrumentation(std::move(coordinator));
++  g_instance =
++      new MemoryInstrumentation(std::move(coordinator), is_browser_process);
+ }
+
+ // static
+@@ -33,8 +34,10 @@ MemoryInstrumentation* MemoryInstrumentation::GetInstance() {
+ }
+
+ MemoryInstrumentation::MemoryInstrumentation(
+-    mojo::PendingRemote<memory_instrumentation::mojom::Coordinator> coordinator)
+-    : coordinator_(std::move(coordinator)) {}
++    mojo::PendingRemote<memory_instrumentation::mojom::Coordinator> coordinator,
++    bool is_browser_process)
++    : coordinator_(std::move(coordinator)),
++      is_browser_process_(is_browser_process) {}
+
+ MemoryInstrumentation::~MemoryInstrumentation() {
+   g_instance = nullptr;
+@@ -43,6 +46,7 @@ MemoryInstrumentation::~MemoryInstrumentation() {
+ void MemoryInstrumentation::RequestGlobalDump(
+     const std::vector<std::string>& allocator_dump_names,
+     RequestGlobalDumpCallback callback) {
++  CHECK(is_browser_process_);
+   coordinator_->RequestGlobalMemoryDump(
+       MemoryDumpType::SUMMARY_ONLY, MemoryDumpLevelOfDetail::BACKGROUND,
+       MemoryDumpDeterminism::NONE, allocator_dump_names,
+@@ -52,6 +56,7 @@ void MemoryInstrumentation::RequestGlobalDump(
+ void MemoryInstrumentation::RequestPrivateMemoryFootprint(
+     base::ProcessId pid,
+         RequestGlobalDumpCallback callback) {
++  CHECK(is_browser_process_);
+   coordinator_->RequestPrivateMemoryFootprint(
+       pid, base::BindOnce(&WrapGlobalMemoryDump, std::move(callback)));
+ }
+@@ -60,6 +65,7 @@ void MemoryInstrumentation::RequestGlobalDumpForPid(
+     base::ProcessId pid,
+     const std::vector<std::string>& allocator_dump_names,
+     RequestGlobalDumpCallback callback) {
++  CHECK(is_browser_process_);
+   coordinator_->RequestGlobalMemoryDumpForPid(
+       pid, allocator_dump_names,
+       base::BindOnce(&WrapGlobalMemoryDump, std::move(callback)));
+@@ -70,6 +76,7 @@ void MemoryInstrumentation::RequestGlobalDumpAndAppendToTrace(
+     MemoryDumpLevelOfDetail level_of_detail,
+     MemoryDumpDeterminism determinism,
+     RequestGlobalMemoryDumpAndAppendToTraceCallback callback) {
++  CHECK(is_browser_process_);
+   coordinator_->RequestGlobalMemoryDumpAndAppendToTrace(
+       dump_type, level_of_detail, determinism, std::move(callback));
+ }
+diff --git a/services/resource_coordinator/public/cpp/memory_instrumentation/memory_instrumentation.h b/services/resource_coordinator/public/cpp/memory_instrumentation/memory_instrumentation.h
+index 3264917890cc30179c4477657158fd359a9d1e01..72157b5345fb003452f67045e2b2c984e748958a 100644
+--- a/services/resource_coordinator/public/cpp/memory_instrumentation/memory_instrumentation.h
++++ b/services/resource_coordinator/public/cpp/memory_instrumentation/memory_instrumentation.h
+@@ -34,7 +34,8 @@ class COMPONENT_EXPORT(RESOURCE_COORDINATOR_PUBLIC_MEMORY_INSTRUMENTATION)
+
+   static void CreateInstance(
+       mojo::PendingRemote<memory_instrumentation::mojom::Coordinator>
+-          coordinator);
++          coordinator,
++      bool is_browser_process);
+   static MemoryInstrumentation* GetInstance();
+
+   // Retrieves a Coordinator interface to communicate with the service. This is
+@@ -100,12 +101,16 @@ class COMPONENT_EXPORT(RESOURCE_COORDINATOR_PUBLIC_MEMORY_INSTRUMENTATION)
+  private:
+   explicit MemoryInstrumentation(
+       mojo::PendingRemote<memory_instrumentation::mojom::Coordinator>
+-          coordinator);
++          coordinator,
++      bool is_browser_process);
+   ~MemoryInstrumentation();
+
+   const mojo::SharedRemote<memory_instrumentation::mojom::Coordinator>
+       coordinator_;
+
++  // Only browser process is allowed to request memory dumps.
++  const bool is_browser_process_;
++
+   DISALLOW_COPY_AND_ASSIGN(MemoryInstrumentation);
+ };
+