refactor: use compile-time cli arg sets. (#38748)

We're currently building these on the heap with `std::set<std::string>`
but this can be a very small compile-time container instead.

Marking as 'refactor' rather than 'perf' since this isn't called often,
but moving from heap to compile-time is good and using this container
makes the code more readable.
This commit is contained in:
Charles Kerr 2023-06-13 14:37:59 -05:00 committed by GitHub
parent 1621fcf5d3
commit 167c2b250e
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
2 changed files with 39 additions and 33 deletions

View file

@ -7,12 +7,12 @@
#include <map> #include <map>
#include <memory> #include <memory>
#include <string> #include <string>
#include <unordered_set>
#include <utility> #include <utility>
#include <vector> #include <vector>
#include "base/base_switches.h" #include "base/base_switches.h"
#include "base/command_line.h" #include "base/command_line.h"
#include "base/containers/fixed_flat_set.h"
#include "base/feature_list.h" #include "base/feature_list.h"
#include "base/strings/string_util.h" #include "base/strings/string_util.h"
#include "base/strings/utf_string_conversions.h" #include "base/strings/utf_string_conversions.h"
@ -54,9 +54,13 @@ namespace {
// See https://nodejs.org/api/cli.html#cli_options // See https://nodejs.org/api/cli.html#cli_options
int SetNodeCliFlags() { int SetNodeCliFlags() {
// Options that are unilaterally disallowed // Options that are unilaterally disallowed
const std::unordered_set<base::StringPiece, base::StringPieceHash> static constexpr auto disallowed = base::MakeFixedFlatSet<base::StringPiece>({
disallowed = {"--openssl-config", "--use-bundled-ca", "--use-openssl-ca", "--enable-fips",
"--force-fips", "--enable-fips"}; "--force-fips",
"--openssl-config",
"--use-bundled-ca",
"--use-openssl-ca",
});
const auto argv = base::CommandLine::ForCurrentProcess()->argv(); const auto argv = base::CommandLine::ForCurrentProcess()->argv();
std::vector<std::string> args; std::vector<std::string> args;
@ -74,7 +78,7 @@ int SetNodeCliFlags() {
const auto& option = arg; const auto& option = arg;
#endif #endif
const auto stripped = base::StringPiece(option).substr(0, option.find('=')); const auto stripped = base::StringPiece(option).substr(0, option.find('='));
if (disallowed.count(stripped) != 0) { if (disallowed.contains(stripped)) {
LOG(ERROR) << "The Node.js cli flag " << stripped LOG(ERROR) << "The Node.js cli flag " << stripped
<< " is not supported in Electron"; << " is not supported in Electron";
// Node.js returns 9 from ProcessGlobalArgs for any errors encountered // Node.js returns 9 from ProcessGlobalArgs for any errors encountered

View file

@ -6,14 +6,13 @@
#include <algorithm> #include <algorithm>
#include <memory> #include <memory>
#include <set>
#include <string> #include <string>
#include <unordered_set>
#include <utility> #include <utility>
#include <vector> #include <vector>
#include "base/base_paths.h" #include "base/base_paths.h"
#include "base/command_line.h" #include "base/command_line.h"
#include "base/containers/fixed_flat_set.h"
#include "base/environment.h" #include "base/environment.h"
#include "base/path_service.h" #include "base/path_service.h"
#include "base/run_loop.h" #include "base/run_loop.h"
@ -231,32 +230,39 @@ void ErrorMessageListener(v8::Local<v8::Message> message,
} }
} }
const std::unordered_set<base::StringPiece, base::StringPieceHash> // Only allow DebugOptions in non-ELECTRON_RUN_AS_NODE mode.
GetAllowedDebugOptions() { // If node CLI inspect support is disabled, allow no debug options.
if (electron::fuses::IsNodeCliInspectEnabled()) { bool IsAllowedDebugOption(base::StringPiece option) {
// Only allow DebugOptions in non-ELECTRON_RUN_AS_NODE mode static constexpr auto options = base::MakeFixedFlatSet<base::StringPiece>({
return { "--debug",
"--inspect", "--inspect-brk", "--debug-brk",
"--inspect-port", "--debug", "--debug-port",
"--debug-brk", "--debug-port", "--inspect",
"--inspect-brk-node", "--inspect-publish-uid", "--inspect-brk",
}; "--inspect-brk-node",
} "--inspect-port",
// If node CLI inspect support is disabled, allow no debug options. "--inspect-publish-uid",
return {}; });
return electron::fuses::IsNodeCliInspectEnabled() && options.contains(option);
} }
// Initialize NODE_OPTIONS to pass to Node.js // Initialize NODE_OPTIONS to pass to Node.js
// See https://nodejs.org/api/cli.html#cli_node_options_options // See https://nodejs.org/api/cli.html#cli_node_options_options
void SetNodeOptions(base::Environment* env) { void SetNodeOptions(base::Environment* env) {
// Options that are unilaterally disallowed // Options that are unilaterally disallowed
const std::set<std::string> disallowed = { static constexpr auto disallowed = base::MakeFixedFlatSet<base::StringPiece>({
"--openssl-config", "--use-bundled-ca", "--use-openssl-ca", "--enable-fips",
"--force-fips", "--enable-fips"}; "--force-fips",
"--openssl-config",
"--use-bundled-ca",
"--use-openssl-ca",
});
// Subset of options allowed in packaged apps static constexpr auto pkg_opts = base::MakeFixedFlatSet<base::StringPiece>({
const std::set<std::string> allowed_in_packaged = {"--max-http-header-size", "--http-parser",
"--http-parser"}; "--max-http-header-size",
});
if (env->HasVar("NODE_OPTIONS")) { if (env->HasVar("NODE_OPTIONS")) {
if (electron::fuses::IsNodeOptionsEnabled()) { if (electron::fuses::IsNodeOptionsEnabled()) {
@ -271,13 +277,12 @@ void SetNodeOptions(base::Environment* env) {
// Strip off values passed to individual NODE_OPTIONs // Strip off values passed to individual NODE_OPTIONs
std::string option = part.substr(0, part.find('=')); std::string option = part.substr(0, part.find('='));
if (is_packaged_app && if (is_packaged_app && !pkg_opts.contains(option)) {
allowed_in_packaged.find(option) == allowed_in_packaged.end()) {
// Explicitly disallow majority of NODE_OPTIONS in packaged apps // Explicitly disallow majority of NODE_OPTIONS in packaged apps
LOG(ERROR) << "Most NODE_OPTIONs are not supported in packaged apps." LOG(ERROR) << "Most NODE_OPTIONs are not supported in packaged apps."
<< " See documentation for more details."; << " See documentation for more details.";
options.erase(options.find(option), part.length()); options.erase(options.find(option), part.length());
} else if (disallowed.find(option) != disallowed.end()) { } else if (disallowed.contains(option)) {
// Remove NODE_OPTIONS specifically disallowed for use in Node.js // Remove NODE_OPTIONS specifically disallowed for use in Node.js
// through Electron owing to constraints like BoringSSL. // through Electron owing to constraints like BoringSSL.
LOG(ERROR) << "The NODE_OPTION " << option LOG(ERROR) << "The NODE_OPTION " << option
@ -375,9 +380,6 @@ bool NodeBindings::IsInitialized() {
// Initialize Node.js cli options to pass to Node.js // Initialize Node.js cli options to pass to Node.js
// See https://nodejs.org/api/cli.html#cli_options // See https://nodejs.org/api/cli.html#cli_options
void NodeBindings::SetNodeCliFlags() { void NodeBindings::SetNodeCliFlags() {
const std::unordered_set<base::StringPiece, base::StringPieceHash> allowed =
GetAllowedDebugOptions();
const auto argv = base::CommandLine::ForCurrentProcess()->argv(); const auto argv = base::CommandLine::ForCurrentProcess()->argv();
std::vector<std::string> args; std::vector<std::string> args;
@ -396,7 +398,7 @@ void NodeBindings::SetNodeCliFlags() {
const auto stripped = base::StringPiece(option).substr(0, option.find('=')); const auto stripped = base::StringPiece(option).substr(0, option.find('='));
// Only allow in no-op (--) option or DebugOptions // Only allow in no-op (--) option or DebugOptions
if (allowed.count(stripped) != 0 || stripped == "--") if (IsAllowedDebugOption(stripped) || stripped == "--")
args.push_back(option); args.push_back(option);
} }