From 6ca36400ba06476c20f17655e2a0f69b33540243 Mon Sep 17 00:00:00 2001 From: Lance Ball Date: Mon, 2 Oct 2017 11:33:25 -0400 Subject: [PATCH 001/241] repl: refactor `LineParser` implementation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move the core logic from `LineParser` should fail handling into the recoverable error check for the REPL default eval. Fixes: https://github.com/nodejs/node/issues/15704 Backport-PR-URL: https://github.com/nodejs/node/pull/15773 PR-URL: https://github.com/nodejs/node/pull/6171 Reviewed-By: James M Snell Reviewed-By: Michaël Zasso --- lib/repl.js | 286 +++++++----------- .../test-repl-multi-line-templates.js | 25 ++ test/parallel/test-repl.js | 15 +- 3 files changed, 142 insertions(+), 184 deletions(-) create mode 100644 test/parallel/test-repl-multi-line-templates.js diff --git a/lib/repl.js b/lib/repl.js index f841b3e3eff34b..4e8555d9f0af7b 100644 --- a/lib/repl.js +++ b/lib/repl.js @@ -77,110 +77,6 @@ exports.writer = util.inspect; exports._builtinLibs = internalModule.builtinLibs; - -const BLOCK_SCOPED_ERROR = 'Block-scoped declarations (let, const, function, ' + - 'class) not yet supported outside strict mode'; - - -class LineParser { - - constructor() { - this.reset(); - } - - reset() { - this._literal = null; - this.shouldFail = false; - this.blockComment = false; - this.regExpLiteral = false; - this.prevTokenChar = null; - } - - parseLine(line) { - var previous = null; - this.shouldFail = false; - const wasWithinStrLiteral = this._literal !== null; - - for (const current of line) { - if (previous === '\\') { - // valid escaping, skip processing. previous doesn't matter anymore - previous = null; - continue; - } - - if (!this._literal) { - if (this.regExpLiteral && current === '/') { - this.regExpLiteral = false; - previous = null; - continue; - } - if (previous === '*' && current === '/') { - if (this.blockComment) { - this.blockComment = false; - previous = null; - continue; - } else { - this.shouldFail = true; - break; - } - } - - // ignore rest of the line if `current` and `previous` are `/`s - if (previous === current && previous === '/' && !this.blockComment) { - break; - } - - if (previous === '/') { - if (current === '*') { - this.blockComment = true; - } else if ( - // Distinguish between a division operator and the start of a regex - // by examining the non-whitespace character that precedes the / - [null, '(', '[', '{', '}', ';'].includes(this.prevTokenChar) - ) { - this.regExpLiteral = true; - } - previous = null; - } - } - - if (this.blockComment || this.regExpLiteral) continue; - - if (current === this._literal) { - this._literal = null; - } else if (current === '\'' || current === '"') { - this._literal = this._literal || current; - } - - if (current.trim() && current !== '/') this.prevTokenChar = current; - - previous = current; - } - - const isWithinStrLiteral = this._literal !== null; - - if (!wasWithinStrLiteral && !isWithinStrLiteral) { - // Current line has nothing to do with String literals, trim both ends - line = line.trim(); - } else if (wasWithinStrLiteral && !isWithinStrLiteral) { - // was part of a string literal, but it is over now, trim only the end - line = line.trimRight(); - } else if (isWithinStrLiteral && !wasWithinStrLiteral) { - // was not part of a string literal, but it is now, trim only the start - line = line.trimLeft(); - } - - const lastChar = line.charAt(line.length - 1); - - this.shouldFail = this.shouldFail || - ((!this._literal && lastChar === '\\') || - (this._literal && lastChar !== '\\')); - - return line; - } -} - - function REPLServer(prompt, stream, eval_, @@ -232,8 +128,6 @@ function REPLServer(prompt, self.breakEvalOnSigint = !!breakEvalOnSigint; self.editorMode = false; - self._inTemplateLiteral = false; - // just for backwards compat, see github.com/joyent/node/pull/7127 self.rli = this; @@ -245,69 +139,48 @@ function REPLServer(prompt, eval_ = eval_ || defaultEval; - function preprocess(code) { - let cmd = code; - if (/^\s*\{/.test(cmd) && /\}\s*$/.test(cmd)) { + function defaultEval(code, context, file, cb) { + var err, result, script, wrappedErr; + var wrappedCmd = false; + var input = code; + + if (/^\s*\{/.test(code) && /\}\s*$/.test(code)) { // It's confusing for `{ a : 1 }` to be interpreted as a block // statement rather than an object literal. So, we first try // to wrap it in parentheses, so that it will be interpreted as // an expression. - cmd = `(${cmd})`; - self.wrappedCmd = true; - } else { - // Mitigate https://github.com/nodejs/node/issues/548 - cmd = cmd.replace( - /^\s*function(?:\s*(\*)\s*|\s+)([^(]+)/, - (_, genStar, name) => `var ${name} = function ${genStar || ''}${name}` - ); + code = `(${code.trim()})\n`; + wrappedCmd = true; } - // Append a \n so that it will be either - // terminated, or continued onto the next expression if it's an - // unexpected end of input. - return `${cmd}\n`; - } - function defaultEval(code, context, file, cb) { - // Remove trailing new line - code = code.replace(/\n$/, ''); - code = preprocess(code); - - var err, result, retry = false, input = code, wrappedErr; // first, create the Script object to check the syntax - if (code === '\n') return cb(null); while (true) { try { if (!/^\s*$/.test(code) && - (self.replMode === exports.REPL_MODE_STRICT || retry)) { + (self.replMode === exports.REPL_MODE_STRICT)) { // "void 0" keeps the repl from returning "use strict" as the // result value for let/const statements. code = `'use strict'; void 0;\n${code}`; } - var script = vm.createScript(code, { + script = vm.createScript(code, { filename: file, displayErrors: true }); } catch (e) { debug('parse error %j', code, e); - if (self.replMode === exports.REPL_MODE_MAGIC && - e.message === BLOCK_SCOPED_ERROR && - !retry || self.wrappedCmd) { - if (self.wrappedCmd) { - self.wrappedCmd = false; - // unwrap and try again - code = `${input.substring(1, input.length - 2)}\n`; - wrappedErr = e; - } else { - retry = true; - } + if (wrappedCmd) { + wrappedCmd = false; + // unwrap and try again + code = input; + wrappedErr = e; continue; } // preserve original error for wrapped command const error = wrappedErr || e; - if (isRecoverableError(error, self)) + if (isRecoverableError(error, code)) err = new Recoverable(error); else err = error; @@ -394,7 +267,6 @@ function REPLServer(prompt, (_, pre, line) => pre + (line - 1)); } top.outputStream.write((e.stack || e) + '\n'); - top.lineParser.reset(); top.bufferedCommand = ''; top.lines.level = []; top.displayPrompt(); @@ -421,7 +293,6 @@ function REPLServer(prompt, self.outputStream = output; self.resetContext(); - self.lineParser = new LineParser(); self.bufferedCommand = ''; self.lines.level = []; @@ -485,7 +356,6 @@ function REPLServer(prompt, sawSIGINT = false; } - self.lineParser.reset(); self.bufferedCommand = ''; self.lines.level = []; self.displayPrompt(); @@ -493,6 +363,7 @@ function REPLServer(prompt, self.on('line', function(cmd) { debug('line %j', cmd); + cmd = cmd || ''; sawSIGINT = false; if (self.editorMode) { @@ -510,24 +381,28 @@ function REPLServer(prompt, return; } - // leading whitespaces in template literals should not be trimmed. - if (self._inTemplateLiteral) { - self._inTemplateLiteral = false; - } else { - cmd = self.lineParser.parseLine(cmd); - } + // Check REPL keywords and empty lines against a trimmed line input. + const trimmedCmd = cmd.trim(); // Check to see if a REPL keyword was used. If it returns true, // display next prompt and return. - if (cmd && cmd.charAt(0) === '.' && cmd.charAt(1) !== '.' && - isNaN(parseFloat(cmd))) { - const matches = cmd.match(/^\.([^\s]+)\s*(.*)$/); - const keyword = matches && matches[1]; - const rest = matches && matches[2]; - if (self.parseREPLKeyword(keyword, rest) === true) { - return; - } else if (!self.bufferedCommand) { - self.outputStream.write('Invalid REPL keyword\n'); + if (trimmedCmd) { + if (trimmedCmd.charAt(0) === '.' && isNaN(parseFloat(trimmedCmd))) { + const matches = trimmedCmd.match(/^\.([^\s]+)\s*(.*)$/); + const keyword = matches && matches[1]; + const rest = matches && matches[2]; + if (self.parseREPLKeyword(keyword, rest) === true) { + return; + } + if (!self.bufferedCommand) { + self.outputStream.write('Invalid REPL keyword\n'); + finish(null); + return; + } + } + } else { + // Print a new line when hitting enter. + if (!self.bufferedCommand) { finish(null); return; } @@ -542,12 +417,10 @@ function REPLServer(prompt, debug('finish', e, ret); self.memory(cmd); - self.wrappedCmd = false; if (e && !self.bufferedCommand && cmd.trim().startsWith('npm ')) { self.outputStream.write('npm should be run outside of the ' + 'node repl, in your normal shell.\n' + '(Press Control-D to exit.)\n'); - self.lineParser.reset(); self.bufferedCommand = ''; self.displayPrompt(); return; @@ -555,8 +428,7 @@ function REPLServer(prompt, // If error was SyntaxError and not JSON.parse error if (e) { - if (e instanceof Recoverable && !self.lineParser.shouldFail && - !sawCtrlD) { + if (e instanceof Recoverable && !sawCtrlD) { // Start buffering data like that: // { // ... x: 1 @@ -570,7 +442,6 @@ function REPLServer(prompt, } // Clear buffer if no SyntaxErrors - self.lineParser.reset(); self.bufferedCommand = ''; sawCtrlD = false; @@ -1232,7 +1103,6 @@ function defineDefaultCommands(repl) { repl.defineCommand('break', { help: 'Sometimes you get stuck, this gets you out', action: function() { - this.lineParser.reset(); this.bufferedCommand = ''; this.displayPrompt(); } @@ -1247,7 +1117,6 @@ function defineDefaultCommands(repl) { repl.defineCommand('clear', { help: clearMessage, action: function() { - this.lineParser.reset(); this.bufferedCommand = ''; if (!this.useGlobal) { this.outputStream.write('Clearing context...\n'); @@ -1367,20 +1236,13 @@ REPLServer.prototype.convertToContext = function(cmd) { return cmd; }; -function bailOnIllegalToken(parser) { - return parser._literal === null && - !parser.blockComment && - !parser.regExpLiteral; -} - // If the error is that we've unexpectedly ended the input, // then let the user try to recover by adding more input. -function isRecoverableError(e, self) { +function isRecoverableError(e, code) { if (e && e.name === 'SyntaxError') { var message = e.message; if (message === 'Unterminated template literal' || message === 'Missing } in template expression') { - self._inTemplateLiteral = true; return true; } @@ -1390,11 +1252,81 @@ function isRecoverableError(e, self) { return true; if (message === 'Invalid or unexpected token') - return !bailOnIllegalToken(self.lineParser); + return isCodeRecoverable(code); } return false; } +// Check whether a code snippet should be forced to fail in the REPL. +function isCodeRecoverable(code) { + var current, previous, stringLiteral; + var isBlockComment = false; + var isSingleComment = false; + var isRegExpLiteral = false; + var lastChar = code.charAt(code.length - 2); + var prevTokenChar = null; + + for (var i = 0; i < code.length; i++) { + previous = current; + current = code[i]; + + if (previous === '\\' && (stringLiteral || isRegExpLiteral)) { + current = null; + continue; + } + + if (stringLiteral) { + if (stringLiteral === current) { + stringLiteral = null; + } + continue; + } else { + if (isRegExpLiteral && current === '/') { + isRegExpLiteral = false; + continue; + } + + if (isBlockComment && previous === '*' && current === '/') { + isBlockComment = false; + continue; + } + + if (isSingleComment && current === '\n') { + isSingleComment = false; + continue; + } + + if (isBlockComment || isRegExpLiteral || isSingleComment) continue; + + if (current === '/' && previous === '/') { + isSingleComment = true; + continue; + } + + if (previous === '/') { + if (current === '*') { + isBlockComment = true; + } else if ( + // Distinguish between a division operator and the start of a regex + // by examining the non-whitespace character that precedes the / + [null, '(', '[', '{', '}', ';'].includes(prevTokenChar) + ) { + isRegExpLiteral = true; + } + continue; + } + + if (current.trim()) prevTokenChar = current; + } + + if (current === '\'' || current === '"') { + stringLiteral = current; + } + } + + return stringLiteral ? lastChar === '\\' : isBlockComment; +} + function Recoverable(err) { this.err = err; } diff --git a/test/parallel/test-repl-multi-line-templates.js b/test/parallel/test-repl-multi-line-templates.js new file mode 100644 index 00000000000000..59cd179cfb3f8c --- /dev/null +++ b/test/parallel/test-repl-multi-line-templates.js @@ -0,0 +1,25 @@ +'use strict'; + +require('../common'); +const assert = require('assert'); +const repl = require('repl'); +const stream = require('stream'); + +const inputStream = new stream.PassThrough(); +const outputStream = new stream.PassThrough(); +outputStream.accum = ''; + +outputStream.on('data', (data) => { + outputStream.accum += data; +}); + +const r = repl.start({ + input: inputStream, + output: outputStream, + useColors: false, + terminal: false, + prompt: '' +}); + +r.write('const str = `tacos \\\nfoobar`'); +assert.strictEqual(r.output.accum, '... '); diff --git a/test/parallel/test-repl.js b/test/parallel/test-repl.js index 3b705f7b6be97f..c2fc9678b73556 100644 --- a/test/parallel/test-repl.js +++ b/test/parallel/test-repl.js @@ -386,14 +386,15 @@ function error_test() { { client: client_unix, send: '(function() {\nif (false) {} /bar"/;\n}())', - expect: `${prompt_multiline}${prompt_multiline}undefined\n${prompt_unix}` + expect: prompt_multiline + prompt_multiline + 'undefined\n' + prompt_unix }, - // Do not parse `...[]` as a REPL keyword - { client: client_unix, send: '...[]\n', - expect: `${prompt_multiline}` }, - // bring back the repl to prompt - { client: client_unix, send: '.break', - expect: `${prompt_unix}` } + + // Newline within template string maintains whitespace. + { client: client_unix, send: '`foo \n`', + expect: prompt_multiline + '\'foo \\n\'\n' + prompt_unix }, + // Whitespace is not evaluated. + { client: client_unix, send: ' \t \n', + expect: prompt_unix } ]); } From 11256f7b20ae345d8b562103e8792d48d0fdafe9 Mon Sep 17 00:00:00 2001 From: Shigeki Ohtsu Date: Tue, 20 Jun 2017 23:44:53 +0900 Subject: [PATCH 002/241] crypto: warn if counter mode used in createCipher MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `crypto.createCipher()` sets the fixed IV derived from password and it leads to a security risk of nonce reuse when counter mode is used. A warning is emitted when CTR, GCM or CCM is used in `crypto.createCipher()` to notify users to avoid nonce reuse. Backport-PR-URL: https://github.com/nodejs/node/pull/16583 Fixes: https://github.com/nodejs/node/issues/13801 PR-URL: https://github.com/nodejs/node/pull/13821 Reviewed-By: Ben Noordhuis Reviewed-By: Fedor Indutny Reviewed-By: James M Snell Reviewed-By: Tobias Nießen --- doc/api/crypto.md | 7 ++++++- src/node_crypto.cc | 8 ++++++++ test/parallel/test-crypto-cipher-decipher.js | 4 ++++ 3 files changed, 18 insertions(+), 1 deletion(-) diff --git a/doc/api/crypto.md b/doc/api/crypto.md index 19fdaa4dd6ad19..4a7488ba6f2f70 100644 --- a/doc/api/crypto.md +++ b/doc/api/crypto.md @@ -1102,7 +1102,11 @@ rapidly. In line with OpenSSL's recommendation to use pbkdf2 instead of [`EVP_BytesToKey`][] it is recommended that developers derive a key and IV on their own using [`crypto.pbkdf2()`][] and to use [`crypto.createCipheriv()`][] -to create the `Cipher` object. +to create the `Cipher` object. Users should not use ciphers with counter mode +(e.g. CTR, GCM or CCM) in `crypto.createCipher()`. A warning is emitted when +they are used in order to avoid the risk of IV reuse that causes +vulnerabilities. For the case when IV is reused in GCM, see [Nonce-Disrespecting +Adversaries][] for details. ### crypto.createCipheriv(algorithm, key, iv) @@ -2024,6 +2028,7 @@ the `crypto`, `tls`, and `https` modules and are generally specific to OpenSSL. [NIST SP 800-131A]: http://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-131Ar1.pdf [NIST SP 800-132]: http://nvlpubs.nist.gov/nistpubs/Legacy/SP/nistspecialpublication800-132.pdf [OpenSSL cipher list format]: https://www.openssl.org/docs/man1.0.2/apps/ciphers.html#CIPHER-LIST-FORMAT +[Nonce-Disrespecting Adversaries]: https://github.com/nonce-disrespect/nonce-disrespect [OpenSSL's SPKAC implementation]: https://www.openssl.org/docs/man1.0.2/apps/spkac.html [publicly trusted list of CAs]: https://mxr.mozilla.org/mozilla/source/security/nss/lib/ckfw/builtins/certdata.txt [RFC 2412]: https://www.rfc-editor.org/rfc/rfc2412.txt diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 09002972a94b5f..12ace3358373c1 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -3351,6 +3351,14 @@ void CipherBase::Init(const char* cipher_type, EVP_CIPHER_CTX_init(&ctx_); const bool encrypt = (kind_ == kCipher); EVP_CipherInit_ex(&ctx_, cipher_, nullptr, nullptr, nullptr, encrypt); + + int mode = EVP_CIPHER_CTX_mode(&ctx_); + if (encrypt && (mode == EVP_CIPH_CTR_MODE || mode == EVP_CIPH_GCM_MODE || + mode == EVP_CIPH_CCM_MODE)) { + ProcessEmitWarning(env(), "Use Cipheriv for counter mode of %s", + cipher_type); + } + if (!EVP_CIPHER_CTX_set_key_length(&ctx_, key_len)) { EVP_CIPHER_CTX_cleanup(&ctx_); return env()->ThrowError("Invalid key length"); diff --git a/test/parallel/test-crypto-cipher-decipher.js b/test/parallel/test-crypto-cipher-decipher.js index 8b1b0051d34962..6b2df3d81fd893 100644 --- a/test/parallel/test-crypto-cipher-decipher.js +++ b/test/parallel/test-crypto-cipher-decipher.js @@ -148,3 +148,7 @@ testCipher2(Buffer.from('0123456789abcdef')); assert.strictEqual(decipher.setAuthTag(tagbuf), decipher); assert.strictEqual(decipher.setAAD(aadbuf), decipher); } + +// https://github.com/nodejs/node/issues/13801 +common.expectWarning('Warning', 'Use Cipheriv for counter mode of aes-256-gcm'); +crypto.createCipher('aes-256-gcm', '0123456789'); From 12036afbdaae175bd1d36aa508e7e2cec10c1bc9 Mon Sep 17 00:00:00 2001 From: Shigeki Ohtsu Date: Fri, 25 Aug 2017 01:42:55 +0900 Subject: [PATCH 003/241] crypto: fix error of createCipher in wrap mode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit EVP_CIPHER_CTX_FLAG_WRAP_ALLOW flag needs to be set in using wrap mode ciphers. In `crypto.createCipher()`, AES key wrap mode does not use a default IV defined in RFC3394 but a generated IV with `EVP_BytesToKey()` to be consistent API behaviors with other ciphers. The built-in AES wrap mode in OpenSSL is not supported in FIPS mode as http://openssl.6102.n7.nabble.com/AES-Key-Wrap-in-FIPS-Mode-td50238.html so its tests in FIPS mode are skipped. Backport-PR-URL: https://github.com/nodejs/node/pull/16584 Fixes: https://github.com/nodejs/node/issues/15009 PR-URL: https://github.com/nodejs/node/pull/15037 Reviewed-By: Fedor Indutny Reviewed-By: Ben Noordhuis Reviewed-By: Tobias Nießen Reviewed-By: James M Snell --- src/node_crypto.cc | 10 +++++++- test/parallel/test-crypto-binary-default.js | 21 ++++++++++++++++ .../test-crypto-cipheriv-decipheriv.js | 24 +++++++++++++++++++ 3 files changed, 54 insertions(+), 1 deletion(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 12ace3358373c1..4941d96655a9bf 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -3359,6 +3359,9 @@ void CipherBase::Init(const char* cipher_type, cipher_type); } + if (mode == EVP_CIPH_WRAP_MODE) + EVP_CIPHER_CTX_set_flags(&ctx_, EVP_CIPHER_CTX_FLAG_WRAP_ALLOW); + if (!EVP_CIPHER_CTX_set_key_length(&ctx_, key_len)) { EVP_CIPHER_CTX_cleanup(&ctx_); return env()->ThrowError("Invalid key length"); @@ -3406,13 +3409,18 @@ void CipherBase::InitIv(const char* cipher_type, } const int expected_iv_len = EVP_CIPHER_iv_length(cipher_); - const bool is_gcm_mode = (EVP_CIPH_GCM_MODE == EVP_CIPHER_mode(cipher_)); + const int mode = EVP_CIPHER_mode(cipher_); + const bool is_gcm_mode = (EVP_CIPH_GCM_MODE == mode); if (is_gcm_mode == false && iv_len != expected_iv_len) { return env()->ThrowError("Invalid IV length"); } EVP_CIPHER_CTX_init(&ctx_); + + if (mode == EVP_CIPH_WRAP_MODE) + EVP_CIPHER_CTX_set_flags(&ctx_, EVP_CIPHER_CTX_FLAG_WRAP_ALLOW); + const bool encrypt = (kind_ == kCipher); EVP_CipherInit_ex(&ctx_, cipher_, nullptr, nullptr, nullptr, encrypt); diff --git a/test/parallel/test-crypto-binary-default.js b/test/parallel/test-crypto-binary-default.js index 2343360df9fadf..c477301256d919 100644 --- a/test/parallel/test-crypto-binary-default.js +++ b/test/parallel/test-crypto-binary-default.js @@ -509,12 +509,33 @@ function testCipher4(key, iv) { 'encryption and decryption with key and iv'); } + +function testCipher5(key, iv) { + // Test encryption and decryption with explicit key with aes128-wrap + const plaintext = + '32|RmVZZkFUVmpRRkp0TmJaUm56ZU9qcnJkaXNNWVNpTTU*|iXmckfRWZBGWWELw' + + 'eCBsThSsfUHLeRe0KCsK8ooHgxie0zOINpXxfZi/oNG7uq9JWFVCk70gfzQH8ZUJ' + + 'jAfaFg**'; + const cipher = crypto.createCipher('id-aes128-wrap', key); + let ciph = cipher.update(plaintext, 'utf8', 'buffer'); + ciph = Buffer.concat([ciph, cipher.final('buffer')]); + + const decipher = crypto.createDecipher('id-aes128-wrap', key); + let txt = decipher.update(ciph, 'buffer', 'utf8'); + txt += decipher.final('utf8'); + + assert.strictEqual(txt, plaintext, + 'encryption and decryption with key'); +} + if (!common.hasFipsCrypto) { testCipher1('MySecretKey123'); testCipher1(Buffer.from('MySecretKey123')); testCipher2('0123456789abcdef'); testCipher2(Buffer.from('0123456789abcdef')); + + testCipher5(Buffer.from('0123456789abcd0123456789')); } testCipher3('0123456789abcd0123456789', '12345678'); diff --git a/test/parallel/test-crypto-cipheriv-decipheriv.js b/test/parallel/test-crypto-cipheriv-decipheriv.js index 2ee8e813233e77..285440643c42b8 100644 --- a/test/parallel/test-crypto-cipheriv-decipheriv.js +++ b/test/parallel/test-crypto-cipheriv-decipheriv.js @@ -55,12 +55,36 @@ function testCipher2(key, iv) { assert.strictEqual(txt, plaintext, 'encryption/decryption with key and iv'); } + +function testCipher3(key, iv) { + // Test encryption and decryption with explicit key and iv. + // AES Key Wrap test vector comes from RFC3394 + const plaintext = Buffer.from('00112233445566778899AABBCCDDEEFF', 'hex'); + + const cipher = crypto.createCipheriv('id-aes128-wrap', key, iv); + let ciph = cipher.update(plaintext, 'utf8', 'buffer'); + ciph = Buffer.concat([ciph, cipher.final('buffer')]); + const ciph2 = Buffer.from('1FA68B0A8112B447AEF34BD8FB5A7B829D3E862371D2CFE5', + 'hex'); + assert(ciph.equals(ciph2)); + const decipher = crypto.createDecipheriv('id-aes128-wrap', key, iv); + let deciph = decipher.update(ciph, 'buffer'); + deciph = Buffer.concat([deciph, decipher.final()]); + + assert(deciph.equals(plaintext), 'encryption/decryption with key and iv'); +} + testCipher1('0123456789abcd0123456789', '12345678'); testCipher1('0123456789abcd0123456789', Buffer.from('12345678')); testCipher1(Buffer.from('0123456789abcd0123456789'), '12345678'); testCipher1(Buffer.from('0123456789abcd0123456789'), Buffer.from('12345678')); testCipher2(Buffer.from('0123456789abcd0123456789'), Buffer.from('12345678')); +if (!common.hasFipsCrypto) { + testCipher3(Buffer.from('000102030405060708090A0B0C0D0E0F', 'hex'), + Buffer.from('A6A6A6A6A6A6A6A6', 'hex')); +} + // Zero-sized IV should be accepted in ECB mode. crypto.createCipheriv('aes-128-ecb', Buffer.alloc(16), Buffer.alloc(0)); From 8b70ad1932c4d734d2cc6fe43ceec5d40332e635 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Sun, 27 Aug 2017 06:31:59 +0200 Subject: [PATCH 004/241] src: turn key length exception into CHECK This exception can logically never happen because of the key stretching that takes place first. Failure must therefore be a bug in Node.js and not in the executing script. Backport-PR-URL: https://github.com/nodejs/node/pull/16585 PR-URL: https://github.com/nodejs/node/pull/15183 Reviewed-By: Colin Ihrig Reviewed-By: James M Snell --- src/node_crypto.cc | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 4941d96655a9bf..b4c338aa59c34e 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -3362,10 +3362,7 @@ void CipherBase::Init(const char* cipher_type, if (mode == EVP_CIPH_WRAP_MODE) EVP_CIPHER_CTX_set_flags(&ctx_, EVP_CIPHER_CTX_FLAG_WRAP_ALLOW); - if (!EVP_CIPHER_CTX_set_key_length(&ctx_, key_len)) { - EVP_CIPHER_CTX_cleanup(&ctx_); - return env()->ThrowError("Invalid key length"); - } + CHECK_EQ(1, EVP_CIPHER_CTX_set_key_length(&ctx_, key_len)); EVP_CipherInit_ex(&ctx_, nullptr, From 4bdbcc3e40abfaa1252672da3b1b43bbfeaed17b Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Wed, 5 Jul 2017 14:25:55 -0700 Subject: [PATCH 005/241] src: whitelist v8 options with '_' or '-' V8 options allow either '_' or '-' to be used in options as a seperator, such as "--abort-on_uncaught-exception". Allow these case variations when used with NODE_OPTIONS. PR-URL: https://github.com/nodejs/node/pull/14093 Reviewed-By: Richard Lau Reviewed-By: Refael Ackermann Reviewed-By: James M Snell --- doc/api/cli.md | 2 +- src/node.cc | 33 ++++++++++++++++++++------ test/parallel/test-cli-node-options.js | 4 ++++ 3 files changed, 31 insertions(+), 8 deletions(-) diff --git a/doc/api/cli.md b/doc/api/cli.md index 41c747c1c3da75..f1b33e883ae883 100644 --- a/doc/api/cli.md +++ b/doc/api/cli.md @@ -372,7 +372,7 @@ Node options that are allowed are: V8 options that are allowed are: - `--abort-on-uncaught-exception` -- `--max_old_space_size` +- `--max-old-space-size` ### `NODE_REPL_HISTORY=file` diff --git a/src/node.cc b/src/node.cc index 98fde0dbbe3f65..2fd6da0c6f4c37 100644 --- a/src/node.cc +++ b/src/node.cc @@ -3786,15 +3786,34 @@ static void PrintHelp() { } +static bool ArgIsAllowed(const char* arg, const char* allowed) { + for (; *arg && *allowed; arg++, allowed++) { + // Like normal strcmp(), except that a '_' in `allowed` matches either a '-' + // or '_' in `arg`. + if (*allowed == '_') { + if (!(*arg == '_' || *arg == '-')) + return false; + } else { + if (*arg != *allowed) + return false; + } + } + + // "--some-arg=val" is allowed for "--some-arg" + if (*arg == '=') + return true; + + // Both must be null, or one string is just a prefix of the other, not a + // match. + return !*arg && !*allowed; +} + + static void CheckIfAllowedInEnv(const char* exe, bool is_env, const char* arg) { if (!is_env) return; - // Find the arg prefix when its --some_arg=val - const char* eq = strchr(arg, '='); - size_t arglen = eq ? eq - arg : strlen(arg); - static const char* whitelist[] = { // Node options, sorted in `node --help` order for ease of comparison. "--require", "-r", @@ -3820,14 +3839,14 @@ static void CheckIfAllowedInEnv(const char* exe, bool is_env, "--openssl-config", "--icu-data-dir", - // V8 options - "--abort-on-uncaught-exception", + // V8 options (define with '_', which allows '-' or '_') + "--abort_on_uncaught_exception", "--max_old_space_size", }; for (unsigned i = 0; i < arraysize(whitelist); i++) { const char* allowed = whitelist[i]; - if (strlen(allowed) == arglen && strncmp(allowed, arg, arglen) == 0) + if (ArgIsAllowed(arg, allowed)) return; } diff --git a/test/parallel/test-cli-node-options.js b/test/parallel/test-cli-node-options.js index 104473bfa69e34..a1148fd0516703 100644 --- a/test/parallel/test-cli-node-options.js +++ b/test/parallel/test-cli-node-options.js @@ -26,6 +26,7 @@ disallow('--interactive'); disallow('-i'); disallow('--v8-options'); disallow('--'); +disallow('--no_warnings'); // Node options don't allow '_' instead of '-'. function disallow(opt) { const options = {env: {NODE_OPTIONS: opt}}; @@ -43,6 +44,7 @@ const printA = require.resolve('../fixtures/printA.js'); expect(`-r ${printA}`, 'A\nB\n'); expect('--abort-on-uncaught-exception', 'B\n'); +expect('--abort_on_uncaught_exception', 'B\n'); expect('--no-deprecation', 'B\n'); expect('--no-warnings', 'B\n'); expect('--trace-warnings', 'B\n'); @@ -61,6 +63,8 @@ expect('--icu-data-dir=_d', 'B\n'); // V8 options expect('--max_old_space_size=0', 'B\n'); +expect('--max-old_space-size=0', 'B\n'); +expect('--max-old-space-size=0', 'B\n'); function expect(opt, want) { const printB = require.resolve('../fixtures/printB.js'); From b76d9d4b376b848986505e852ce5414f85d32128 Mon Sep 17 00:00:00 2001 From: Oblosys Date: Thu, 14 Sep 2017 01:31:37 +0200 Subject: [PATCH 006/241] doc: fix emitKeypressEvents stream type PR-URL: https://github.com/nodejs/node/pull/15399 Reviewed-By: Colin Ihrig Reviewed-By: Luigi Pinca Reviewed-By: James M Snell --- doc/api/readline.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/api/readline.md b/doc/api/readline.md index bccee609ed3e8e..06a8b86205e55c 100644 --- a/doc/api/readline.md +++ b/doc/api/readline.md @@ -446,7 +446,7 @@ added: v0.7.7 * `stream` {Readable} * `interface` {readline.Interface} -The `readline.emitKeypressEvents()` method causes the given [Writable][] +The `readline.emitKeypressEvents()` method causes the given [Readable][] `stream` to begin emitting `'keypress'` events corresponding to received input. Optionally, `interface` specifies a `readline.Interface` instance for which From 3e6a7e179b51b16903213f586ee893d51166139e Mon Sep 17 00:00:00 2001 From: Alex Gresnel <31708810+agresnel@users.noreply.github.com> Date: Sat, 9 Sep 2017 11:44:55 -0700 Subject: [PATCH 007/241] child_process: set shell to false in fork() This commit ensures that spawn()'s shell option is unconditionally set to false when fork() is called. Refs: https://github.com/nodejs/node/pull/15299 Fixes: https://github.com/nodejs/node/issues/13983 PR-URL: https://github.com/nodejs/node/pull/15352 Reviewed-By: James M Snell Reviewed-By: Luigi Pinca Reviewed-By: Richard Lau Reviewed-By: Colin Ihrig --- doc/api/child_process.md | 3 +++ lib/child_process.js | 1 + 2 files changed, 4 insertions(+) diff --git a/doc/api/child_process.md b/doc/api/child_process.md index b3484de5762890..c984004f94b6d2 100644 --- a/doc/api/child_process.md +++ b/doc/api/child_process.md @@ -304,6 +304,9 @@ output on this fd is expected to be line delimited JSON objects. *Note: Unlike the fork(2) POSIX system call, `child_process.fork()` does not clone the current process.* +*Note*: The `shell` option available in [`child_process.spawn()`][] is not +supported by `child_process.fork()` and will be ignored if set. + ### child_process.spawn(command[, args][, options]) \n` + - inc + `\n\n`; - input = input.split(include + '\n').join(includeData[fname] + '\n'); + `${inc}\n\n`; + input = input.split(`${include}\n`).join(`${includeData[fname]}\n`); if (incCount === 0) { return cb(null, input); } From b38616c187618042215e73dce237e7d7ead27e00 Mon Sep 17 00:00:00 2001 From: Jimi van der Woning Date: Mon, 6 Nov 2017 17:36:56 +0100 Subject: [PATCH 208/241] test: fix typos in read-buffer tests The offset-exceeding tests for readFloat contained a double test for readFloatLE instead of one for readFloatLE and one for readFloatBE. This is fixed in this commit. PR-URL: https://github.com/nodejs/node/pull/16834 Reviewed-By: Vse Mozhet Byt Reviewed-By: Rich Trott --- test/parallel/test-buffer-read.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-buffer-read.js b/test/parallel/test-buffer-read.js index d3a1c941422e51..5eac575ff5ab81 100644 --- a/test/parallel/test-buffer-read.js +++ b/test/parallel/test-buffer-read.js @@ -59,13 +59,13 @@ read(buf, 'readUIntBE', [2, 0], 0xfd); read(buf, 'readUIntLE', [2, 0], 0x48); // attempt to overflow buffers, similar to previous bug in array buffers -assert.throws(() => Buffer.allocUnsafe(8).readFloatLE(0xffffffff), +assert.throws(() => Buffer.allocUnsafe(8).readFloatBE(0xffffffff), RangeError); assert.throws(() => Buffer.allocUnsafe(8).readFloatLE(0xffffffff), RangeError); // ensure negative values can't get past offset -assert.throws(() => Buffer.allocUnsafe(8).readFloatLE(-1), RangeError); +assert.throws(() => Buffer.allocUnsafe(8).readFloatBE(-1), RangeError); assert.throws(() => Buffer.allocUnsafe(8).readFloatLE(-1), RangeError); // offset checks From 9b65fe4e208b45a63155e0c2a164711e2e8ab0b3 Mon Sep 17 00:00:00 2001 From: Vse Mozhet Byt Date: Mon, 6 Nov 2017 20:46:06 +0200 Subject: [PATCH 209/241] tools: remove unneeded parentheses in doc/html.js PR-URL: https://github.com/nodejs/node/pull/16845 Ref: https://github.com/nodejs/node/pull/16801#discussion_r149142120 Reviewed-By: Colin Ihrig Reviewed-By: Jeremiah Senkpiel Reviewed-By: Rich Trott --- tools/doc/html.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/doc/html.js b/tools/doc/html.js index 898118d42bf9da..730031471fcc04 100644 --- a/tools/doc/html.js +++ b/tools/doc/html.js @@ -466,7 +466,7 @@ function getId(text) { text = text.replace(/^_+|_+$/, ''); text = text.replace(/^([^a-z])/, '_$1'); if (idCounters.hasOwnProperty(text)) { - text += `_${(++idCounters[text])}`; + text += `_${++idCounters[text]}`; } else { idCounters[text] = 0; } From 5e4ce5c3ab8101cb532986edf8f073a8f1b82d72 Mon Sep 17 00:00:00 2001 From: Dara Hayes Date: Mon, 6 Nov 2017 15:23:13 +0000 Subject: [PATCH 210/241] test: update test to use fixtures.readKey Use fixtures.readKey() rather than common.fixturesDir in test-regress-GH-1531. PR-URL: https://github.com/nodejs/node/pull/16811 Reviewed-By: Colin Ihrig Reviewed-By: Anatoli Papirovski Reviewed-By: Gireesh Punathil Reviewed-By: Rich Trott --- test/parallel/test-regress-GH-1531.js | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/test/parallel/test-regress-GH-1531.js b/test/parallel/test-regress-GH-1531.js index 7d1f4a0dbec4c1..a61cc64ab626af 100644 --- a/test/parallel/test-regress-GH-1531.js +++ b/test/parallel/test-regress-GH-1531.js @@ -1,16 +1,20 @@ 'use strict'; const common = require('../common'); +// This test ensures that a http request callback is called +// when the agent option is set +// See https://github.com/nodejs/node-v0.x-archive/issues/1531 + if (!common.hasCrypto) common.skip('missing crypto'); -const https = require('https'); +const fixtures = require('../common/fixtures'); -const fs = require('fs'); +const https = require('https'); const options = { - key: fs.readFileSync(`${common.fixturesDir}/keys/agent1-key.pem`), - cert: fs.readFileSync(`${common.fixturesDir}/keys/agent1-cert.pem`) + key: fixtures.readKey('agent1-key.pem'), + cert: fixtures.readKey('agent1-cert.pem') }; const server = https.createServer(options, function(req, res) { From 4c066a4fac95d5455e825fad7a38e31def242ef4 Mon Sep 17 00:00:00 2001 From: "Maring, Damian Lion" Date: Mon, 6 Nov 2017 15:06:30 +0000 Subject: [PATCH 211/241] test: use fixtures module in test-repl PR-URL: https://github.com/nodejs/node/pull/16809 Reviewed-By: Rich Trott Reviewed-By: Gireesh Punathil --- test/parallel/test-repl.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/parallel/test-repl.js b/test/parallel/test-repl.js index c2fc9678b73556..378c81871d1356 100644 --- a/test/parallel/test-repl.js +++ b/test/parallel/test-repl.js @@ -1,6 +1,7 @@ 'use strict'; const common = require('../common'); +const fixtures = require('../common/fixtures'); const assert = require('assert'); common.globalCheck = false; @@ -20,7 +21,7 @@ let server_tcp, server_unix, client_tcp, client_unix, replServer; // absolute path to test/fixtures/a.js -const moduleFilename = require('path').join(common.fixturesDir, 'a'); +const moduleFilename = fixtures.path('a'); console.error('repl test'); From f0043bd24b6629c93b5411a492924f9234382676 Mon Sep 17 00:00:00 2001 From: Brian O'Connell Date: Mon, 6 Nov 2017 15:15:14 +0000 Subject: [PATCH 212/241] test: refactor tls test to use fixtres.readSync PR-URL: https://github.com/nodejs/node/pull/16816 Reviewed-By: Colin Ihrig Reviewed-By: Gireesh Punathil --- test/parallel/test-tls-hello-parser-failure.js | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/test/parallel/test-tls-hello-parser-failure.js b/test/parallel/test-tls-hello-parser-failure.js index a6659d4578bc9b..9d9f8c77716b64 100644 --- a/test/parallel/test-tls-hello-parser-failure.js +++ b/test/parallel/test-tls-hello-parser-failure.js @@ -1,6 +1,10 @@ 'use strict'; const common = require('../common'); +const fixtures = require('../common/fixtures'); + +// This test ensures that the tls parser causes a client error if the client +// sends invalid data. if (!common.hasCrypto) common.skip('missing crypto'); @@ -9,11 +13,10 @@ const assert = require('assert'); const tls = require('tls'); const net = require('net'); -const fs = require('fs'); const options = { - key: fs.readFileSync(`${common.fixturesDir}/test_key.pem`), - cert: fs.readFileSync(`${common.fixturesDir}/test_cert.pem`) + key: fixtures.readSync('test_key.pem'), + cert: fixtures.readSync('test_cert.pem') }; const bonkers = Buffer.alloc(1024 * 1024, 42); From 10b7545f3adbbebf3112e041a83223c192c25fc2 Mon Sep 17 00:00:00 2001 From: Sascha Tandel Date: Mon, 6 Nov 2017 15:19:09 +0000 Subject: [PATCH 213/241] test: include file mode in assert message If the REPL history file is created with an invalid mode include the failed mode in the error message. PR-URL: https://github.com/nodejs/node/pull/16815 Reviewed-By: Gireesh Punathil Reviewed-By: Joyee Cheung --- test/parallel/test-repl-history-perm.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-repl-history-perm.js b/test/parallel/test-repl-history-perm.js index 81c4b50d6a1adf..773e91ac1bede2 100644 --- a/test/parallel/test-repl-history-perm.js +++ b/test/parallel/test-repl-history-perm.js @@ -1,4 +1,7 @@ 'use strict'; + +// Verifies that the REPL history file is created with mode 0600 + // Flags: --expose_internals const common = require('../common'); @@ -40,9 +43,10 @@ const checkResults = common.mustCall(function(err, r) { r.input.end(); const stat = fs.statSync(replHistoryPath); + const fileMode = stat.mode & 0o777; assert.strictEqual( - stat.mode & 0o777, 0o600, - 'REPL history file should be mode 0600'); + fileMode, 0o600, + `REPL history file should be mode 0600 but was 0${fileMode.toString(8)}`); }); repl.createInternalRepl( From 551164986288f19915f672d2bf4d00b3143130a7 Mon Sep 17 00:00:00 2001 From: jonask Date: Mon, 6 Nov 2017 17:28:03 +0200 Subject: [PATCH 214/241] test: use default assertion message In test-child-process-spawnsync, the assert.strictEqual() custom message was hiding information about why the test has failed. It just showed what value is expected and in case of failure we want to know which value has caused test to fail. PR-URL: https://github.com/nodejs/node/pull/16819 Reviewed-By: Colin Ihrig Reviewed-By: Gireesh Punathil Reviewed-By: Anatoli Papirovski Reviewed-By: Rich Trott --- test/parallel/test-child-process-spawnsync.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-child-process-spawnsync.js b/test/parallel/test-child-process-spawnsync.js index 77d205f79a414e..91ac346f3ba5cf 100644 --- a/test/parallel/test-child-process-spawnsync.js +++ b/test/parallel/test-child-process-spawnsync.js @@ -7,7 +7,7 @@ const spawnSync = require('child_process').spawnSync; // Echo does different things on Windows and Unix, but in both cases, it does // more-or-less nothing if there are no parameters const ret = spawnSync('sleep', ['0']); -assert.strictEqual(ret.status, 0, 'exit status should be zero'); +assert.strictEqual(ret.status, 0); // Error test when command does not exist const ret_err = spawnSync('command_does_not_exist', ['bar']).error; From b458cb1b066ed46024a5b82b1411cfaddf9abf04 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Sun, 5 Nov 2017 14:44:43 +0000 Subject: [PATCH 215/241] test: move test-http-keepalive-maxsockets to sequential test-http-keepalive-maxsockets.js will fail if sufficient copies are run at once. Move to sequential. PR-URL: https://github.com/nodejs/node/pull/16777 Reviewed-By: Anatoli Papirovski Reviewed-By: Refael Ackermann Reviewed-By: Colin Ihrig Reviewed-By: Gireesh Punathil Reviewed-By: Yuta Hiroto Reviewed-By: James M Snell --- test/{parallel => sequential}/test-http-keepalive-maxsockets.js | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename test/{parallel => sequential}/test-http-keepalive-maxsockets.js (100%) diff --git a/test/parallel/test-http-keepalive-maxsockets.js b/test/sequential/test-http-keepalive-maxsockets.js similarity index 100% rename from test/parallel/test-http-keepalive-maxsockets.js rename to test/sequential/test-http-keepalive-maxsockets.js From 4252c481df7f99fad69f00b437f7bd3896ff0e2f Mon Sep 17 00:00:00 2001 From: mbornath Date: Mon, 6 Nov 2017 18:01:27 +0100 Subject: [PATCH 216/241] test: remove message argument in cluster setup test In test/parallel/test-cluster-setup-master-cumulative.js: Remove the message parameter to ensure that the compared objects are printed out. Add the original message as a comment above. PR-URL: https://github.com/nodejs/node/pull/16838 Reviewed-By: Colin Ihrig Reviewed-By: Luigi Pinca Reviewed-By: Gireesh Punathil Reviewed-By: Rich Trott --- test/parallel/test-cluster-setup-master-cumulative.js | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/test/parallel/test-cluster-setup-master-cumulative.js b/test/parallel/test-cluster-setup-master-cumulative.js index 977c89a56aa573..76f66e8fea13dd 100644 --- a/test/parallel/test-cluster-setup-master-cumulative.js +++ b/test/parallel/test-cluster-setup-master-cumulative.js @@ -5,11 +5,8 @@ const cluster = require('cluster'); assert(cluster.isMaster); -assert.deepStrictEqual( - cluster.settings, - {}, - 'cluster.settings should not be initialized until needed' -); +// cluster.settings should not be initialized until needed +assert.deepStrictEqual(cluster.settings, {}); cluster.setupMaster(); assert.deepStrictEqual(cluster.settings, { From 7b25eba57b6a4cb7120530fa6fe1a0a123de8948 Mon Sep 17 00:00:00 2001 From: cjihrig Date: Sat, 4 Nov 2017 18:05:16 -0400 Subject: [PATCH 217/241] src: CHECK() for argument overflow in Spawn() This commit adds checks for overflow to args and env in Spawn(). It seems extremely unlikely that either of these values would overflow from a valid use case. Fixes: https://github.com/nodejs/node/issues/15622 PR-URL: https://github.com/nodejs/node/pull/16761 Reviewed-By: Gireesh Punathil --- src/process_wrap.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/process_wrap.cc b/src/process_wrap.cc index 0c41723ee77610..dcfda4678383f5 100644 --- a/src/process_wrap.cc +++ b/src/process_wrap.cc @@ -154,6 +154,8 @@ class ProcessWrap : public HandleWrap { if (!argv_v.IsEmpty() && argv_v->IsArray()) { Local js_argv = Local::Cast(argv_v); int argc = js_argv->Length(); + CHECK_GT(argc + 1, 0); // Check for overflow. + // Heap allocate to detect errors. +1 is for nullptr. options.args = new char*[argc + 1]; for (int i = 0; i < argc; i++) { @@ -177,6 +179,7 @@ class ProcessWrap : public HandleWrap { if (!env_v.IsEmpty() && env_v->IsArray()) { Local env_opt = Local::Cast(env_v); int envc = env_opt->Length(); + CHECK_GT(envc + 1, 0); // Check for overflow. options.env = new char*[envc + 1]; // Heap allocated to detect errors. for (int i = 0; i < envc; i++) { node::Utf8Value pair(env->isolate(), env_opt->Get(i)); From e2b80129cab745469ab63a5e3fcacf8b450acce4 Mon Sep 17 00:00:00 2001 From: cjihrig Date: Sat, 4 Nov 2017 10:36:41 -0400 Subject: [PATCH 218/241] doc: update subprocess.killed This commit changes the wording of subprocess.killed to reflect that a child process was successfully signaled, and not necessarily terminated. Fixes: https://github.com/nodejs/node/issues/16747 PR-URL: https://github.com/nodejs/node/pull/16748 Reviewed-By: Luigi Pinca Reviewed-By: Rich Trott Reviewed-By: James M Snell --- doc/api/child_process.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/doc/api/child_process.md b/doc/api/child_process.md index 2859930c730626..a8baf56c69b2ba 100644 --- a/doc/api/child_process.md +++ b/doc/api/child_process.md @@ -904,10 +904,11 @@ added: v0.5.10 --> * {boolean} Set to `true` after `subprocess.kill()` is used to successfully - terminate the child process. + send a signal to the child process. -The `subprocess.killed` property indicates whether the child process was -successfully terminated using `subprocess.kill()`. +The `subprocess.killed` property indicates whether the child process +successfully received a signal from `subprocess.kill()`. The `killed` property +does not indicate that the child process has been terminated. ### subprocess.pid From c1bbf1c1d605527724723df48f435d995fcbd04c Mon Sep 17 00:00:00 2001 From: woj Date: Mon, 6 Nov 2017 15:24:50 +0000 Subject: [PATCH 219/241] test: replace common.fixtiresDir with fixtures.readKey() Use fixtures module in test/parallel/test-tls-js-stream.js. PR-URL: https://github.com/nodejs/node/pull/16817 Reviewed-By: Rich Trott Reviewed-By: James M Snell --- test/parallel/test-tls-js-stream.js | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/test/parallel/test-tls-js-stream.js b/test/parallel/test-tls-js-stream.js index 56f62aecfa450f..bfebb06f8727fe 100644 --- a/test/parallel/test-tls-js-stream.js +++ b/test/parallel/test-tls-js-stream.js @@ -1,13 +1,15 @@ 'use strict'; const common = require('../common'); + if (!common.hasCrypto) common.skip('missing crypto'); +const fixtures = require('../common/fixtures'); + const assert = require('assert'); -const tls = require('tls'); -const stream = require('stream'); -const fs = require('fs'); const net = require('net'); +const stream = require('stream'); +const tls = require('tls'); const connected = { client: 0, @@ -15,8 +17,8 @@ const connected = { }; const server = tls.createServer({ - key: fs.readFileSync(`${common.fixturesDir}/keys/agent1-key.pem`), - cert: fs.readFileSync(`${common.fixturesDir}/keys/agent1-cert.pem`) + key: fixtures.readKey('agent1-key.pem'), + cert: fixtures.readKey('agent1-cert.pem') }, function(c) { console.log('new client'); connected.server++; From 94032cf5f54ebc928c6541bb5632555f29c35e3e Mon Sep 17 00:00:00 2001 From: Adam Jeffery Date: Mon, 6 Nov 2017 15:44:56 +0000 Subject: [PATCH 220/241] test: add values to error message In test-require-extensions-main, include the values that caused the test to fail in the messages reporting the failure. PR-URL: https://github.com/nodejs/node/pull/16831 Reviewed-By: Rich Trott Reviewed-By: Gireesh Punathil --- test/parallel/test-require-extensions-main.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/parallel/test-require-extensions-main.js b/test/parallel/test-require-extensions-main.js index 40a857b8e63614..16fbad6cf7af6e 100644 --- a/test/parallel/test-require-extensions-main.js +++ b/test/parallel/test-require-extensions-main.js @@ -3,11 +3,11 @@ require('../common'); const assert = require('assert'); const fixtures = require('../common/fixtures'); -const fixturesRequire = - require(fixtures.path('require-bin', 'bin', 'req.js')); +const fixturesRequire = require(fixtures.path('require-bin', 'bin', 'req.js')); assert.strictEqual( fixturesRequire, '', - 'test-require-extensions-main failed to import fixture requirements' + 'test-require-extensions-main failed to import fixture requirements: ' + + fixturesRequire ); From 6bc0b3e933eee5122e313146711142cd0fcf9dae Mon Sep 17 00:00:00 2001 From: Adam Wegrzynek Date: Mon, 6 Nov 2017 16:11:12 +0100 Subject: [PATCH 221/241] test: improve assertion in test-require-dot Include the value that differed from the expected value in an assertion message in test-require-dot. PR-URL: https://github.com/nodejs/node/pull/16805 Reviewed-By: Colin Ihrig Reviewed-By: Gireesh Punathil Reviewed-By: Michael Dawson Reviewed-By: James M Snell Reviewed-By: Rich Trott --- test/parallel/test-require-dot.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-require-dot.js b/test/parallel/test-require-dot.js index e2202efec1df1a..002a7ac988f2a1 100644 --- a/test/parallel/test-require-dot.js +++ b/test/parallel/test-require-dot.js @@ -14,5 +14,8 @@ process.env.NODE_PATH = fixtures.path('module-require', 'relative'); m._initPaths(); const c = require('.'); - -assert.strictEqual(c.value, 42, 'require(".") should honor NODE_PATH'); +assert.strictEqual( + c.value, + 42, + `require(".") should honor NODE_PATH; expected 42, found ${c.value}` +); From 5e4de71eed4aced22d85ebe12df2f01cc046d28a Mon Sep 17 00:00:00 2001 From: Katie Stockton Roberts Date: Wed, 8 Nov 2017 12:28:47 +0000 Subject: [PATCH 222/241] test: improve assert messages in stream test In test-stream-pipe-await-train-manual-resume, include unexpected value in error messages. PR-URL: https://github.com/nodejs/node/pull/16884 Reviewed-By: Anna Henningsen Reviewed-By: Gireesh Punathil Reviewed-By: Rich Trott --- .../test-stream-pipe-await-drain-manual-resume.js | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/test/parallel/test-stream-pipe-await-drain-manual-resume.js b/test/parallel/test-stream-pipe-await-drain-manual-resume.js index 96360429e58856..68fe95e8a4a3f7 100644 --- a/test/parallel/test-stream-pipe-await-drain-manual-resume.js +++ b/test/parallel/test-stream-pipe-await-drain-manual-resume.js @@ -30,11 +30,13 @@ readable.once('pause', common.mustCall(() => { assert.strictEqual( readable._readableState.awaitDrain, 1, - 'awaitDrain doesn\'t increase' + 'Expected awaitDrain to equal 1 but instead got ' + + `${readable._readableState.awaitDrain}` ); // First pause, resume manually. The next write() to writable will still // return false, because chunks are still being buffered, so it will increase // the awaitDrain counter again. + process.nextTick(common.mustCall(() => { readable.resume(); })); @@ -43,7 +45,8 @@ readable.once('pause', common.mustCall(() => { assert.strictEqual( readable._readableState.awaitDrain, 1, - '.resume() does not reset counter' + '.resume() should not reset the counter but instead got ' + + `${readable._readableState.awaitDrain}` ); // Second pause, handle all chunks from now on. Once all callbacks that // are currently queued up are handled, the awaitDrain drain counter should @@ -64,7 +67,8 @@ writable.on('finish', common.mustCall(() => { assert.strictEqual( readable._readableState.awaitDrain, 0, - 'awaitDrain not 0 after all chunks are written' + 'awaitDrain should equal 0 after all chunks are written but instead got' + + `${readable._readableState.awaitDrain}` ); // Everything okay, all chunks were written. })); From 1fa1ad2dd09f50f8fea266fd4d16b8d8e5deddce Mon Sep 17 00:00:00 2001 From: Franziska Hinkelmann Date: Tue, 7 Nov 2017 18:22:23 +0100 Subject: [PATCH 223/241] doc: improve documentation for the vm module Add an intro section and example for the vm module. PR-URL: https://github.com/nodejs/node/pull/16867 Reviewed-By: Colin Ihrig Reviewed-By: Gireesh Punathil Reviewed-By: Luigi Pinca --- doc/api/vm.md | 32 ++++++++++++++++++++++++++++---- 1 file changed, 28 insertions(+), 4 deletions(-) diff --git a/doc/api/vm.md b/doc/api/vm.md index 592c78148e7040..408a2f4fdfbebb 100644 --- a/doc/api/vm.md +++ b/doc/api/vm.md @@ -7,14 +7,38 @@ The `vm` module provides APIs for compiling and running code within V8 Virtual -Machine contexts. It can be accessed using: +Machine contexts. + +JavaScript code can be compiled and run immediately or +compiled, saved, and run later. + +A common use case is to run the code in a sandboxed environment. +The sandboxed code uses a different V8 Context, meaning that +it has a different global object than the rest of the code. + +One can provide the context by ["contextifying"][contextified] a sandbox +object. The sandboxed code treats any property on the sandbox like a +global variable. Any changes on global variables caused by the sandboxed +code are reflected in the sandbox object. ```js const vm = require('vm'); -``` -JavaScript code can be compiled and run immediately or compiled, saved, and run -later. +const x = 1; + +const sandbox = { x: 2 }; +vm.createContext(sandbox); // Contextify the sandbox. + +const code = 'x += 40; var y = 17;'; +// x and y are global variables in the sandboxed environment. +// Initially, x has the value 2 because that is the value of sandbox.x. +vm.runInContext(code, sandbox); + +console.log(sandbox.x); // 42 +console.log(sandbox.y); // 17 + +console.log(x); // 1; y is not defined. +``` *Note*: The vm module is not a security mechanism. **Do not use it to run untrusted code**. From bbd1cc2be9d4637f3193b7bc061a6c880d4cd659 Mon Sep 17 00:00:00 2001 From: Neil Vass Date: Wed, 8 Nov 2017 12:48:01 +0000 Subject: [PATCH 224/241] test: improve assertion messages Print content of domain stack if it doesn't match expected values PR-URL: https://github.com/nodejs/node/pull/16885 Reviewed-By: James M Snell Reviewed-By: Rich Trott Reviewed-By: Luigi Pinca Reviewed-By: Gireesh Punathil --- test/parallel/test-domain-safe-exit.js | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/test/parallel/test-domain-safe-exit.js b/test/parallel/test-domain-safe-exit.js index bbc3b2fe22cae8..bf8b9faaa424ce 100644 --- a/test/parallel/test-domain-safe-exit.js +++ b/test/parallel/test-domain-safe-exit.js @@ -1,16 +1,19 @@ 'use strict'; +require('../common'); // Make sure the domain stack doesn't get clobbered by un-matched .exit() -require('../common'); const assert = require('assert'); const domain = require('domain'); +const util = require('util'); const a = domain.create(); const b = domain.create(); a.enter(); // push b.enter(); // push -assert.deepStrictEqual(domain._stack, [a, b], 'b not pushed'); +assert.deepStrictEqual(domain._stack, [a, b], 'Unexpected stack shape ' + + `(domain._stack = ${util.inspect(domain._stack)})`); domain.create().exit(); // no-op -assert.deepStrictEqual(domain._stack, [a, b], 'stack mangled!'); +assert.deepStrictEqual(domain._stack, [a, b], 'Unexpected stack shape ' + + `(domain._stack = ${util.inspect(domain._stack)})`); From 14c119c331fc7976c0a6ae92e4d28aa2c232175f Mon Sep 17 00:00:00 2001 From: cjihrig Date: Thu, 9 Nov 2017 15:36:04 -0500 Subject: [PATCH 225/241] test: cover vm.runInNewContext() There is currently one if branch missing coverage in lib/vm.js. This commit adds a test to cover the case where the third argument to runInNewContext() is a string. PR-URL: https://github.com/nodejs/node/pull/16906 Reviewed-By: Ben Noordhuis Reviewed-By: Luigi Pinca Reviewed-By: Yuta Hiroto Reviewed-By: James M Snell Reviewed-By: Franziska Hinkelmann --- test/parallel/test-vm-run-in-new-context.js | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/test/parallel/test-vm-run-in-new-context.js b/test/parallel/test-vm-run-in-new-context.js index 73c1eccd937295..f3b70b3931050a 100644 --- a/test/parallel/test-vm-run-in-new-context.js +++ b/test/parallel/test-vm-run-in-new-context.js @@ -52,3 +52,23 @@ const fn = vm.runInNewContext('(function() { obj.p = {}; })', { obj: {} }); global.gc(); fn(); // Should not crash + +{ + // Verify that providing a custom filename as a string argument works. + const code = 'throw new Error("foo");'; + const file = 'test_file.vm'; + + assert.throws(() => { + vm.runInNewContext(code, {}, file); + }, (err) => { + const lines = err.stack.split('\n'); + + assert.strictEqual(lines[0].trim(), `${file}:1`); + assert.strictEqual(lines[1].trim(), code); + // Skip lines[2] and lines[3]. They're just a ^ and blank line. + assert.strictEqual(lines[4].trim(), 'Error: foo'); + assert.strictEqual(lines[5].trim(), `at ${file}:1:7`); + // The rest of the stack is uninteresting. + return true; + }); +} From 27f5dd69116d022bbb647ec2883bfbf965076c74 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Tue, 7 Nov 2017 19:28:06 +0800 Subject: [PATCH 226/241] src: make StreamBase prototype accessors robust This PR makes the prototype accessors added by StreamBase::AddMethods nonenumerable and checks the signatures in the accessors so they throw instead of raising assertions when called with incompatible receivers. They could be enumerated when inspecting the prototype with util.inspect or the inspector protocol. PR-URL: https://github.com/nodejs/node/pull/16860 Reviewed-By: Anna Henningsen Reviewed-By: Ben Noordhuis Reviewed-By: James M Snell Reviewed-By: Colin Ihrig --- src/stream_base-inl.h | 15 ++++++++--- ...-base-prototype-accessors-enumerability.js | 19 +++++++++++++ .../test-stream-base-prototype-accessors.js | 27 +++++++++++++++++++ 3 files changed, 57 insertions(+), 4 deletions(-) create mode 100644 test/parallel/test-stream-base-prototype-accessors-enumerability.js create mode 100644 test/parallel/test-stream-base-prototype-accessors.js diff --git a/src/stream_base-inl.h b/src/stream_base-inl.h index 23a3f6e8cd7c1c..6d857a74ddfdbd 100644 --- a/src/stream_base-inl.h +++ b/src/stream_base-inl.h @@ -11,6 +11,7 @@ namespace node { +using v8::AccessorSignature; using v8::External; using v8::FunctionCallbackInfo; using v8::FunctionTemplate; @@ -29,27 +30,33 @@ void StreamBase::AddMethods(Environment* env, HandleScope scope(env->isolate()); enum PropertyAttribute attributes = - static_cast(v8::ReadOnly | v8::DontDelete); + static_cast( + v8::ReadOnly | v8::DontDelete | v8::DontEnum); + Local signature = + AccessorSignature::New(env->isolate(), t); t->PrototypeTemplate()->SetAccessor(env->fd_string(), GetFD, nullptr, env->as_external(), v8::DEFAULT, - attributes); + attributes, + signature); t->PrototypeTemplate()->SetAccessor(env->external_stream_string(), GetExternal, nullptr, env->as_external(), v8::DEFAULT, - attributes); + attributes, + signature); t->PrototypeTemplate()->SetAccessor(env->bytes_read_string(), GetBytesRead, nullptr, env->as_external(), v8::DEFAULT, - attributes); + attributes, + signature); env->SetProtoMethod(t, "readStart", JSMethod); env->SetProtoMethod(t, "readStop", JSMethod); diff --git a/test/parallel/test-stream-base-prototype-accessors-enumerability.js b/test/parallel/test-stream-base-prototype-accessors-enumerability.js new file mode 100644 index 00000000000000..f59aced197c515 --- /dev/null +++ b/test/parallel/test-stream-base-prototype-accessors-enumerability.js @@ -0,0 +1,19 @@ +'use strict'; + +require('../common'); + +// This tests that the prototype accessors added by StreamBase::AddMethods +// are not enumerable. They could be enumerated when inspecting the prototype +// with util.inspect or the inspector protocol. + +const assert = require('assert'); + +// Or anything that calls StreamBase::AddMethods when setting up its prototype +const TTY = process.binding('tty_wrap').TTY; + +{ + assert.strictEqual(TTY.prototype.propertyIsEnumerable('bytesRead'), false); + assert.strictEqual(TTY.prototype.propertyIsEnumerable('fd'), false); + assert.strictEqual( + TTY.prototype.propertyIsEnumerable('_externalStream'), false); +} diff --git a/test/parallel/test-stream-base-prototype-accessors.js b/test/parallel/test-stream-base-prototype-accessors.js new file mode 100644 index 00000000000000..f9e12582a098d8 --- /dev/null +++ b/test/parallel/test-stream-base-prototype-accessors.js @@ -0,0 +1,27 @@ +'use strict'; + +require('../common'); + +// This tests that the prototype accessors added by StreamBase::AddMethods +// do not raise assersions when called with incompatible receivers. + +const assert = require('assert'); + +// Or anything that calls StreamBase::AddMethods when setting up its prototype +const TTY = process.binding('tty_wrap').TTY; + +// Should throw instead of raise assertions +{ + const msg = /TypeError: Method \w+ called on incompatible receiver/; + assert.throws(() => { + TTY.prototype.bytesRead; + }, msg); + + assert.throws(() => { + TTY.prototype.fd; + }, msg); + + assert.throws(() => { + TTY.prototype._externalStream; + }, msg); +} From 0f85885ae41af8decde0e2bcad8f68342fae7afa Mon Sep 17 00:00:00 2001 From: Tanvi Kini Date: Fri, 10 Nov 2017 05:22:12 +0000 Subject: [PATCH 227/241] test: replace string concatenation with template MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR-URL: https://github.com/nodejs/node/pull/16913 Reviewed-By: Luigi Pinca Reviewed-By: James M Snell Reviewed-By: Tobias Nießen Reviewed-By: Gireesh Punathil Reviewed-By: Colin Ihrig Reviewed-By: Franziska Hinkelmann --- test/fixtures/guess-hash-seed.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/fixtures/guess-hash-seed.js b/test/fixtures/guess-hash-seed.js index c0d072b23d2e48..18a6f5124dc1d8 100644 --- a/test/fixtures/guess-hash-seed.js +++ b/test/fixtures/guess-hash-seed.js @@ -134,7 +134,7 @@ const slow_str_gen = (function*() { let strgen_i = 0; outer: while (1) { - const str = '#' + (strgen_i++); + const str = `#${strgen_i++}`; for (let i = 0; i < 1000; i++) { if (time_set_lookup(tester_set, str) < tester_set_treshold) continue outer; From b593d5e9c977af1128285cc786eaf90c3d743a92 Mon Sep 17 00:00:00 2001 From: Vipin Menon Date: Fri, 10 Nov 2017 12:00:09 +0530 Subject: [PATCH 228/241] test: enable mustCall() during child exit PR-URL: https://github.com/nodejs/node/pull/16915 Reviewed-By: Khaidi Chu Reviewed-By: Gireesh Punathil Reviewed-By: James M Snell Reviewed-By: Colin Ihrig Reviewed-By: Anatoli Papirovski Reviewed-By: Franziska Hinkelmann --- test/parallel/test-process-raw-debug.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/parallel/test-process-raw-debug.js b/test/parallel/test-process-raw-debug.js index 23163654d88c79..b9dbb8a2de9490 100644 --- a/test/parallel/test-process-raw-debug.js +++ b/test/parallel/test-process-raw-debug.js @@ -1,5 +1,5 @@ 'use strict'; -require('../common'); +const common = require('../common'); const assert = require('assert'); const os = require('os'); @@ -29,10 +29,10 @@ function parent() { console.log('ok - got expected message'); }); - child.on('exit', function(c) { + child.on('exit', common.mustCall(function(c) { assert(!c); console.log('ok - child exited nicely'); - }); + })); } function child() { From 67a058633232701d90ee614028e5bd2f48e7ad1d Mon Sep 17 00:00:00 2001 From: Kabir Islam Date: Fri, 10 Nov 2017 12:20:28 +0530 Subject: [PATCH 229/241] test: replace string concatenation with template MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR-URL: https://github.com/nodejs/node/pull/16916 Reviewed-By: Vse Mozhet Byt Reviewed-By: James M Snell Reviewed-By: Daniel Bevenius Reviewed-By: Tobias Nießen Reviewed-By: Gireesh Punathil Reviewed-By: Colin Ihrig Reviewed-By: Franziska Hinkelmann --- test/fixtures/cluster-preload-test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/fixtures/cluster-preload-test.js b/test/fixtures/cluster-preload-test.js index 43d3887b0daf01..1bc6c29fb7aa5e 100644 --- a/test/fixtures/cluster-preload-test.js +++ b/test/fixtures/cluster-preload-test.js @@ -2,6 +2,6 @@ var cluster = require('cluster'); if (cluster.isMaster) { cluster.fork(); // one child cluster.on('exit', function(worker, code, signal) { - console.log('worker terminated with code ' + code); + console.log(`worker terminated with code ${code}`); }); } From c4292eaf254cfc6a9394a3e319803cbc38fef232 Mon Sep 17 00:00:00 2001 From: Suryanarayana Murthy N Date: Fri, 10 Nov 2017 13:36:00 +0530 Subject: [PATCH 230/241] test: change string concatenation to template MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR-URL: https://github.com/nodejs/node/pull/16919 Reviewed-By: James M Snell Reviewed-By: Tobias Nießen Reviewed-By: Colin Ihrig Reviewed-By: Gireesh Punathil Reviewed-By: Franziska Hinkelmann --- test/fixtures/print-10-lines.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/fixtures/print-10-lines.js b/test/fixtures/print-10-lines.js index 0aaf3ef293000e..8971e0c5bbcff8 100644 --- a/test/fixtures/print-10-lines.js +++ b/test/fixtures/print-10-lines.js @@ -1,3 +1,3 @@ for (var i = 0; i < 10; i++) { - console.log('count ' + i); + console.log(`count ${i}`); } From 91e2ed4b742b46e9564e22d66d360128ff0520d6 Mon Sep 17 00:00:00 2001 From: Anawesha Khuntia Date: Fri, 10 Nov 2017 10:54:32 +0530 Subject: [PATCH 231/241] test: change concatenated string to template MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR-URL: https://github.com/nodejs/node/pull/16912 Reviewed-By: Gireesh Punathil Reviewed-By: Vse Mozhet Byt Reviewed-By: James M Snell Reviewed-By: Tobias Nießen Reviewed-By: Colin Ihrig Reviewed-By: Franziska Hinkelmann --- test/fixtures/loop.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/fixtures/loop.js b/test/fixtures/loop.js index e91b831d01b816..461fb393583e68 100644 --- a/test/fixtures/loop.js +++ b/test/fixtures/loop.js @@ -4,7 +4,7 @@ console.log('A message', 5); while (t > 0) { if (t++ === 1000) { t = 0; - console.log('Outputed message #' + k++); + console.log(`Outputed message #${k++}`); } } process.exit(55); From 66387d13b2569ada3ca54670d3c7c51ec89ef6e3 Mon Sep 17 00:00:00 2001 From: Deepthi Sebastian Date: Fri, 10 Nov 2017 17:57:00 +0530 Subject: [PATCH 232/241] test: change concatenated string to template MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR-URL: https://github.com/nodejs/node/pull/16929 Reviewed-By: Vse Mozhet Byt Reviewed-By: Tobias Nießen Reviewed-By: Colin Ihrig Reviewed-By: James M Snell Reviewed-By: Anatoli Papirovski Reviewed-By: Gireesh Punathil Reviewed-By: Franziska Hinkelmann --- test/fixtures/net-fd-passing-receiver.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/fixtures/net-fd-passing-receiver.js b/test/fixtures/net-fd-passing-receiver.js index 0dba242766a62b..fb4faee1264464 100644 --- a/test/fixtures/net-fd-passing-receiver.js +++ b/test/fixtures/net-fd-passing-receiver.js @@ -15,12 +15,12 @@ receiver = net.createServer(function(socket) { }); passedSocket.on('data', function(data) { - passedSocket.send('[echo] ' + data); + passedSocket.send(`[echo] ${data}`); }); passedSocket.on('close', function() { receiver.close(); }); - passedSocket.send('[greeting] ' + greeting); + passedSocket.send(`[greeting] ${greeting}`); }); }); From 50e226deebd08ccfd2662b279b47fb26bd7af61b Mon Sep 17 00:00:00 2001 From: Stephan Smith Date: Mon, 6 Nov 2017 15:42:01 +0000 Subject: [PATCH 233/241] test: improve template value for test message Include value that cause failure in error message in test-cluster-master-kill.js. PR-URL: https://github.com/nodejs/node/pull/16826 Reviewed-By: Gireesh Punathil Reviewed-By: James M Snell Reviewed-By: Rich Trott --- test/parallel/test-cluster-master-kill.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/parallel/test-cluster-master-kill.js b/test/parallel/test-cluster-master-kill.js index 98ed0d22aa9cfd..6ad8eb9e43f727 100644 --- a/test/parallel/test-cluster-master-kill.js +++ b/test/parallel/test-cluster-master-kill.js @@ -60,7 +60,8 @@ if (cluster.isWorker) { process.once('exit', () => { assert.strictEqual(typeof pid, 'number', `got ${pid} instead of a worker pid`); - assert.strictEqual(alive, false, 'worker was alive after master died'); + assert.strictEqual(alive, false, + `worker was alive after master died (alive = ${alive})`); }); } From 3d83c637eda16a1b11b8373c80056671ca367914 Mon Sep 17 00:00:00 2001 From: Javier Blanco Date: Mon, 6 Nov 2017 15:13:30 +0000 Subject: [PATCH 234/241] test: use common/fixtures module in hash-seed test Replace `common.fixturesDir` with `fixtures.path()` usage in test/pummel/test-hash-seed.js. PR-URL: https://github.com/nodejs/node/pull/16823 Reviewed-By: Joyee Cheung Reviewed-By: Gireesh Punathil Reviewed-By: James M Snell --- test/pummel/test-hash-seed.js | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/test/pummel/test-hash-seed.js b/test/pummel/test-hash-seed.js index 05d29fff0b8b90..fd59bbe5e0653c 100644 --- a/test/pummel/test-hash-seed.js +++ b/test/pummel/test-hash-seed.js @@ -1,20 +1,20 @@ 'use strict'; +// Check that spawn child doesn't create duplicated entries +require('../common'); const REPETITIONS = 2; - const assert = require('assert'); -const common = require('../common'); -const cp = require('child_process'); -const path = require('path'); -const targetScript = path.resolve(common.fixturesDir, 'guess-hash-seed.js'); +const fixtures = require('../common/fixtures'); +const { spawnSync } = require('child_process'); +const targetScript = fixtures.path('guess-hash-seed.js'); const seeds = []; for (let i = 0; i < REPETITIONS; ++i) { - const seed = cp.spawnSync(process.execPath, [targetScript], - { encoding: 'utf8' }).stdout.trim(); + const seed = spawnSync(process.execPath, [targetScript], { + encoding: 'utf8' + }).stdout.trim(); seeds.push(seed); } console.log(`Seeds: ${seeds}`); -const hasDuplicates = (new Set(seeds)).size !== seeds.length; -assert.strictEqual(hasDuplicates, false); +assert.strictEqual(new Set(seeds).size, seeds.length); From 3755a0dc21fe6b876509525f926efbbda959965d Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Sun, 1 Oct 2017 08:33:55 -0700 Subject: [PATCH 235/241] doc: reorganize COLLABORATOR_GUIDE.md Based on feedback during a recent collaborator onboarding, move the metadata description closer to where the instructions for editing a commit message are. Indicate which metadata are required and which are optional. Make the punctuation more consistent. Previously, sentences prior to instructions ended in a period, a colon, or no punctuation at all. Colon seems best, so changed the Technical How-To section to use colons. PR-URL: https://github.com/nodejs/node/pull/15710 Reviewed-By: Vse Mozhet Byt Reviewed-By: Benedikt Meurer Reviewed-By: Luigi Pinca Reviewed-By: James M Snell Reviewed-By: Refael Ackermann --- COLLABORATOR_GUIDE.md | 60 +++++++++++++++++++++++-------------------- doc/onboarding.md | 2 +- 2 files changed, 33 insertions(+), 29 deletions(-) diff --git a/COLLABORATOR_GUIDE.md b/COLLABORATOR_GUIDE.md index 3e1b058ac57c75..9788e9f05f72a4 100644 --- a/COLLABORATOR_GUIDE.md +++ b/COLLABORATOR_GUIDE.md @@ -365,25 +365,11 @@ The TSC should serve as the final arbiter where required. * If more than one author has contributed to the PR, keep the most recent author when squashing. -Always modify the original commit message to include additional meta -information regarding the change process: - -- A `PR-URL:` line that references the *full* GitHub URL of the original - pull request being merged so it's easy to trace a commit back to the - conversation that led up to that change. -- A `Fixes: X` line, where _X_ either includes the *full* GitHub URL - for an issue, and/or the hash and commit message if the commit fixes - a bug in a previous commit. Multiple `Fixes:` lines may be added if - appropriate. -- A `Refs:` line referencing a URL for any relevant background. -- A `Reviewed-By: Name ` line for yourself and any - other Collaborators who have reviewed the change. - - Useful for @mentions / contact list if something goes wrong in the PR. - - Protects against the assumption that GitHub will be around forever. - Review the commit message to ensure that it adheres to the guidelines outlined in the [contributing](./CONTRIBUTING.md#commit-message-guidelines) guide. +Add all necessary [metadata](#metadata) to commit messages before landing. + See the commit log for examples such as [this one](https://github.com/nodejs/node/commit/b636ba8186) if unsure exactly how to format your commit messages. @@ -391,34 +377,33 @@ exactly how to format your commit messages. Additionally: - Double check PRs to make sure the person's _full name_ and email address are correct before merging. -- Except when updating dependencies, all commits should be self - contained (meaning every commit should pass all tests). This makes - it much easier when bisecting to find a breaking change. +- All commits should be self-contained (meaning every commit should pass all + tests). This makes it much easier when bisecting to find a breaking change. ### Technical HOWTO -Clear any `am`/`rebase` that may already be underway. +Clear any `am`/`rebase` that may already be underway: ```text $ git am --abort $ git rebase --abort ``` -Checkout proper target branch +Checkout proper target branch: ```text $ git checkout master ``` Update the tree (assumes your repo is set up as detailed in -[CONTRIBUTING.md](CONTRIBUTING.md#step-1-fork)) +[CONTRIBUTING.md](CONTRIBUTING.md#step-1-fork)): ```text $ git fetch upstream $ git merge --ff-only upstream/master ``` -Apply external patches +Apply external patches: ```text $ curl -L https://github.com/nodejs/node/pull/xxx.patch | git am --whitespace=fix @@ -436,21 +421,19 @@ against the original PR carefully and build/test on at least one platform before landing. If the 3-way merge fails, then it is most likely that a conflicting PR has landed since the CI run and you will have to ask the author to rebase. -Check and re-review the changes +Check and re-review the changes: ```text $ git diff upstream/master ``` -Check number of commits and commit messages +Check number of commits and commit messages: ```text $ git log upstream/master...master ``` -If there are multiple commits that relate to the same feature or -one with a feature and separate with a test for that feature, -you'll need to use `squash` or `fixup`: +Squash commits and add metadata: ```text $ git rebase -i upstream/master @@ -506,9 +489,29 @@ Save the file and close the editor. You'll be asked to enter a new commit message for that commit. This is a good moment to fix incorrect commit logs, ensure that they are properly formatted, and add `Reviewed-By` lines. + * The commit message text must conform to the [commit message guidelines](./CONTRIBUTING.md#commit-message-guidelines). + +* Modify the original commit message to include additional metadata regarding + the change process. (If you use Chrome or Edge, [`node-review`][] fetches + the metadata for you.) + + * Required: A `PR-URL:` line that references the *full* GitHub URL of the + original pull request being merged so it's easy to trace a commit back to + the conversation that led up to that change. + * Optional: A `Fixes: X` line, where _X_ either includes the *full* GitHub URL + for an issue, and/or the hash and commit message if the commit fixes + a bug in a previous commit. Multiple `Fixes:` lines may be added if + appropriate. + * Optional: One or more `Refs:` lines referencing a URL for any relevant + background. + * Required: A `Reviewed-By: Name ` line for yourself and any + other Collaborators who have reviewed the change. + * Useful for @mentions / contact list if something goes wrong in the PR. + * Protects against the assumption that GitHub will be around forever. + Run tests (`make -j4 test` or `vcbuild test`). Even though there was a successful continuous integration run, other changes may have landed on master since then, so running the tests one last time locally is a good practice. @@ -672,3 +675,4 @@ LTS working group and the Release team. [Stability Index]: doc/api/documentation.md#stability-index [Enhancement Proposal]: https://github.com/nodejs/node-eps [git-username]: https://help.github.com/articles/setting-your-username-in-git/ +[`node-review`]: https://github.com/evanlucas/node-review diff --git a/doc/onboarding.md b/doc/onboarding.md index d028803aa0ef80..41df0d00bd93b3 100644 --- a/doc/onboarding.md +++ b/doc/onboarding.md @@ -158,7 +158,7 @@ onboarding session. * After one or two approvals, land the PR. * Be sure to add the `PR-URL: ` and appropriate `Reviewed-By:` metadata! * [`core-validate-commit`][] helps a lot with this – install and use it if you can! - * If you use Chrome, [`node-review`][] fetches the metadata for you + * If you use Chrome or Edge, [`node-review`][] fetches the metadata for you. ## Final notes From 340b6e243ce4421148f7528371561b66ffb27c62 Mon Sep 17 00:00:00 2001 From: Grant Gasparyan Date: Mon, 6 Nov 2017 17:37:28 +0100 Subject: [PATCH 236/241] test: add a test description PR-URL: https://github.com/nodejs/node/pull/16833 Reviewed-By: James M Snell Reviewed-By: Rich Trott Reviewed-By: Vse Mozhet Byt Reviewed-By: Gireesh Punathil --- test/parallel/test-cluster-fork-env.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/parallel/test-cluster-fork-env.js b/test/parallel/test-cluster-fork-env.js index 112aeb087c49a1..faa8b954e4443c 100644 --- a/test/parallel/test-cluster-fork-env.js +++ b/test/parallel/test-cluster-fork-env.js @@ -1,5 +1,10 @@ 'use strict'; require('../common'); + +// This test checks that arguments provided to cluster.fork() will create +// new environment variables and override existing environment variables +// in the created worker process. + const assert = require('assert'); const cluster = require('cluster'); From b60ac4fe992634bf7fd4933c96b76a5dcd729a2b Mon Sep 17 00:00:00 2001 From: Klemen Kogovsek Date: Mon, 6 Nov 2017 15:05:59 +0000 Subject: [PATCH 237/241] test: used fixturesDir from fixtures modules In test-fs-realpath-on-substed-drive, require common/fixtures module and swapped the location of fixturesDir from common to fixtures module. PR-URL: https://github.com/nodejs/node/pull/16813 Reviewed-By: James M Snell Reviewed-By: Rich Trott Reviewed-By: Gireesh Punathil --- test/parallel/test-fs-realpath-on-substed-drive.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/parallel/test-fs-realpath-on-substed-drive.js b/test/parallel/test-fs-realpath-on-substed-drive.js index 7a871b7e59807c..88fa04c2ced646 100644 --- a/test/parallel/test-fs-realpath-on-substed-drive.js +++ b/test/parallel/test-fs-realpath-on-substed-drive.js @@ -4,6 +4,8 @@ const common = require('../common'); if (!common.isWindows) common.skip('Test for Windows only'); +const fixtures = require('../common/fixtures'); + const assert = require('assert'); const fs = require('fs'); const spawnSync = require('child_process').spawnSync; @@ -16,7 +18,7 @@ let drive; let i; for (i = 0; i < driveLetters.length; ++i) { drive = `${driveLetters[i]}:`; - result = spawnSync('subst', [drive, common.fixturesDir]); + result = spawnSync('subst', [drive, fixtures.fixturesDir]); if (result.status === 0) break; } From d1761e4b34076567c729b3adf74556914e5c62b4 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Sun, 12 Nov 2017 16:29:27 -0800 Subject: [PATCH 238/241] doc: recommend node-core-utils for metadata MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR-URL: https://github.com/nodejs/node/pull/16978 Reviewed-By: Gireesh Punathil Reviewed-By: Luigi Pinca Reviewed-By: Khaidi Chu Reviewed-By: Colin Ihrig Reviewed-By: Tobias Nießen Reviewed-By: Anatoli Papirovski Reviewed-By: James M Snell --- COLLABORATOR_GUIDE.md | 5 ++--- doc/onboarding.md | 4 ++-- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/COLLABORATOR_GUIDE.md b/COLLABORATOR_GUIDE.md index 9788e9f05f72a4..8f84f6f9f78baa 100644 --- a/COLLABORATOR_GUIDE.md +++ b/COLLABORATOR_GUIDE.md @@ -495,8 +495,7 @@ commit logs, ensure that they are properly formatted, and add * Modify the original commit message to include additional metadata regarding - the change process. (If you use Chrome or Edge, [`node-review`][] fetches - the metadata for you.) + the change process. ([`node-core-utils`][] fetches the metadata for you.) * Required: A `PR-URL:` line that references the *full* GitHub URL of the original pull request being merged so it's easy to trace a commit back to @@ -675,4 +674,4 @@ LTS working group and the Release team. [Stability Index]: doc/api/documentation.md#stability-index [Enhancement Proposal]: https://github.com/nodejs/node-eps [git-username]: https://help.github.com/articles/setting-your-username-in-git/ -[`node-review`]: https://github.com/evanlucas/node-review +[`node-core-utils`]: https://github.com/nodejs/node-core-utils diff --git a/doc/onboarding.md b/doc/onboarding.md index 41df0d00bd93b3..09cbb432a2bb85 100644 --- a/doc/onboarding.md +++ b/doc/onboarding.md @@ -158,7 +158,7 @@ onboarding session. * After one or two approvals, land the PR. * Be sure to add the `PR-URL: ` and appropriate `Reviewed-By:` metadata! * [`core-validate-commit`][] helps a lot with this – install and use it if you can! - * If you use Chrome or Edge, [`node-review`][] fetches the metadata for you. + * [`node-core-utils`][] fetches the metadata for you. ## Final notes @@ -180,4 +180,4 @@ onboarding session. [Code of Conduct]: https://github.com/nodejs/TSC/blob/master/CODE_OF_CONDUCT.md [`core-validate-commit`]: https://github.com/evanlucas/core-validate-commit -[`node-review`]: https://github.com/evanlucas/node-review +[`node-core-utils`]: https://github.com/nodejs/node-core-utils From 2c7ad2fa0239b8421f54a897568a1fcb49960a3c Mon Sep 17 00:00:00 2001 From: ChrBergert Date: Mon, 6 Nov 2017 16:04:36 +0000 Subject: [PATCH 239/241] test: refactor comments in test-child-process-spawnsync-maxbuf * remove comment that isn't relevant/important * add comment that explains what the test does PR-URL: https://github.com/nodejs/node/pull/16829 Reviewed-By: Rich Trott Reviewed-By: Joyee Cheung Reviewed-By: Luigi Pinca Reviewed-By: James M Snell --- test/parallel/test-child-process-spawnsync-maxbuf.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-child-process-spawnsync-maxbuf.js b/test/parallel/test-child-process-spawnsync-maxbuf.js index 665cfd11bd2224..276308b7502cbd 100644 --- a/test/parallel/test-child-process-spawnsync-maxbuf.js +++ b/test/parallel/test-child-process-spawnsync-maxbuf.js @@ -1,12 +1,14 @@ 'use strict'; require('../common'); + +// This test checks that the maxBuffer option for child_process.spawnSync() +// works as expected. + const assert = require('assert'); const spawnSync = require('child_process').spawnSync; const msgOut = 'this is stdout'; - -// This is actually not os.EOL? const msgOutBuf = Buffer.from(`${msgOut}\n`); const args = [ From 659c45856d3c7f50cb4cdcd6a9b4348e9af8c536 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= Date: Fri, 10 Nov 2017 18:27:35 +0100 Subject: [PATCH 240/241] test: reuse existing PassThrough implementation PR-URL: https://github.com/nodejs/node/pull/16936 Reviewed-By: Colin Ihrig Reviewed-By: Refael Ackermann Reviewed-By: Anna Henningsen Reviewed-By: James M Snell Reviewed-By: Luigi Pinca --- test/parallel/test-stream-big-packet.js | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/test/parallel/test-stream-big-packet.js b/test/parallel/test-stream-big-packet.js index 09ed825036f2e4..9a5828f1fa7230 100644 --- a/test/parallel/test-stream-big-packet.js +++ b/test/parallel/test-stream-big-packet.js @@ -5,13 +5,6 @@ const stream = require('stream'); let passed = false; -class PassThrough extends stream.Transform { - _transform(chunk, encoding, done) { - this.push(chunk); - done(); - } -} - class TestStream extends stream.Transform { _transform(chunk, encoding, done) { if (!passed) { @@ -22,8 +15,8 @@ class TestStream extends stream.Transform { } } -const s1 = new PassThrough(); -const s2 = new PassThrough(); +const s1 = new stream.PassThrough(); +const s2 = new stream.PassThrough(); const s3 = new TestStream(); s1.pipe(s3); // Don't let s2 auto close which may close s3 From 8d3e7e1854926bcaabceed4d7c00700157d30a74 Mon Sep 17 00:00:00 2001 From: Yihong Wang Date: Wed, 1 Nov 2017 21:19:16 -0700 Subject: [PATCH 241/241] build: remove cctest extension cctest has `so.59` extension when building node shared library in linux. The appending is defined in node.gypi and the cctest target in node.gyp includes node.gypi. Moving the appending from node.gypi to node target in node.gyp fixes the issue. Signed-off-by: Yihong Wang PR-URL: https://github.com/nodejs/node/pull/16680 Reviewed-By: Ben Noordhuis Reviewed-By: James M Snell Reviewed-By: Michael Dawson Reviewed-By: Colin Ihrig Reviewed-By: Gibson Fahnestock Reviewed-By: Gireesh Punathil Reviewed-By: Daniel Bevenius Reviewed-By: Refael Ackermann --- node.gyp | 5 +++++ node.gypi | 5 ----- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/node.gyp b/node.gyp index fa1b204ee6fb0b..345188257998e0 100644 --- a/node.gyp +++ b/node.gyp @@ -237,6 +237,11 @@ # Warn when using deprecated V8 APIs. 'V8_DEPRECATION_WARNINGS=1', ], + 'conditions': [ + [ 'node_shared=="true" and node_module_version!="" and OS!="win"', { + 'product_extension': '<(shlib_suffix)', + }] + ], }, { 'target_name': 'mkssldef', diff --git a/node.gypi b/node.gypi index 6aaabeed42aac1..3b3548fdab5308 100644 --- a/node.gypi +++ b/node.gypi @@ -11,11 +11,6 @@ 'defines': [ 'NODE_SHARED_MODE', ], - 'conditions': [ - [ 'node_module_version!="" and OS!="win"', { - 'product_extension': '<(shlib_suffix)', - }] - ], }], [ 'node_enable_d8=="true"', { 'dependencies': [ 'deps/v8/src/d8.gyp:d8' ],