From efb47cb620c4e9a679ae2d6a059092bc0ca237f2 Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Wed, 7 Nov 2018 16:18:03 -0800 Subject: [PATCH 1/2] test: simplify regression test for SEGV Test was introduced in 08a5b442e4 as a regression test, and has evolved since then. Simplify the test so that it doesn't rely on an undocumented argument to tls.createSecureCredentials(). See: https://github.com/nodejs/node-v0.x-archive/issues/6690 Confirmation that this reworked test triggers the original bug: %) % node > process.version 'v0.10.48' > credentials = crypto.createCredentials() { context: {} } > context = credentials.context {} > notcontext = { setOptions: context.setOptions } { setOptions: [Function: setOptions] } > notcontext.setOptions() node: ../src/node_object_wrap.h:61: static T* node::ObjectWrap::Unwrap(v8::Handle) [with T = node::crypto::SecureContext]: Assertion `handle->InternalFieldCount() > 0' failed. zsh: abort (core dumped) node --- test/parallel/test-crypto.js | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/test/parallel/test-crypto.js b/test/parallel/test-crypto.js index 1f02c07b02eb2f..21e2cca5bd3329 100644 --- a/test/parallel/test-crypto.js +++ b/test/parallel/test-crypto.js @@ -38,19 +38,19 @@ const tls = require('tls'); const fixtures = require('../common/fixtures'); // Test Certificates -const caPem = fixtures.readSync('test_ca.pem', 'ascii'); -const certPem = fixtures.readSync('test_cert.pem', 'ascii'); const certPfx = fixtures.readSync('test_cert.pfx'); -const keyPem = fixtures.readSync('test_key.pem', 'ascii'); // 'this' safety // https://github.com/joyent/node/issues/6690 assert.throws(function() { - const options = { key: keyPem, cert: certPem, ca: caPem }; - const credentials = tls.createSecureContext(options); + const credentials = tls.createSecureContext(); const context = credentials.context; - const notcontext = { setOptions: context.setOptions, setKey: context.setKey }; - tls.createSecureContext({ secureOptions: 1 }, notcontext); + const notcontext = { setOptions: context.setOptions }; + + // Methods of native objects should not segfault when reassigned to a new + // object and called illegally. This core dumped in 0.10 and was fixed in + // 0.11. + notcontext.setOptions(); }, (err) => { // Throws TypeError, so there is no opensslErrorStack property. if ((err instanceof Error) && From ae8029da9cf2f0bcdcc7571cc0b54165d2078b29 Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Wed, 7 Nov 2018 16:36:19 -0800 Subject: [PATCH 2/2] tls: remove unused arg to createSecureContext() The context arg is unused by node or its test suites and undocumented. Remove it. --- lib/_tls_common.js | 23 ++++++----------------- 1 file changed, 6 insertions(+), 17 deletions(-) diff --git a/lib/_tls_common.js b/lib/_tls_common.js index 2a880450677258..7153334a141777 100644 --- a/lib/_tls_common.js +++ b/lib/_tls_common.js @@ -35,22 +35,13 @@ const { SSL_OP_CIPHER_SERVER_PREFERENCE } = internalBinding('constants').crypto; let toBuf = null; const { SecureContext: NativeSecureContext } = internalBinding('crypto'); -function SecureContext(secureProtocol, secureOptions, context) { +function SecureContext(secureProtocol, secureOptions) { if (!(this instanceof SecureContext)) { - return new SecureContext(secureProtocol, secureOptions, context); + return new SecureContext(secureProtocol, secureOptions); } - if (context) { - this.context = context; - } else { - this.context = new NativeSecureContext(); - - if (secureProtocol) { - this.context.init(secureProtocol); - } else { - this.context.init(); - } - } + this.context = new NativeSecureContext(); + this.context.init(secureProtocol); if (secureOptions) this.context.setOptions(secureOptions); } @@ -68,19 +59,17 @@ function validateKeyCert(name, value) { exports.SecureContext = SecureContext; -exports.createSecureContext = function createSecureContext(options, context) { +exports.createSecureContext = function createSecureContext(options) { if (!options) options = {}; var secureOptions = options.secureOptions; if (options.honorCipherOrder) secureOptions |= SSL_OP_CIPHER_SERVER_PREFERENCE; - const c = new SecureContext(options.secureProtocol, secureOptions, context); + const c = new SecureContext(options.secureProtocol, secureOptions); var i; var val; - if (context) return c; - // NOTE: It's important to add CA before the cert to be able to load // cert's issuer in C++ code. const { ca } = options;