From cc9e95411e84a2ff97f19fc5e1a3135ec1a9acf5 Mon Sep 17 00:00:00 2001 From: Daniel Bevenius Date: Wed, 19 Sep 2018 07:33:28 +0200 Subject: [PATCH 1/2] src: add CheckOptions to Options classes This commit adds a CheckOptions function that the options classes can optionally implement to check that options specified are correct (dependencies between options are met or options that are mutually exclusive). In the process of doing this the error pointer passed to Parse was changed to be of type vector so that potentially multiple options check failures can be reported. --- src/node.cc | 33 ++++++--------------------------- src/node_options-inl.h | 13 +++++++------ src/node_options.cc | 36 +++++++++++++++++++++++++++++++----- src/node_options.h | 20 ++++++++++++++------ 4 files changed, 58 insertions(+), 44 deletions(-) diff --git a/src/node.cc b/src/node.cc index cd5173478170da..ee9c76f08e888a 100644 --- a/src/node.cc +++ b/src/node.cc @@ -2467,7 +2467,7 @@ void ProcessArgv(std::vector* args, bool is_env) { // Parse a few arguments which are specific to Node. std::vector v8_args; - std::string error; + std::vector errors{}; { // TODO(addaleax): The mutex here should ideally be held during the @@ -2479,11 +2479,13 @@ void ProcessArgv(std::vector* args, &v8_args, per_process_opts.get(), is_env ? kAllowedInEnvironment : kDisallowedInEnvironment, - &error); + &errors); } - if (!error.empty()) { - fprintf(stderr, "%s: %s\n", args->at(0).c_str(), error.c_str()); + if (!errors.empty()) { + for (auto const& error : errors) { + fprintf(stderr, "%s: %s\n", args->at(0).c_str(), error.c_str()); + } exit(9); } @@ -2500,30 +2502,7 @@ void ProcessArgv(std::vector* args, for (const std::string& cve : per_process_opts->security_reverts) Revert(cve.c_str()); - // TODO(addaleax): Move this validation to the option parsers. auto env_opts = per_process_opts->per_isolate->per_env; - if (!env_opts->userland_loader.empty() && - !env_opts->experimental_modules) { - fprintf(stderr, "%s: --loader requires --experimental-modules be enabled\n", - args->at(0).c_str()); - exit(9); - } - - if (env_opts->syntax_check_only && env_opts->has_eval_string) { - fprintf(stderr, "%s: either --check or --eval can be used, not both\n", - args->at(0).c_str()); - exit(9); - } - -#if HAVE_OPENSSL - if (per_process_opts->use_openssl_ca && per_process_opts->use_bundled_ca) { - fprintf(stderr, "%s: either --use-openssl-ca or --use-bundled-ca can be " - "used, not both\n", - args->at(0).c_str()); - exit(9); - } -#endif - if (std::find(v8_args.begin(), v8_args.end(), "--abort-on-uncaught-exception") != v8_args.end() || std::find(v8_args.begin(), v8_args.end(), diff --git a/src/node_options-inl.h b/src/node_options-inl.h index ba18e7c19b03c7..1d1eda54751a8f 100644 --- a/src/node_options-inl.h +++ b/src/node_options-inl.h @@ -270,7 +270,7 @@ void OptionsParser::Parse( std::vector* const v8_args, Options* const options, OptionEnvvarSettings required_env_settings, - std::string* const error) { + std::vector* const errors) { ArgsInfo args(orig_args, exec_args); // The first entry is the process name. Make sure it ends up in the V8 argv, @@ -279,7 +279,7 @@ void OptionsParser::Parse( if (v8_args->empty()) v8_args->push_back(args.program_name()); - while (!args.empty() && error->empty()) { + while (!args.empty() && errors->empty()) { if (args.first().size() <= 1 || args.first()[0] != '-') break; // We know that we're either going to consume this @@ -288,7 +288,7 @@ void OptionsParser::Parse( if (arg == "--") { if (required_env_settings == kAllowedInEnvironment) - *error = NotAllowedInEnvErr("--"); + errors->push_back(NotAllowedInEnvErr("--")); break; } @@ -357,7 +357,7 @@ void OptionsParser::Parse( if ((it == options_.end() || it->second.env_setting == kDisallowedInEnvironment) && required_env_settings == kAllowedInEnvironment) { - *error = NotAllowedInEnvErr(original_name); + errors->push_back(NotAllowedInEnvErr(original_name)); break; } @@ -379,7 +379,7 @@ void OptionsParser::Parse( value = arg.substr(equals_index + 1); if (value.empty()) { missing_argument: - *error = RequiresArgumentErr(original_name); + errors->push_back(RequiresArgumentErr(original_name)); break; } } else { @@ -413,7 +413,7 @@ void OptionsParser::Parse( break; case kHostPort: Lookup(info.field, options) - ->Update(SplitHostPort(value, error)); + ->Update(SplitHostPort(value, errors)); break; case kNoOp: break; @@ -424,6 +424,7 @@ void OptionsParser::Parse( UNREACHABLE(); } } + options->CheckOptions(errors); } } // namespace options_parser diff --git a/src/node_options.cc b/src/node_options.cc index 8027ee677c1e12..6c423c7bfea988 100644 --- a/src/node_options.cc +++ b/src/node_options.cc @@ -16,6 +16,30 @@ using v8::Undefined; using v8::Value; namespace node { + +void PerProcessOptions::CheckOptions(std::vector* errors) { + // Check any per-process options here. +#if HAVE_OPENSSL + if (use_openssl_ca && use_bundled_ca) { + errors->push_back("either --use-openssl-ca or --use-bundled-ca can be " + "used, not both"); + } +#endif + per_isolate->CheckOptions(errors); + per_isolate->per_env->CheckOptions(errors); + per_isolate->per_env->debug_options->CheckOptions(errors); +} + +void EnvironmentOptions::CheckOptions(std::vector* errors) { + if (!userland_loader.empty() && !experimental_modules) { + errors->push_back("--loader requires --experimental-modules be enabled"); + } + + if (syntax_check_only && has_eval_string) { + errors->push_back("either --check or --eval can be used, not both"); + } +} + namespace options_parser { // XXX: If you add an option here, please also add it to doc/node.1 and @@ -302,18 +326,20 @@ inline std::string RemoveBrackets(const std::string& host) { return host; } -inline int ParseAndValidatePort(const std::string& port, std::string* error) { +inline int ParseAndValidatePort(const std::string& port, + std::vector* errors) { char* endptr; errno = 0; const long result = strtol(port.c_str(), &endptr, 10); // NOLINT(runtime/int) if (errno != 0 || *endptr != '\0'|| (result != 0 && result < 1024) || result > 65535) { - *error = "Port must be 0 or in range 1024 to 65535."; + errors->push_back(" must be 0 or in range 1024 to 65535."); } return static_cast(result); } -HostPort SplitHostPort(const std::string& arg, std::string* error) { +HostPort SplitHostPort(const std::string& arg, + std::vector* errors) { // remove_brackets only works if no port is specified // so if it has an effect only an IPv6 address was specified. std::string host = RemoveBrackets(arg); @@ -329,11 +355,11 @@ HostPort SplitHostPort(const std::string& arg, std::string* error) { return HostPort { arg, -1 }; } } - return HostPort { "", ParseAndValidatePort(arg, error) }; + return HostPort { "", ParseAndValidatePort(arg, errors) }; } // Host and port found: return HostPort { RemoveBrackets(arg.substr(0, colon)), - ParseAndValidatePort(arg.substr(colon + 1), error) }; + ParseAndValidatePort(arg.substr(colon + 1), errors) }; } // Usage: Either: diff --git a/src/node_options.h b/src/node_options.h index 71615cf0839aa4..dfa4f603d1e951 100644 --- a/src/node_options.h +++ b/src/node_options.h @@ -21,11 +21,16 @@ struct HostPort { } }; +class Options { + public: + virtual void CheckOptions(std::vector* errors) {} +}; + // These options are currently essentially per-Environment, but it can be nice // to keep them separate since they are a group of options applying to a very // specific part of Node. It might also make more sense for them to be // per-Isolate, rather than per-Environment. -class DebugOptions { +class DebugOptions : public Options { public: bool inspector_enabled = false; bool deprecated_debug = false; @@ -57,7 +62,7 @@ class DebugOptions { } }; -class EnvironmentOptions { +class EnvironmentOptions : public Options { public: std::shared_ptr debug_options { new DebugOptions() }; bool abort_on_uncaught_exception = false; @@ -91,9 +96,10 @@ class EnvironmentOptions { std::vector user_argv; inline DebugOptions* get_debug_options(); + void CheckOptions(std::vector* errors); }; -class PerIsolateOptions { +class PerIsolateOptions : public Options { public: std::shared_ptr per_env { new EnvironmentOptions() }; bool track_heap_objects = false; @@ -101,7 +107,7 @@ class PerIsolateOptions { inline EnvironmentOptions* get_per_env_options(); }; -class PerProcessOptions { +class PerProcessOptions : public Options { public: std::shared_ptr per_isolate { new PerIsolateOptions() }; @@ -138,13 +144,15 @@ class PerProcessOptions { #endif inline PerIsolateOptions* get_per_isolate_options(); + void CheckOptions(std::vector* errors); }; // The actual options parser, as opposed to the structs containing them: namespace options_parser { -HostPort SplitHostPort(const std::string& arg, std::string* error); +HostPort SplitHostPort(const std::string& arg, + std::vector* errors); void GetOptions(const v8::FunctionCallbackInfo& args); enum OptionEnvvarSettings { @@ -254,7 +262,7 @@ class OptionsParser { std::vector* const v8_args, Options* const options, OptionEnvvarSettings required_env_settings, - std::string* const error); + std::vector* const errors); private: // We support the wide variety of different option types by remembering From 0c61a6d9e4f71403e943ea7ab8519385acf88cf6 Mon Sep 17 00:00:00 2001 From: Daniel Bevenius Date: Thu, 20 Sep 2018 07:35:06 +0200 Subject: [PATCH 2/2] squash: make CheckOptions delegate --- src/node_options.cc | 8 +++++--- src/node_options.h | 1 + 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/node_options.cc b/src/node_options.cc index 6c423c7bfea988..eaedb8b0786a88 100644 --- a/src/node_options.cc +++ b/src/node_options.cc @@ -18,7 +18,6 @@ using v8::Value; namespace node { void PerProcessOptions::CheckOptions(std::vector* errors) { - // Check any per-process options here. #if HAVE_OPENSSL if (use_openssl_ca && use_bundled_ca) { errors->push_back("either --use-openssl-ca or --use-bundled-ca can be " @@ -26,8 +25,10 @@ void PerProcessOptions::CheckOptions(std::vector* errors) { } #endif per_isolate->CheckOptions(errors); - per_isolate->per_env->CheckOptions(errors); - per_isolate->per_env->debug_options->CheckOptions(errors); +} + +void PerIsolateOptions::CheckOptions(std::vector* errors) { + per_env->CheckOptions(errors); } void EnvironmentOptions::CheckOptions(std::vector* errors) { @@ -38,6 +39,7 @@ void EnvironmentOptions::CheckOptions(std::vector* errors) { if (syntax_check_only && has_eval_string) { errors->push_back("either --check or --eval can be used, not both"); } + debug_options->CheckOptions(errors); } namespace options_parser { diff --git a/src/node_options.h b/src/node_options.h index dfa4f603d1e951..b7e067ba17f291 100644 --- a/src/node_options.h +++ b/src/node_options.h @@ -105,6 +105,7 @@ class PerIsolateOptions : public Options { bool track_heap_objects = false; inline EnvironmentOptions* get_per_env_options(); + void CheckOptions(std::vector* errors); }; class PerProcessOptions : public Options {