From 167c2b250e39070da360c34498c2dc0d292ac671 Mon Sep 17 00:00:00 2001 From: Charles Kerr Date: Tue, 13 Jun 2023 14:37:59 -0500 Subject: [PATCH] refactor: use compile-time cli arg sets. (#38748) We're currently building these on the heap with `std::set` 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. --- shell/app/node_main.cc | 14 ++++++--- shell/common/node_bindings.cc | 58 ++++++++++++++++++----------------- 2 files changed, 39 insertions(+), 33 deletions(-) diff --git a/shell/app/node_main.cc b/shell/app/node_main.cc index 2369bd5a859c..8d06bef9a722 100644 --- a/shell/app/node_main.cc +++ b/shell/app/node_main.cc @@ -7,12 +7,12 @@ #include #include #include -#include #include #include #include "base/base_switches.h" #include "base/command_line.h" +#include "base/containers/fixed_flat_set.h" #include "base/feature_list.h" #include "base/strings/string_util.h" #include "base/strings/utf_string_conversions.h" @@ -54,9 +54,13 @@ namespace { // See https://nodejs.org/api/cli.html#cli_options int SetNodeCliFlags() { // Options that are unilaterally disallowed - const std::unordered_set - disallowed = {"--openssl-config", "--use-bundled-ca", "--use-openssl-ca", - "--force-fips", "--enable-fips"}; + static constexpr auto disallowed = base::MakeFixedFlatSet({ + "--enable-fips", + "--force-fips", + "--openssl-config", + "--use-bundled-ca", + "--use-openssl-ca", + }); const auto argv = base::CommandLine::ForCurrentProcess()->argv(); std::vector args; @@ -74,7 +78,7 @@ int SetNodeCliFlags() { const auto& option = arg; #endif 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 << " is not supported in Electron"; // Node.js returns 9 from ProcessGlobalArgs for any errors encountered diff --git a/shell/common/node_bindings.cc b/shell/common/node_bindings.cc index 36a06d1ed454..45f2e19107de 100644 --- a/shell/common/node_bindings.cc +++ b/shell/common/node_bindings.cc @@ -6,14 +6,13 @@ #include #include -#include #include -#include #include #include #include "base/base_paths.h" #include "base/command_line.h" +#include "base/containers/fixed_flat_set.h" #include "base/environment.h" #include "base/path_service.h" #include "base/run_loop.h" @@ -231,32 +230,39 @@ void ErrorMessageListener(v8::Local message, } } -const std::unordered_set -GetAllowedDebugOptions() { - if (electron::fuses::IsNodeCliInspectEnabled()) { - // Only allow DebugOptions in non-ELECTRON_RUN_AS_NODE mode - return { - "--inspect", "--inspect-brk", - "--inspect-port", "--debug", - "--debug-brk", "--debug-port", - "--inspect-brk-node", "--inspect-publish-uid", - }; - } - // If node CLI inspect support is disabled, allow no debug options. - return {}; +// Only allow DebugOptions in non-ELECTRON_RUN_AS_NODE mode. +// If node CLI inspect support is disabled, allow no debug options. +bool IsAllowedDebugOption(base::StringPiece option) { + static constexpr auto options = base::MakeFixedFlatSet({ + "--debug", + "--debug-brk", + "--debug-port", + "--inspect", + "--inspect-brk", + "--inspect-brk-node", + "--inspect-port", + "--inspect-publish-uid", + }); + + return electron::fuses::IsNodeCliInspectEnabled() && options.contains(option); } // Initialize NODE_OPTIONS to pass to Node.js // See https://nodejs.org/api/cli.html#cli_node_options_options void SetNodeOptions(base::Environment* env) { // Options that are unilaterally disallowed - const std::set disallowed = { - "--openssl-config", "--use-bundled-ca", "--use-openssl-ca", - "--force-fips", "--enable-fips"}; + static constexpr auto disallowed = base::MakeFixedFlatSet({ + "--enable-fips", + "--force-fips", + "--openssl-config", + "--use-bundled-ca", + "--use-openssl-ca", + }); - // Subset of options allowed in packaged apps - const std::set allowed_in_packaged = {"--max-http-header-size", - "--http-parser"}; + static constexpr auto pkg_opts = base::MakeFixedFlatSet({ + "--http-parser", + "--max-http-header-size", + }); if (env->HasVar("NODE_OPTIONS")) { if (electron::fuses::IsNodeOptionsEnabled()) { @@ -271,13 +277,12 @@ void SetNodeOptions(base::Environment* env) { // Strip off values passed to individual NODE_OPTIONs std::string option = part.substr(0, part.find('=')); - if (is_packaged_app && - allowed_in_packaged.find(option) == allowed_in_packaged.end()) { + if (is_packaged_app && !pkg_opts.contains(option)) { // Explicitly disallow majority of NODE_OPTIONS in packaged apps LOG(ERROR) << "Most NODE_OPTIONs are not supported in packaged apps." << " See documentation for more details."; 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 // through Electron owing to constraints like BoringSSL. LOG(ERROR) << "The NODE_OPTION " << option @@ -375,9 +380,6 @@ bool NodeBindings::IsInitialized() { // Initialize Node.js cli options to pass to Node.js // See https://nodejs.org/api/cli.html#cli_options void NodeBindings::SetNodeCliFlags() { - const std::unordered_set allowed = - GetAllowedDebugOptions(); - const auto argv = base::CommandLine::ForCurrentProcess()->argv(); std::vector args; @@ -396,7 +398,7 @@ void NodeBindings::SetNodeCliFlags() { const auto stripped = base::StringPiece(option).substr(0, option.find('=')); // Only allow in no-op (--) option or DebugOptions - if (allowed.count(stripped) != 0 || stripped == "--") + if (IsAllowedDebugOption(stripped) || stripped == "--") args.push_back(option); }