Skip to content

Commit

Permalink
src: add CheckOptions to Options classes
Browse files Browse the repository at this point in the history
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: #22943
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
  • Loading branch information
danbev authored and targos committed Sep 23, 2018
1 parent 9c36827 commit b6cd185
Show file tree
Hide file tree
Showing 4 changed files with 61 additions and 44 deletions.
33 changes: 6 additions & 27 deletions src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2467,7 +2467,7 @@ void ProcessArgv(std::vector<std::string>* args,
bool is_env) {
// Parse a few arguments which are specific to Node.
std::vector<std::string> v8_args;
std::string error;
std::vector<std::string> errors{};

{
// TODO(addaleax): The mutex here should ideally be held during the
Expand All @@ -2479,11 +2479,13 @@ void ProcessArgv(std::vector<std::string>* 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);
}

Expand All @@ -2500,30 +2502,7 @@ void ProcessArgv(std::vector<std::string>* 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(),
Expand Down
13 changes: 7 additions & 6 deletions src/node_options-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,7 @@ void OptionsParser<Options>::Parse(
std::vector<std::string>* const v8_args,
Options* const options,
OptionEnvvarSettings required_env_settings,
std::string* const error) {
std::vector<std::string>* const errors) {
ArgsInfo args(orig_args, exec_args);

// The first entry is the process name. Make sure it ends up in the V8 argv,
Expand All @@ -279,7 +279,7 @@ void OptionsParser<Options>::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
Expand All @@ -288,7 +288,7 @@ void OptionsParser<Options>::Parse(

if (arg == "--") {
if (required_env_settings == kAllowedInEnvironment)
*error = NotAllowedInEnvErr("--");
errors->push_back(NotAllowedInEnvErr("--"));
break;
}

Expand Down Expand Up @@ -357,7 +357,7 @@ void OptionsParser<Options>::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;
}

Expand All @@ -379,7 +379,7 @@ void OptionsParser<Options>::Parse(
value = arg.substr(equals_index + 1);
if (value.empty()) {
missing_argument:
*error = RequiresArgumentErr(original_name);
errors->push_back(RequiresArgumentErr(original_name));
break;
}
} else {
Expand Down Expand Up @@ -413,7 +413,7 @@ void OptionsParser<Options>::Parse(
break;
case kHostPort:
Lookup<HostPort>(info.field, options)
->Update(SplitHostPort(value, error));
->Update(SplitHostPort(value, errors));
break;
case kNoOp:
break;
Expand All @@ -424,6 +424,7 @@ void OptionsParser<Options>::Parse(
UNREACHABLE();
}
}
options->CheckOptions(errors);
}

} // namespace options_parser
Expand Down
38 changes: 33 additions & 5 deletions src/node_options.cc
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,32 @@ using v8::Undefined;
using v8::Value;

namespace node {

void PerProcessOptions::CheckOptions(std::vector<std::string>* 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<std::string>* errors) {
per_env->CheckOptions(errors);
}

void EnvironmentOptions::CheckOptions(std::vector<std::string>* 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
Expand Down Expand Up @@ -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<std::string>* 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<int>(result);
}

HostPort SplitHostPort(const std::string& arg, std::string* error) {
HostPort SplitHostPort(const std::string& arg,
std::vector<std::string>* 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);
Expand All @@ -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:
Expand Down
21 changes: 15 additions & 6 deletions src/node_options.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,16 @@ struct HostPort {
}
};

class Options {
public:
virtual void CheckOptions(std::vector<std::string>* 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;
Expand Down Expand Up @@ -57,7 +62,7 @@ class DebugOptions {
}
};

class EnvironmentOptions {
class EnvironmentOptions : public Options {
public:
std::shared_ptr<DebugOptions> debug_options { new DebugOptions() };
bool abort_on_uncaught_exception = false;
Expand Down Expand Up @@ -91,17 +96,19 @@ class EnvironmentOptions {
std::vector<std::string> user_argv;

inline DebugOptions* get_debug_options();
void CheckOptions(std::vector<std::string>* errors);
};

class PerIsolateOptions {
class PerIsolateOptions : public Options {
public:
std::shared_ptr<EnvironmentOptions> per_env { new EnvironmentOptions() };
bool track_heap_objects = false;

inline EnvironmentOptions* get_per_env_options();
void CheckOptions(std::vector<std::string>* errors);
};

class PerProcessOptions {
class PerProcessOptions : public Options {
public:
std::shared_ptr<PerIsolateOptions> per_isolate { new PerIsolateOptions() };

Expand Down Expand Up @@ -139,13 +146,15 @@ class PerProcessOptions {
#endif

inline PerIsolateOptions* get_per_isolate_options();
void CheckOptions(std::vector<std::string>* 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<std::string>* errors);
void GetOptions(const v8::FunctionCallbackInfo<v8::Value>& args);

enum OptionEnvvarSettings {
Expand Down Expand Up @@ -255,7 +264,7 @@ class OptionsParser {
std::vector<std::string>* const v8_args,
Options* const options,
OptionEnvvarSettings required_env_settings,
std::string* const error);
std::vector<std::string>* const errors);

private:
// We support the wide variety of different option types by remembering
Expand Down

0 comments on commit b6cd185

Please sign in to comment.