fix: crashReporter incompatible with sandbox on Linux (#23265)

This commit is contained in:
Jeremy Apthorp 2020-05-07 13:31:26 -07:00 committed by GitHub
parent fc434f136b
commit 06bf0d08dc
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
77 changed files with 2235 additions and 2404 deletions

View file

@ -89,3 +89,8 @@ feat_add_onclose_to_messageport.patch
fix_account_for_print_preview_disabled_when_printing_to_pdf.patch
web_contents.patch
ui_gtk_public_header.patch
crash_allow_setting_more_options.patch
breakpad_disable_upload_compression.patch
breakpad_treat_node_processes_as_browser_processes.patch
upload_list_add_loadsync_method.patch
breakpad_allow_getting_string_values_for_crash_keys.patch

View file

@ -0,0 +1,29 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Jeremy Apthorp <nornagon@nornagon.net>
Date: Tue, 5 May 2020 12:36:39 -0700
Subject: breakpad: allow getting string values for crash keys
Linux is currently recording both crashpad and breakpad keys on linux
(because upstream is experimenting with crashpad-on-linux). We can fetch
the string values for crashpad keys on win/mac, and they're easily
available on linux too, this just exposes them.
Should be upstreamed, or failing that, deleted once crashpad is enabled
on linux. If removing this patch doesn't cause a compile failure, it's
fine to delete!
diff --git a/components/crash/core/common/crash_key.h b/components/crash/core/common/crash_key.h
index 9d193ea962e919d4509eef296c900e47059761f4..225b8ce822a42d31d59bc89aab473710384c3010 100644
--- a/components/crash/core/common/crash_key.h
+++ b/components/crash/core/common/crash_key.h
@@ -212,6 +212,10 @@ class CrashKeyStringCombined : public internal::CrashKeyStringCombinedImpl {
crashpad_key_.Set(value);
}
+ const base::StringPiece value() const {
+ return crashpad_key_.value();
+ }
+
private:
CrashKeyStringBreakpad<MaxLength> breakpad_key_;
crashpad::StringAnnotation<MaxLength> crashpad_key_;

View file

@ -0,0 +1,49 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Jeremy Apthorp <nornagon@nornagon.net>
Date: Wed, 29 Apr 2020 16:28:35 -0700
Subject: breakpad: disable upload compression
Our prior implementation of breakpad uploading did not compress files on
upload. In order to maintain that behavior, this disables compression in
//components/crash.
Ideally, this would be made configurable.
diff --git a/components/crash/core/app/breakpad_linux.cc b/components/crash/core/app/breakpad_linux.cc
index 364af690879d79d25d118d5b2fdbaabe21a1c52d..390cca9edc26d3153c8dbc86024cb50097d7ac78 100644
--- a/components/crash/core/app/breakpad_linux.cc
+++ b/components/crash/core/app/breakpad_linux.cc
@@ -1334,6 +1334,7 @@ void ExecUploadProcessOrTerminate(const BreakpadInfo& info,
#else // defined(OS_CHROMEOS)
+ /*
// Compress |dumpfile| with gzip.
const pid_t gzip_child = sys_fork();
if (gzip_child < 0) {
@@ -1377,13 +1378,16 @@ void ExecUploadProcessOrTerminate(const BreakpadInfo& info,
WriteLog(msg, sizeof(msg) - 1);
sys__exit(1);
}
+ */
// The --header argument to wget looks like:
// --header=Content-Encoding: gzip
// --header=Content-Type: multipart/form-data; boundary=XYZ
// where the boundary has two fewer leading '-' chars
+ /*
static const char header_content_encoding[] =
"--header=Content-Encoding: gzip";
+ */
static const char header_msg[] =
"--header=Content-Type: multipart/form-data; boundary=";
const size_t header_content_type_size =
@@ -1412,7 +1416,7 @@ void ExecUploadProcessOrTerminate(const BreakpadInfo& info,
static const char kWgetBinary[] = "/usr/bin/wget";
const char* args[] = {
kWgetBinary,
- header_content_encoding,
+ //header_content_encoding,
header_content_type,
post_file,
upload_url,

View file

@ -0,0 +1,45 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Jeremy Apthorp <nornagon@nornagon.net>
Date: Thu, 30 Apr 2020 17:04:13 -0700
Subject: breakpad: treat node processes as browser processes
On Linux, to avoid the need to pass breakpad FDs to child node processes
spawned by child_process.fork(), each child process must re-initialize
breakpad independently, as a "browser" process. This patches
//components/crash so that it will correctly report 'ptype=node' as a
crash annotation.
diff --git a/components/crash/core/app/breakpad_linux.cc b/components/crash/core/app/breakpad_linux.cc
index 390cca9edc26d3153c8dbc86024cb50097d7ac78..2ae2429e6054f2d54e785c49a49fc243469bb72b 100644
--- a/components/crash/core/app/breakpad_linux.cc
+++ b/components/crash/core/app/breakpad_linux.cc
@@ -722,8 +722,13 @@ bool CrashDone(const MinidumpDescriptor& minidump,
log_path[log_path_len] = '\0';
info.log_filename = log_path;
#endif
- info.process_type = "browser";
- info.process_type_length = 7;
+ if (g_is_node) {
+ info.process_type = "node";
+ info.process_type_length = 4;
+ } else {
+ info.process_type = "browser";
+ info.process_type_length = 7;
+ }
info.distro = base::g_linux_distro;
info.distro_length = my_strlen(base::g_linux_distro);
info.upload = upload;
@@ -2050,8 +2055,13 @@ void InitCrashReporter(const std::string& process_type) {
process_type == kWebViewSingleProcessType ||
process_type == kBrowserProcessType ||
#endif
+ process_type == "node" ||
process_type.empty();
+ if (process_type == "node") {
+ g_is_node = true;
+ }
+
std::string upload_url;
if (GetCrashReporterClient()->GetUploadUrl(&upload_url))
SetUploadURL(upload_url);

View file

@ -0,0 +1,201 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Jeremy Apthorp <jeremya@chromium.org>
Date: Thu, 30 Apr 2020 10:08:06 -0700
Subject: crash: allow setting more options
This allows the client of //components/crash to set upload url,
rate-limiting, compression and global annotations.
This should be upstreamed.
diff --git a/components/crash/core/app/breakpad_linux.cc b/components/crash/core/app/breakpad_linux.cc
index 192b0a7f137f311abb6a991f997e11f5eba52256..364af690879d79d25d118d5b2fdbaabe21a1c52d 100644
--- a/components/crash/core/app/breakpad_linux.cc
+++ b/components/crash/core/app/breakpad_linux.cc
@@ -103,9 +103,18 @@ namespace {
// while we do have functions to deal with uint64_t's.
uint64_t g_crash_loop_before_time = 0;
#else
-const char kUploadURL[] = "https://clients2.google.com/cr/report";
+const char kDefaultUploadURL[] = "https://clients2.google.com/cr/report";
+char* g_upload_url = nullptr;
#endif
+void SetUploadURL(const std::string& url) {
+ const size_t url_len = url.size() + 1;
+ DCHECK(!g_upload_url);
+ g_upload_url = new char[url_len];
+ strncpy(g_upload_url, url.c_str(), url_len);
+}
+
+bool g_is_node = false;
bool g_is_crash_reporter_enabled = false;
uint64_t g_process_start_time = 0;
pid_t g_pid = 0;
@@ -1398,13 +1407,15 @@ void ExecUploadProcessOrTerminate(const BreakpadInfo& info,
char* status_fd_path =
StringFromPrefixAndUint("/dev/fd/", upload_status_fd, allocator);
+ const char* upload_url = g_upload_url ? g_upload_url : kDefaultUploadURL;
+
static const char kWgetBinary[] = "/usr/bin/wget";
const char* args[] = {
kWgetBinary,
header_content_encoding,
header_content_type,
post_file,
- kUploadURL,
+ upload_url,
"--timeout=10", // Set a timeout so we don't hang forever.
"--tries=1", // Don't retry if the upload fails.
"-O", // Output reply to the file descriptor path.
@@ -2037,6 +2048,10 @@ void InitCrashReporter(const std::string& process_type) {
#endif
process_type.empty();
+ std::string upload_url;
+ if (GetCrashReporterClient()->GetUploadUrl(&upload_url))
+ SetUploadURL(upload_url);
+
if (is_browser_process) {
bool enable_breakpad = GetCrashReporterClient()->GetCollectStatsConsent() ||
GetCrashReporterClient()->IsRunningUnattended();
diff --git a/components/crash/core/app/crash_reporter_client.cc b/components/crash/core/app/crash_reporter_client.cc
index e778f68af30f8c071d1360d06b553cc957160c5b..b7f6ca60ef9c2367989d39e1268bf9f9a517951c 100644
--- a/components/crash/core/app/crash_reporter_client.cc
+++ b/components/crash/core/app/crash_reporter_client.cc
@@ -148,6 +148,17 @@ bool CrashReporterClient::ReportingIsEnforcedByPolicy(bool* breakpad_enabled) {
return false;
}
+bool CrashReporterClient::GetShouldRateLimit() {
+ return true;
+}
+
+bool CrashReporterClient::GetShouldCompressUploads() {
+ return true;
+}
+
+void CrashReporterClient::GetProcessSimpleAnnotations(std::map<std::string, std::string>* annotations) {
+}
+
#if defined(OS_ANDROID)
unsigned int CrashReporterClient::GetCrashDumpPercentage() {
return 100;
@@ -196,6 +207,10 @@ void CrashReporterClient::GetSanitizationInformation(
}
#endif
+bool CrashReporterClient::GetUploadUrl(std::string* url) {
+ return false;
+}
+
bool CrashReporterClient::ShouldMonitorCrashHandlerExpensively() {
return false;
}
diff --git a/components/crash/core/app/crash_reporter_client.h b/components/crash/core/app/crash_reporter_client.h
index 9cc78fc2584061d26fd1e83b3ebf5ada0a12c138..aa63d7b95c37e4a143283450798b8bd500dc17a1 100644
--- a/components/crash/core/app/crash_reporter_client.h
+++ b/components/crash/core/app/crash_reporter_client.h
@@ -5,6 +5,7 @@
#ifndef COMPONENTS_CRASH_CORE_APP_CRASH_REPORTER_CLIENT_H_
#define COMPONENTS_CRASH_CORE_APP_CRASH_REPORTER_CLIENT_H_
+#include <map>
#include <string>
#include "base/strings/string16.h"
@@ -153,6 +154,19 @@ class CrashReporterClient {
// that case, |breakpad_enabled| is set to the value enforced by policies.
virtual bool ReportingIsEnforcedByPolicy(bool* breakpad_enabled);
+ // Returns true if crash uploads should be rate limited. If false, no
+ // throttling will be applied for uploads.
+ virtual bool GetShouldRateLimit();
+
+ // Returns true if crash uploads should be compressed with gzip. If false,
+ // reports will be uploaded uncompressed.
+ virtual bool GetShouldCompressUploads();
+
+ // Allows the client to add or edit global annotations passed to the crashpad
+ // handler.
+ virtual void GetProcessSimpleAnnotations(
+ std::map<std::string, std::string>* annotations);
+
#if defined(OS_ANDROID)
// Used by WebView to sample crashes without generating the unwanted dumps. If
// the returned value is less than 100, crash dumping will be sampled to that
@@ -194,6 +208,9 @@ class CrashReporterClient {
bool* sanitize_stacks);
#endif
+ // Override the default upload url. Returns true if overridden.
+ virtual bool GetUploadUrl(std::string* url);
+
// This method should return true to configure a crash reporter capable of
// monitoring itself for its own crashes to do so, even if self-monitoring
// would be expensive. "Expensive" self-monitoring dedicates an additional
diff --git a/components/crash/core/app/crashpad_mac.mm b/components/crash/core/app/crashpad_mac.mm
index b579521d55860823722df2ee849f6b1628b3c950..f4f71e5174cf8fb706a2f8385252ba877d1a03a7 100644
--- a/components/crash/core/app/crashpad_mac.mm
+++ b/components/crash/core/app/crashpad_mac.mm
@@ -67,6 +67,8 @@ std::map<std::string, std::string> GetProcessSimpleAnnotations() {
} // @autoreleasepool
return process_annotations;
}();
+ CrashReporterClient* crash_reporter_client = GetCrashReporterClient();
+ crash_reporter_client->GetProcessSimpleAnnotations(&annotations);
return annotations;
}
@@ -140,9 +142,17 @@ base::FilePath PlatformCrashpadInitialization(
#else
std::string url;
#endif
+ crash_reporter_client->GetUploadUrl(&url);
std::vector<std::string> arguments;
+ if (!crash_reporter_client->GetShouldRateLimit()) {
+ arguments.push_back("--no-rate-limit");
+ }
+ if (!crash_reporter_client->GetShouldCompressUploads()) {
+ arguments.push_back("--no-upload-gzip");
+ }
+
if (crash_reporter_client->ShouldMonitorCrashHandlerExpensively()) {
arguments.push_back("--monitor-self");
}
diff --git a/components/crash/core/app/crashpad_win.cc b/components/crash/core/app/crashpad_win.cc
index 669f5bea844d75f0e5c34b58994f4cfb8e856af0..8c1fb8fb8f2e02466b51ef08de25b056f8b2052d 100644
--- a/components/crash/core/app/crashpad_win.cc
+++ b/components/crash/core/app/crashpad_win.cc
@@ -84,12 +84,14 @@ base::FilePath PlatformCrashpadInitialization(
std::map<std::string, std::string> process_annotations;
GetPlatformCrashpadAnnotations(&process_annotations);
+ crash_reporter_client->GetProcessSimpleAnnotations(&process_annotations);
#if BUILDFLAG(GOOGLE_CHROME_BRANDING)
std::string url = "https://clients2.google.com/cr/report";
#else
std::string url;
#endif
+ crash_reporter_client->GetUploadUrl(&url);
// Allow the crash server to be overridden for testing. If the variable
// isn't present in the environment then the default URL will remain.
@@ -126,6 +128,13 @@ base::FilePath PlatformCrashpadInitialization(
std::vector<std::string> arguments(start_arguments);
+ if (!crash_reporter_client->GetShouldRateLimit()) {
+ arguments.push_back("--no-rate-limit");
+ }
+ if (!crash_reporter_client->GetShouldCompressUploads()) {
+ arguments.push_back("--no-upload-gzip");
+ }
+
if (crash_reporter_client->ShouldMonitorCrashHandlerExpensively()) {
arguments.push_back("--monitor-self");
for (const std::string& start_argument : start_arguments) {

View file

@ -0,0 +1,38 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Jeremy Apthorp <jeremya@chromium.org>
Date: Fri, 1 May 2020 18:25:59 -0700
Subject: upload_list: add LoadSync method
This allows synchronous loading of the upload list, which is required by
the crashReporter.getUploadedReports() API. The synchronous version is
deprecated, and this API should be removed once the deprecated behavior
is no longer supported.
diff --git a/components/upload_list/upload_list.cc b/components/upload_list/upload_list.cc
index e39549c9bb552dc19cad3685629406fdbe44c2c7..428b0f486767b630fc42e0e19c8d63bdb2d04cbc 100644
--- a/components/upload_list/upload_list.cc
+++ b/components/upload_list/upload_list.cc
@@ -71,6 +71,10 @@ void UploadList::Load(base::OnceClosure callback) {
base::BindOnce(&UploadList::OnLoadComplete, this));
}
+void UploadList::LoadSync() {
+ uploads_ = LoadUploadList();
+}
+
void UploadList::Clear(const base::Time& begin,
const base::Time& end,
base::OnceClosure callback) {
diff --git a/components/upload_list/upload_list.h b/components/upload_list/upload_list.h
index 20358339df63ae2bb8b955870c5daf51b65f19f7..7cf89626bea8ee9436f15366446f053a479ac438 100644
--- a/components/upload_list/upload_list.h
+++ b/components/upload_list/upload_list.h
@@ -73,6 +73,8 @@ class UploadList : public base::RefCountedThreadSafe<UploadList> {
// overwrite the previously supplied one, and the first will not be called.
void Load(base::OnceClosure callback);
+ void LoadSync();
+
// Clears any data associated with the upload list, where the upload time or
// capture time falls within the given range.
void Clear(const base::Time& begin,

View file

@ -20,7 +20,6 @@ fix_key_gen_apis_are_not_available_in_boringssl.patch
build_modify_js2c_py_to_allow_injection_of_original-fs_and_custom_embedder_js.patch
refactor_allow_embedder_overriding_of_internal_fs_calls.patch
chore_prevent_warn_non_context-aware_native_modules_being_loaded.patch
inherit_electron_crashpad_pipe_name_in_child_process.patch
chore_read_nobrowserglobals_from_global_not_process.patch
build_bring_back_node_with_ltcg_configuration.patch
revert_crypto_add_oaeplabel_option.patch

View file

@ -1,22 +0,0 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Cheng Zhao <zcbenz@gmail.com>
Date: Tue, 4 Jun 2019 17:42:11 +0900
Subject: Inherit ELECTRON_CRASHPAD_PIPE_NAME in child process
This is required for crashReporter to work correctly in node process.
diff --git a/lib/child_process.js b/lib/child_process.js
index 5ed166e1ed76b830c2d97f8170a4a72841201537..c919527a7c06f87fc5220cb7234368c4686563a3 100644
--- a/lib/child_process.js
+++ b/lib/child_process.js
@@ -115,6 +115,10 @@ function fork(modulePath /* , args, options */) {
options.env = Object.create(options.env || process.env)
options.env.ELECTRON_RUN_AS_NODE = 1;
+ if (process.platform === 'win32') {
+ options.env.ELECTRON_CRASHPAD_PIPE_NAME = process.env.ELECTRON_CRASHPAD_PIPE_NAME;
+ }
+
if (!options.execPath && process.type && process.platform == 'darwin') {
options.execPath = process.helperExecPath;
}