From b6cd18517c55f9209f632ecce1a9782e4d3217dd Mon Sep 17 00:00:00 2001 From: Daniel Bevenius Date: Wed, 19 Sep 2018 07:33:28 +0200 Subject: [PATCH] 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. PR-URL: https://github.com/nodejs/node/pull/22943 Reviewed-By: James M Snell Reviewed-By: Anna Henningsen Reviewed-By: Joyee Cheung --- src/node.cc | 33 ++++++--------------------------- src/node_options-inl.h | 13 +++++++------ src/node_options.cc | 38 +++++++++++++++++++++++++++++++++----- src/node_options.h | 21 +++++++++++++++------ 4 files changed, 61 insertions(+), 44 deletions(-) diff --git a/src/node.cc b/src/node.cc index 1d4db4a2cbb438..1f23da8c5eed38 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 06cce7cf77ccba..b14d18090b734a 100644 --- a/src/node_options.cc +++ b/src/node_options.cc @@ -16,6 +16,32 @@ using v8::Undefined; using v8::Value; namespace node { + +void PerProcessOptions::CheckOptions(std::vector* errors) { +#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); +} + +void PerIsolateOptions::CheckOptions(std::vector* errors) { + per_env->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"); + } + debug_options->CheckOptions(errors); +} + namespace options_parser { // XXX: If you add an option here, please also add it to doc/node.1 and @@ -307,18 +333,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); @@ -334,11 +362,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 4b9ee000367405..506db25dec2efa 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,17 +96,19 @@ 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; inline EnvironmentOptions* get_per_env_options(); + void CheckOptions(std::vector* errors); }; -class PerProcessOptions { +class PerProcessOptions : public Options { public: std::shared_ptr per_isolate { new PerIsolateOptions() }; @@ -139,13 +146,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 { @@ -255,7 +264,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