Skip to content

Commit

Permalink
src: pass along errors from --security-reverts
Browse files Browse the repository at this point in the history
Pass along errors from `Revert()` when a security revert
is unknown (which currently applies to all possible values).

Previously, we would unconditionally call `exit()`, which is
not nice for embedding use cases, and could crash because we
were holding a lock for a mutex in `ProcessGlobalArgs()` that
would be destroyed by calling `exit()`.

Also, add a regression test that makes sure that the process
exits with the right exit code and not a crash.

PR-URL: nodejs#25466
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
addaleax committed Jan 22, 2019
1 parent 8c9aaac commit 38ab1e9
Show file tree
Hide file tree
Showing 3 changed files with 26 additions and 5 deletions.
10 changes: 8 additions & 2 deletions src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -915,8 +915,14 @@ int ProcessGlobalArgs(std::vector<std::string>* args,

if (!errors->empty()) return 9;

for (const std::string& cve : per_process::cli_options->security_reverts)
Revert(cve.c_str());
std::string revert_error;
for (const std::string& cve : per_process::cli_options->security_reverts) {
Revert(cve.c_str(), &revert_error);
if (!revert_error.empty()) {
errors->emplace_back(std::move(revert_error));
return 12;
}
}

auto env_opts = per_process::cli_options->per_isolate->per_env;
if (std::find(v8_args.begin(), v8_args.end(),
Expand Down
7 changes: 4 additions & 3 deletions src/node_revert.h
Original file line number Diff line number Diff line change
Expand Up @@ -43,13 +43,14 @@ inline void Revert(const reversion cve) {
printf("SECURITY WARNING: Reverting %s\n", RevertMessage(cve));
}

inline void Revert(const char* cve) {
inline void Revert(const char* cve, std::string* error) {
#define V(code, label, _) \
if (strcmp(cve, label) == 0) return Revert(SECURITY_REVERT_##code);
SECURITY_REVERSIONS(V)
#undef V
printf("Error: Attempt to revert an unknown CVE [%s]\n", cve);
exit(12);
*error = "Error: Attempt to revert an unknown CVE [";
*error += cve;
*error += ']';
}

inline bool IsReverted(const reversion cve) {
Expand Down
14 changes: 14 additions & 0 deletions test/parallel/test-security-revert-unknown.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
'use strict';
require('../common');
const assert = require('assert');
const { spawnSync } = require('child_process');
const os = require('os');

const { signal, status, output } =
spawnSync(process.execPath, ['--security-reverts=not-a-cve']);
assert.strictEqual(signal, null);
assert.strictEqual(status, 12);
assert.strictEqual(
output[2].toString(),
`${process.execPath}: Error: ` +
`Attempt to revert an unknown CVE [not-a-cve]${os.EOL}`);

0 comments on commit 38ab1e9

Please sign in to comment.