From 2d2a812645db794698abec602a10366e41305b98 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Sun, 26 Jul 2020 22:53:53 -0700 Subject: [PATCH 01/19] test: remove unneeded flag check in test-vm-memleak The `common` module checks that necessary flags are being used, so a check in the test itself is no longer necessary. PR-URL: https://github.com/nodejs/node/pull/34528 Reviewed-By: Ruben Bridgewater Reviewed-By: Luigi Pinca --- test/pummel/test-vm-memleak.js | 5 ----- 1 file changed, 5 deletions(-) diff --git a/test/pummel/test-vm-memleak.js b/test/pummel/test-vm-memleak.js index 94e4055a767096..33f2f3d75905a5 100644 --- a/test/pummel/test-vm-memleak.js +++ b/test/pummel/test-vm-memleak.js @@ -29,11 +29,6 @@ const vm = require('vm'); const start = Date.now(); let maxMem = 0; -const ok = process.execArgv.some(function(arg) { - return arg === '--max_old_space_size=32'; -}); -assert(ok, 'Run this test with --max_old_space_size=32.'); - const interval = setInterval(function() { try { vm.runInNewContext('throw 1;'); From dc00a07426f0b2dee9bfe39c766a703eb9b5b880 Mon Sep 17 00:00:00 2001 From: Gus Caplan Date: Thu, 30 Jul 2020 10:28:38 -0500 Subject: [PATCH 02/19] Revert "module: disable cjs snapshotting into esm loader" This reverts commit 1fe39f0b4bad8da38e5f02542c176d5999ad3ecb. PR-URL: https://github.com/nodejs/node/pull/34562 Reviewed-By: Bradley Farias Reviewed-By: Guy Bedford --- lib/internal/modules/cjs/loader.js | 32 +++++++++++++++++++++-- lib/internal/modules/esm/loader.js | 6 ++++- lib/internal/modules/esm/translators.js | 34 ++++++++++++++----------- test/es-module/test-esm-snapshot.mjs | 2 +- 4 files changed, 55 insertions(+), 19 deletions(-) diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index ab943657a133b4..de24f6c409c498 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -32,6 +32,7 @@ const { ArrayPrototypeJoin, Error, JSONParse, + Map, Number, ObjectCreate, ObjectDefineProperty, @@ -112,6 +113,8 @@ const { } = require('internal/util/types'); const asyncESM = require('internal/process/esm_loader'); +const ModuleJob = require('internal/modules/esm/module_job'); +const { ModuleWrap, kInstantiated } = internalBinding('module_wrap'); const { encodedSepRegEx, packageInternalResolve @@ -371,7 +374,7 @@ function tryPackage(requestPath, exts, isMain, originalPath) { // In order to minimize unnecessary lstat() calls, // this cache is a list of known-real paths. // Set to an empty Map to reset. -const realpathCache = new SafeMap(); +const realpathCache = new Map(); // Check if the file exists and is not a directory // if using --preserve-symlinks and isMain is false, @@ -1115,6 +1118,31 @@ Module.prototype.load = function(filename) { } Module._extensions[extension](this, filename); this.loaded = true; + + const ESMLoader = asyncESM.ESMLoader; + const url = `${pathToFileURL(filename)}`; + const module = ESMLoader.moduleMap.get(url); + // Create module entry at load time to snapshot exports correctly + const exports = this.exports; + // Called from cjs translator + if (module !== undefined && module.module !== undefined) { + if (module.module.getStatus() >= kInstantiated) + module.module.setExport('default', exports); + } else { + // Preemptively cache + // We use a function to defer promise creation for async hooks. + ESMLoader.moduleMap.set( + url, + // Module job creation will start promises. + // We make it a function to lazily trigger those promises + // for async hooks compatibility. + () => new ModuleJob(ESMLoader, url, () => + new ModuleWrap(url, undefined, ['default'], function() { + this.setExport('default', exports); + }) + , false /* isMain */, false /* inspectBrk */) + ); + } }; @@ -1234,7 +1262,7 @@ Module.prototype._compile = function(content, filename) { const exports = this.exports; const thisValue = exports; const module = this; - if (requireDepth === 0) statCache = new SafeMap(); + if (requireDepth === 0) statCache = new Map(); if (inspectorWrapper) { result = inspectorWrapper(compiledWrapper, thisValue, exports, require, module, filename, dirname); diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index 62b935de942366..37191f65bf0b7e 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -5,7 +5,8 @@ require('internal/modules/cjs/loader'); const { FunctionPrototypeBind, - ObjectSetPrototypeOf + ObjectSetPrototypeOf, + SafeMap, } = primordials; const { @@ -45,6 +46,9 @@ class Loader { // Registry of loaded modules, akin to `require.cache` this.moduleMap = new ModuleMap(); + // Map of already-loaded CJS modules to use + this.cjsCache = new SafeMap(); + // This hook is called before the first root module is imported. It's a // function that returns a piece of code that runs as a sloppy-mode script. // The script may evaluate to a function that can be called with a diff --git a/lib/internal/modules/esm/translators.js b/lib/internal/modules/esm/translators.js index 45abbae41193f4..bb3528eddde964 100644 --- a/lib/internal/modules/esm/translators.js +++ b/lib/internal/modules/esm/translators.js @@ -11,8 +11,6 @@ const { StringPrototypeReplace, } = primordials; -const assert = require('internal/assert'); - let _TYPES = null; function lazyTypes() { if (_TYPES !== null) return _TYPES; @@ -134,21 +132,27 @@ const isWindows = process.platform === 'win32'; const winSepRegEx = /\//g; translators.set('commonjs', function commonjsStrategy(url, isMain) { debug(`Translating CJSModule ${url}`); - let filename = internalURLModule.fileURLToPath(new URL(url)); - if (isWindows) - filename = StringPrototypeReplace(filename, winSepRegEx, '\\'); + const pathname = internalURLModule.fileURLToPath(new URL(url)); + const cached = this.cjsCache.get(url); + if (cached) { + this.cjsCache.delete(url); + return cached; + } + const module = CJSModule._cache[ + isWindows ? StringPrototypeReplace(pathname, winSepRegEx, '\\') : pathname + ]; + if (module && module.loaded) { + const exports = module.exports; + return new ModuleWrap(url, undefined, ['default'], function() { + this.setExport('default', exports); + }); + } return new ModuleWrap(url, undefined, ['default'], function() { debug(`Loading CJSModule ${url}`); - let module = CJSModule._cache[filename]; - if (!module || !module.loaded) { - module = new CJSModule(filename); - module.filename = filename; - module.paths = CJSModule._nodeModulePaths(module.path); - CJSModule._cache[filename] = module; - module.load(filename); - assert(module && module.loaded); - } - this.setExport('default', module.exports); + // We don't care about the return val of _load here because Module#load + // will handle it for us by checking the loader registry and filling the + // exports like above + CJSModule._load(pathname, undefined, isMain); }); }); diff --git a/test/es-module/test-esm-snapshot.mjs b/test/es-module/test-esm-snapshot.mjs index 6482e3df605252..e2695d20a81747 100644 --- a/test/es-module/test-esm-snapshot.mjs +++ b/test/es-module/test-esm-snapshot.mjs @@ -3,4 +3,4 @@ import '../fixtures/es-modules/esm-snapshot-mutator.js'; import one from '../fixtures/es-modules/esm-snapshot.js'; import assert from 'assert'; -assert.strictEqual(one, 2); +assert.strictEqual(one, 1); From 1e470510ff74391d7d4ec382909ea8960d2d2fbc Mon Sep 17 00:00:00 2001 From: Andrey Pechkurov Date: Thu, 30 Jul 2020 20:30:43 +0300 Subject: [PATCH 03/19] src: fix unused namespace member in node_util PR-URL: https://github.com/nodejs/node/pull/34565 Reviewed-By: Zeyu Yang Reviewed-By: Richard Lau Reviewed-By: James M Snell --- src/node_util.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/src/node_util.cc b/src/node_util.cc index 3b571180ca9d02..3eefa73739aa3c 100644 --- a/src/node_util.cc +++ b/src/node_util.cc @@ -21,7 +21,6 @@ using v8::Integer; using v8::Isolate; using v8::KeyCollectionMode; using v8::Local; -using v8::MaybeLocal; using v8::Object; using v8::ONLY_CONFIGURABLE; using v8::ONLY_ENUMERABLE; From a97b5f9c6acd101ec20d9278a840b2cb6ef94ac9 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Mon, 27 Jul 2020 14:25:13 -0700 Subject: [PATCH 04/19] quic: use OpenSSL built-in cert and hostname validation PR-URL: https://github.com/nodejs/node/pull/34533 Reviewed-By: Anna Henningsen --- node.gyp | 3 +- src/quic/node_quic_crypto.cc | 232 +----------- src/quic/node_quic_crypto.h | 6 - src/quic/node_quic_session.cc | 37 +- src/quic/node_quic_session.h | 2 +- .../test_quic_verifyhostnameidentity.cc | 349 ------------------ 6 files changed, 21 insertions(+), 608 deletions(-) delete mode 100644 test/cctest/test_quic_verifyhostnameidentity.cc diff --git a/node.gyp b/node.gyp index 497c06c6bc0a6c..6dd9541cc55bca 100644 --- a/node.gyp +++ b/node.gyp @@ -1235,8 +1235,7 @@ ], 'sources': [ 'test/cctest/test_quic_buffer.cc', - 'test/cctest/test_quic_cid.cc', - 'test/cctest/test_quic_verifyhostnameidentity.cc' + 'test/cctest/test_quic_cid.cc' ] }], ['v8_enable_inspector==1', { diff --git a/src/quic/node_quic_crypto.cc b/src/quic/node_quic_crypto.cc index e1762ce1837965..ac5cae6e548ed4 100644 --- a/src/quic/node_quic_crypto.cc +++ b/src/quic/node_quic_crypto.cc @@ -331,229 +331,6 @@ bool InvalidRetryToken( return false; } -namespace { - -bool SplitHostname( - const char* hostname, - std::vector* parts, - const char delim = '.') { - static std::string check_str = - "\x21\x22\x23\x24\x25\x26\x27\x28\x29\x2A\x2B\x2C\x2D\x2E\x2F\x30" - "\x31\x32\x33\x34\x35\x36\x37\x38\x39\x3A\x3B\x3C\x3D\x3E\x3F\x40" - "\x41\x42\x43\x44\x45\x46\x47\x48\x49\x4A\x4B\x4C\x4D\x4E\x4F\x50" - "\x51\x52\x53\x54\x55\x56\x57\x58\x59\x5A\x5B\x5C\x5D\x5E\x5F\x60" - "\x61\x62\x63\x64\x65\x66\x67\x68\x69\x6A\x6B\x6C\x6D\x6E\x6F\x70" - "\x71\x72\x73\x74\x75\x76\x77\x78\x79\x7A\x7B\x7C\x7D\x7E\x7F"; - - std::stringstream str(hostname); - std::string part; - while (getline(str, part, delim)) { - // if (part.length() == 0 || - // part.find_first_not_of(check_str) != std::string::npos) { - // return false; - // } - for (size_t n = 0; n < part.length(); n++) { - if (part[n] >= 'A' && part[n] <= 'Z') - part[n] = (part[n] | 0x20); // Lower case the letter - if (check_str.find(part[n]) == std::string::npos) - return false; - } - parts->push_back(part); - } - return true; -} - -bool CheckCertNames( - const std::vector& host_parts, - const std::string& name, - bool use_wildcard = true) { - - if (name.length() == 0) - return false; - - std::vector name_parts; - if (!SplitHostname(name.c_str(), &name_parts)) - return false; - - if (name_parts.size() != host_parts.size()) - return false; - - for (size_t n = host_parts.size() - 1; n > 0; --n) { - if (host_parts[n] != name_parts[n]) - return false; - } - - if (name_parts[0].find('*') == std::string::npos || - name_parts[0].find("xn--") != std::string::npos) { - return host_parts[0] == name_parts[0]; - } - - if (!use_wildcard) - return false; - - std::vector sub_parts; - SplitHostname(name_parts[0].c_str(), &sub_parts, '*'); - - if (sub_parts.size() > 2) - return false; - - if (name_parts.size() <= 2) - return false; - - std::string prefix; - std::string suffix; - if (sub_parts.size() == 2) { - prefix = sub_parts[0]; - suffix = sub_parts[1]; - } else { - prefix = ""; - suffix = sub_parts[0]; - } - - if (prefix.length() + suffix.length() > host_parts[0].length()) - return false; - - if (host_parts[0].compare(0, prefix.length(), prefix)) - return false; - - if (host_parts[0].compare( - host_parts[0].length() - suffix.length(), - suffix.length(), suffix)) { - return false; - } - - return true; -} - -} // namespace - -int VerifyHostnameIdentity( - const crypto::SSLPointer& ssl, - const char* hostname) { - int err = X509_V_ERR_HOSTNAME_MISMATCH; - crypto::X509Pointer cert(SSL_get_peer_certificate(ssl.get())); - if (!cert) - return err; - - // There are several pieces of information we need from the cert at this point - // 1. The Subject (if it exists) - // 2. The collection of Alt Names (if it exists) - // - // The certificate may have many Alt Names. We only care about the ones that - // are prefixed with 'DNS:', 'URI:', or 'IP Address:'. We might check - // additional ones later but we'll start with these. - // - // Ideally, we'd be able to *just* use OpenSSL's built in name checking for - // this (SSL_set1_host and X509_check_host) but it does not appear to do - // checking on URI or IP Address Alt names, which is unfortunate. We need - // both of those to retain compatibility with the peer identity verification - // Node.js already does elsewhere. At the very least, we'll use - // X509_check_host here first as a first step. If it is successful, awesome, - // there's nothing else for us to do. Return and be happy! - if (X509_check_host( - cert.get(), - hostname, - strlen(hostname), - X509_CHECK_FLAG_ALWAYS_CHECK_SUBJECT | - X509_CHECK_FLAG_MULTI_LABEL_WILDCARDS | - X509_CHECK_FLAG_SINGLE_LABEL_SUBDOMAINS, - nullptr) > 0) { - return 0; - } - - if (X509_check_ip_asc( - cert.get(), - hostname, - X509_CHECK_FLAG_ALWAYS_CHECK_SUBJECT) > 0) { - return 0; - } - - // If we've made it this far, then we have to perform a more check - return VerifyHostnameIdentity( - hostname, - crypto::GetCertificateCN(cert.get()), - crypto::GetCertificateAltNames(cert.get())); -} - -int VerifyHostnameIdentity( - const char* hostname, - const std::string& cert_cn, - const std::unordered_multimap& altnames) { - - int err = X509_V_ERR_HOSTNAME_MISMATCH; - - // 1. If the hostname is an IP address (v4 or v6), the certificate is valid - // if and only if there is an 'IP Address:' alt name specifying the same - // IP address. The IP address must be canonicalized to ensure a proper - // check. It's possible that the X509_check_ip_asc covers this. If so, - // we can remove this check. - - if (SocketAddress::is_numeric_host(hostname)) { - auto ips = altnames.equal_range("ip"); - for (auto ip = ips.first; ip != ips.second; ++ip) { - if (ip->second.compare(hostname) == 0) { - // Success! - return 0; - } - } - // No match, and since the hostname is an IP address, skip any - // further checks - return err; - } - - auto dns_names = altnames.equal_range("dns"); - auto uri_names = altnames.equal_range("uri"); - - size_t dns_count = std::distance(dns_names.first, dns_names.second); - size_t uri_count = std::distance(uri_names.first, uri_names.second); - - std::vector host_parts; - SplitHostname(hostname, &host_parts); - - // 2. If there no 'DNS:' or 'URI:' Alt names, if the certificate has a - // Subject, then we need to extract the CN field from the Subject. and - // check that the hostname matches the CN, taking into consideration - // the possibility of a wildcard in the CN. If there is a match, congrats, - // we have a valid certificate. Return and be happy. - - if (dns_count == 0 && uri_count == 0) { - if (cert_cn.length() > 0 && CheckCertNames(host_parts, cert_cn)) - return 0; - // No match, and since there are no dns or uri entries, return - return err; - } - - // 3. If, however, there are 'DNS:' and 'URI:' Alt names, things become more - // complicated. Essentially, we need to iterate through each 'DNS:' and - // 'URI:' Alt name to find one that matches. The 'DNS:' Alt names are - // relatively simple but may include wildcards. The 'URI:' Alt names - // require the name to be parsed as a URL, then extract the hostname from - // the URL, which is then checked against the hostname. If you find a - // match, yay! Return and be happy. (Note, it's possible that the 'DNS:' - // check in this step is redundant to the X509_check_host check. If so, - // we can simplify by removing those checks here.) - - // First, let's check dns names - for (auto name = dns_names.first; name != dns_names.second; ++name) { - if (name->first.length() > 0 && - CheckCertNames(host_parts, name->second)) { - return 0; - } - } - - // Then, check uri names - for (auto name = uri_names.first; name != uri_names.second; ++name) { - if (name->first.length() > 0 && - CheckCertNames(host_parts, name->second, false)) { - return 0; - } - } - - // 4. Failing all of the previous checks, we assume the certificate is - // invalid for an unspecified reason. - return err; -} - // Get the ALPN protocol identifier that was negotiated for the session Local GetALPNProtocol(const QuicSession& session) { QuicCryptoContext* ctx = session.crypto_context(); @@ -747,10 +524,17 @@ SSL_QUIC_METHOD quic_method = SSL_QUIC_METHOD{ void SetHostname(const crypto::SSLPointer& ssl, const std::string& hostname) { // If the hostname is an IP address, use an empty string // as the hostname instead. - if (SocketAddress::is_numeric_host(hostname.c_str())) { + X509_VERIFY_PARAM* param = SSL_get0_param(ssl.get()); + X509_VERIFY_PARAM_set_hostflags(param, 0); + + if (UNLIKELY(SocketAddress::is_numeric_host(hostname.c_str()))) { SSL_set_tlsext_host_name(ssl.get(), ""); + CHECK_EQ(X509_VERIFY_PARAM_set1_host(param, "", 0), 1); } else { SSL_set_tlsext_host_name(ssl.get(), hostname.c_str()); + CHECK_EQ( + X509_VERIFY_PARAM_set1_host(param, hostname.c_str(), hostname.length()), + 1); } } diff --git a/src/quic/node_quic_crypto.h b/src/quic/node_quic_crypto.h index 21548022f17219..e37c5e059a51f9 100644 --- a/src/quic/node_quic_crypto.h +++ b/src/quic/node_quic_crypto.h @@ -100,12 +100,6 @@ bool InvalidRetryToken( const uint8_t* token_secret, uint64_t verification_expiration); -int VerifyHostnameIdentity(const crypto::SSLPointer& ssl, const char* hostname); -int VerifyHostnameIdentity( - const char* hostname, - const std::string& cert_cn, - const std::unordered_multimap& altnames); - // Get the ALPN protocol identifier that was negotiated for the session v8::Local GetALPNProtocol(const QuicSession& session); diff --git a/src/quic/node_quic_session.cc b/src/quic/node_quic_session.cc index 041c601a3c3268..4aa5f47a51e9ab 100644 --- a/src/quic/node_quic_session.cc +++ b/src/quic/node_quic_session.cc @@ -570,10 +570,13 @@ void JSQuicSessionListener::OnHandshakeCompleted() { String::NewFromUtf8(env->isolate(), hostname).ToLocalChecked(); } - int err = ctx->VerifyPeerIdentity( - hostname != nullptr ? - hostname : - session()->hostname().c_str()); + Local validationErrorReason = v8::Null(env->isolate()); + Local validationErrorCode = v8::Null(env->isolate()); + int err = ctx->VerifyPeerIdentity(); + if (err != X509_V_OK) { + crypto::GetValidationErrorReason(env, err).ToLocal(&validationErrorReason); + crypto::GetValidationErrorCode(env, err).ToLocal(&validationErrorCode); + } Local argv[] = { servername, @@ -581,8 +584,8 @@ void JSQuicSessionListener::OnHandshakeCompleted() { ctx->cipher_name().ToLocalChecked(), ctx->cipher_version().ToLocalChecked(), Integer::New(env->isolate(), session()->max_pktlen_), - crypto::GetValidationErrorReason(env, err).ToLocalChecked(), - crypto::GetValidationErrorCode(env, err).ToLocalChecked(), + validationErrorReason, + validationErrorCode, session()->crypto_context()->early_data() ? v8::True(env->isolate()) : v8::False(env->isolate()) @@ -1144,26 +1147,8 @@ bool QuicCryptoContext::InitiateKeyUpdate() { uv_hrtime()) == 0; } -int QuicCryptoContext::VerifyPeerIdentity(const char* hostname) { - int err = crypto::VerifyPeerCertificate(ssl_); - if (err) - return err; - - // QUIC clients are required to verify the peer identity, servers are not. - switch (side_) { - case NGTCP2_CRYPTO_SIDE_CLIENT: - if (LIKELY(is_option_set( - QUICCLIENTSESSION_OPTION_VERIFY_HOSTNAME_IDENTITY))) { - return VerifyHostnameIdentity(ssl_, hostname); - } - break; - case NGTCP2_CRYPTO_SIDE_SERVER: - // TODO(@jasnell): In the future, we may want to implement this but - // for now we keep things simple and skip peer identity verification. - break; - } - - return 0; +int QuicCryptoContext::VerifyPeerIdentity() { + return crypto::VerifyPeerCertificate(ssl_); } // Write outbound TLS handshake data into the ngtcp2 connection diff --git a/src/quic/node_quic_session.h b/src/quic/node_quic_session.h index 2a034b8c24c5a9..2bdb290478d351 100644 --- a/src/quic/node_quic_session.h +++ b/src/quic/node_quic_session.h @@ -477,7 +477,7 @@ class QuicCryptoContext final : public MemoryRetainer { bool InitiateKeyUpdate(); - int VerifyPeerIdentity(const char* hostname); + int VerifyPeerIdentity(); QuicSession* session() const { return session_.get(); } diff --git a/test/cctest/test_quic_verifyhostnameidentity.cc b/test/cctest/test_quic_verifyhostnameidentity.cc deleted file mode 100644 index f611239ac72e4a..00000000000000 --- a/test/cctest/test_quic_verifyhostnameidentity.cc +++ /dev/null @@ -1,349 +0,0 @@ - -#include "base_object-inl.h" -#include "quic/node_quic_crypto.h" -#include "quic/node_quic_util-inl.h" -#include "node_sockaddr-inl.h" -#include "util.h" -#include "gtest/gtest.h" - -#include - -#include -#include -#include - -using node::quic::VerifyHostnameIdentity; - -enum altname_type { - TYPE_DNS, - TYPE_IP, - TYPE_URI -}; - -struct altname { - altname_type type; - const char* name; -}; - -void ToAltNamesMap( - const altname* names, - size_t names_len, - std::unordered_multimap* map) { - for (size_t n = 0; n < names_len; n++) { - switch (names[n].type) { - case TYPE_DNS: - map->emplace("dns", names[n].name); - continue; - case TYPE_IP: - map->emplace("ip", names[n].name); - continue; - case TYPE_URI: - map->emplace("uri", names[n].name); - continue; - } - } -} - -TEST(QuicCrypto, BasicCN_1) { - std::unordered_multimap altnames; - CHECK_EQ(VerifyHostnameIdentity("a.com", std::string("a.com"), altnames), 0); -} - -TEST(QuicCrypto, BasicCN_2) { - std::unordered_multimap altnames; - CHECK_EQ(VerifyHostnameIdentity("a.com", std::string("A.com"), altnames), 0); -} - -TEST(QuicCrypto, BasicCN_3_Fail) { - std::unordered_multimap altnames; - CHECK_EQ( - VerifyHostnameIdentity("a.com", std::string("b.com"), altnames), - X509_V_ERR_HOSTNAME_MISMATCH); -} - -TEST(QuicCrypto, BasicCN_4) { - std::unordered_multimap altnames; - CHECK_EQ(VerifyHostnameIdentity("a.com", std::string("a.com."), altnames), 0); -} - -TEST(QuicCrypto, BasicCN_5_Fail) { - std::unordered_multimap altnames; - CHECK_EQ( - VerifyHostnameIdentity("a.com", std::string(".a.com"), altnames), - X509_V_ERR_HOSTNAME_MISMATCH); -} - -TEST(QuicCrypto, BasicCN_6_Fail) { - std::unordered_multimap altnames; - CHECK_EQ( - VerifyHostnameIdentity("8.8.8.8", std::string("8.8.8.8"), altnames), - X509_V_ERR_HOSTNAME_MISMATCH); -} - -TEST(QuicCrypto, BasicCN_7_Fail) { - std::unordered_multimap altnames; - altnames.emplace("dns", "8.8.8.8"); - CHECK_EQ( - VerifyHostnameIdentity("8.8.8.8", std::string("8.8.8.8"), altnames), - X509_V_ERR_HOSTNAME_MISMATCH); -} - -TEST(QuicCrypto, BasicCN_8_Fail) { - std::unordered_multimap altnames; - altnames.emplace("uri", "8.8.8.8"); - CHECK_EQ( - VerifyHostnameIdentity("8.8.8.8", std::string("8.8.8.8"), altnames), - X509_V_ERR_HOSTNAME_MISMATCH); -} - -TEST(QuicCrypto, BasicCN_9) { - std::unordered_multimap altnames; - altnames.emplace("ip", "8.8.8.8"); - CHECK_EQ( - VerifyHostnameIdentity("8.8.8.8", std::string("8.8.8.8"), altnames), 0); -} - -TEST(QuicCrypto, BasicCN_10_Fail) { - std::unordered_multimap altnames; - altnames.emplace("ip", "8.8.8.8/24"); - CHECK_EQ( - VerifyHostnameIdentity("8.8.8.8", std::string("8.8.8.8"), altnames), - X509_V_ERR_HOSTNAME_MISMATCH); -} - -TEST(QuicCrypto, BasicCN_11) { - std::unordered_multimap altnames; - CHECK_EQ( - VerifyHostnameIdentity("b.a.com", std::string("*.a.com"), altnames), 0); -} - -TEST(QuicCrypto, BasicCN_12_Fail) { - std::unordered_multimap altnames; - CHECK_EQ( - VerifyHostnameIdentity("ba.com", std::string("*.a.com"), altnames), - X509_V_ERR_HOSTNAME_MISMATCH); -} - -TEST(QuicCrypto, BasicCN_13_Fail) { - std::unordered_multimap altnames; - CHECK_EQ( - VerifyHostnameIdentity("\n.a.com", std::string("*.a.com"), altnames), - X509_V_ERR_HOSTNAME_MISMATCH); -} - -TEST(QuicCrypto, BasicCN_14_Fail) { - std::unordered_multimap altnames; - altnames.emplace("dns", "omg.com"); - CHECK_EQ( - VerifyHostnameIdentity("b.a.com", std::string("*.a.com"), altnames), - X509_V_ERR_HOSTNAME_MISMATCH); -} - -TEST(QuicCrypto, BasicCN_15_Fail) { - std::unordered_multimap altnames; - CHECK_EQ( - VerifyHostnameIdentity("b.a.com", std::string("b*b.a.com"), altnames), - X509_V_ERR_HOSTNAME_MISMATCH); -} - -TEST(QuicCrypto, BasicCN_16_Fail) { - std::unordered_multimap altnames; - CHECK_EQ( - VerifyHostnameIdentity("b.a.com", std::string(), altnames), - X509_V_ERR_HOSTNAME_MISMATCH); -} - -TEST(QuicCrypto, BasicCN_17) { - // TODO(@jasnell): This should test multiple CN's. The code is only - // implemented to support one. Need to fix - std::unordered_multimap altnames; - CHECK_EQ( - VerifyHostnameIdentity("foo.com", std::string("foo.com"), altnames), 0); -} - -TEST(QuicCrypto, BasicCN_18_Fail) { - std::unordered_multimap altnames; - altnames.emplace("dns", "*"); - CHECK_EQ( - VerifyHostnameIdentity("a.com", std::string("b.com"), altnames), - X509_V_ERR_HOSTNAME_MISMATCH); -} - -TEST(QuicCrypto, BasicCN_19_Fail) { - std::unordered_multimap altnames; - altnames.emplace("dns", "*.com"); - CHECK_EQ( - VerifyHostnameIdentity("a.com", std::string("b.com"), altnames), - X509_V_ERR_HOSTNAME_MISMATCH); -} - -TEST(QuicCrypto, BasicCN_20) { - std::unordered_multimap altnames; - altnames.emplace("dns", "*.co.uk"); - CHECK_EQ( - VerifyHostnameIdentity("a.co.uk", std::string("b.com"), altnames), 0); -} - -TEST(QuicCrypto, BasicCN_21_Fail) { - std::unordered_multimap altnames; - altnames.emplace("dns", "*.a.com"); - CHECK_EQ( - VerifyHostnameIdentity("a.com", std::string("a.com"), altnames), - X509_V_ERR_HOSTNAME_MISMATCH); -} - -TEST(QuicCrypto, BasicCN_22_Fail) { - std::unordered_multimap altnames; - altnames.emplace("dns", "*.a.com"); - CHECK_EQ( - VerifyHostnameIdentity("a.com", std::string("b.com"), altnames), - X509_V_ERR_HOSTNAME_MISMATCH); -} - -TEST(QuicCrypto, BasicCN_23) { - std::unordered_multimap altnames; - altnames.emplace("dns", "a.com"); - CHECK_EQ( - VerifyHostnameIdentity("a.com", std::string("b.com"), altnames), 0); -} - -TEST(QuicCrypto, BasicCN_24) { - std::unordered_multimap altnames; - altnames.emplace("dns", "A.COM"); - CHECK_EQ( - VerifyHostnameIdentity("a.com", std::string("b.com"), altnames), 0); -} - -TEST(QuicCrypto, BasicCN_25_Fail) { - std::unordered_multimap altnames; - altnames.emplace("dns", "*.a.com"); - CHECK_EQ( - VerifyHostnameIdentity("a.com", std::string(), altnames), - X509_V_ERR_HOSTNAME_MISMATCH); -} - -TEST(QuicCrypto, BasicCN_26) { - std::unordered_multimap altnames; - altnames.emplace("dns", "*.a.com"); - CHECK_EQ( - VerifyHostnameIdentity("b.a.com", std::string(), altnames), 0); -} - -TEST(QuicCrypto, BasicCN_27_Fail) { - std::unordered_multimap altnames; - altnames.emplace("dns", "*.a.com"); - CHECK_EQ( - VerifyHostnameIdentity("c.b.a.com", std::string(), altnames), - X509_V_ERR_HOSTNAME_MISMATCH); -} - -TEST(QuicCrypto, BasicCN_28) { - std::unordered_multimap altnames; - altnames.emplace("dns", "*b.a.com"); - CHECK_EQ( - VerifyHostnameIdentity("b.a.com", std::string(), altnames), 0); -} - -TEST(QuicCrypto, BasicCN_29) { - std::unordered_multimap altnames; - altnames.emplace("dns", "*b.a.com"); - CHECK_EQ( - VerifyHostnameIdentity("a-cb.a.com", std::string(), altnames), 0); -} - -TEST(QuicCrypto, BasicCN_30_Fail) { - std::unordered_multimap altnames; - altnames.emplace("dns", "*b.a.com"); - CHECK_EQ( - VerifyHostnameIdentity("a.b.a.com", std::string(), altnames), - X509_V_ERR_HOSTNAME_MISMATCH); -} - - -TEST(QuicCrypto, BasicCN_31) { - std::unordered_multimap altnames; - altnames.emplace("dns", "*b.a.com"); - altnames.emplace("dns", "a.b.a.com"); - CHECK_EQ( - VerifyHostnameIdentity("a.b.a.com", std::string(), altnames), 0); -} - - -TEST(QuicCrypto, BasicCN_32) { - std::unordered_multimap altnames; - altnames.emplace("uri", "a.b.a.com"); - CHECK_EQ( - VerifyHostnameIdentity("a.b.a.com", std::string(), altnames), 0); -} - -TEST(QuicCrypto, BasicCN_33_Fail) { - std::unordered_multimap altnames; - altnames.emplace("uri", "*.b.a.com"); - CHECK_EQ( - VerifyHostnameIdentity("a.b.a.com", std::string(), altnames), - X509_V_ERR_HOSTNAME_MISMATCH); -} - -// // Invalid URI -// { -// host: 'a.b.a.com', cert: { -// subjectaltname: 'URI:http://[a.b.a.com]/', -// subject: {} -// } -// }, - -TEST(QuicCrypto, BasicCN_35_Fail) { - std::unordered_multimap altnames; - altnames.emplace("ip", "127.0.0.1"); - CHECK_EQ( - VerifyHostnameIdentity("a.b.a.com", std::string(), altnames), - X509_V_ERR_HOSTNAME_MISMATCH); -} - -TEST(QuicCrypto, BasicCN_36) { - std::unordered_multimap altnames; - altnames.emplace("ip", "127.0.0.1"); - CHECK_EQ( - VerifyHostnameIdentity("127.0.0.1", std::string(), altnames), 0); -} - -TEST(QuicCrypto, BasicCN_37_Fail) { - std::unordered_multimap altnames; - altnames.emplace("ip", "127.0.0.1"); - CHECK_EQ( - VerifyHostnameIdentity("127.0.0.2", std::string(), altnames), - X509_V_ERR_HOSTNAME_MISMATCH); -} - -TEST(QuicCrypto, BasicCN_38_Fail) { - std::unordered_multimap altnames; - altnames.emplace("dns", "a.com"); - CHECK_EQ( - VerifyHostnameIdentity("127.0.0.1", std::string(), altnames), - X509_V_ERR_HOSTNAME_MISMATCH); -} - -TEST(QuicCrypto, BasicCN_39_Fail) { - std::unordered_multimap altnames; - altnames.emplace("dns", "a.com"); - CHECK_EQ( - VerifyHostnameIdentity("localhost", std::string("localhost"), altnames), - X509_V_ERR_HOSTNAME_MISMATCH); -} - -TEST(QuicCrypto, BasicCN_40) { - std::unordered_multimap altnames; - CHECK_EQ( - VerifyHostnameIdentity( - "xn--bcher-kva.example.com", - std::string("*.example.com"), altnames), 0); -} - -TEST(QuicCrypto, BasicCN_41_Fail) { - std::unordered_multimap altnames; - CHECK_EQ( - VerifyHostnameIdentity( - "xn--bcher-kva.example.com", - std::string("xn--*.example.com"), altnames), - X509_V_ERR_HOSTNAME_MISMATCH); -} From 0cc2a54a53831968d5c955cb6cc09a2c46bd75ea Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Wed, 29 Jul 2020 10:48:00 -0700 Subject: [PATCH 05/19] n-api: simplify bigint-from-word creation Macro `CHECK_MAYBE_EMPTY_WITH_PREAMBLE()` does the work of checking the `TryCatch` and returning `napi_pending_exception` so this change reuses it for `napi_create_bigint_words()`. Signed-off-by: Gabriel Schulhof PR-URL: https://github.com/nodejs/node/pull/34554 Reviewed-By: James M Snell Reviewed-By: Anna Henningsen Reviewed-By: Michael Dawson --- src/js_native_api_v8.cc | 11 ++++------- test/js-native-api/test_bigint/test.js | 7 +++++++ test/js-native-api/test_bigint/test_bigint.c | 18 ++++++++++++++++++ 3 files changed, 29 insertions(+), 7 deletions(-) diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index e99333a6a362d1..37cd1a1ab22109 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -1602,13 +1602,10 @@ napi_status napi_create_bigint_words(napi_env env, v8::MaybeLocal b = v8::BigInt::NewFromWords( context, sign_bit, word_count, words); - if (try_catch.HasCaught()) { - return napi_set_last_error(env, napi_pending_exception); - } else { - CHECK_MAYBE_EMPTY(env, b, napi_generic_failure); - *result = v8impl::JsValueFromV8LocalValue(b.ToLocalChecked()); - return napi_clear_last_error(env); - } + CHECK_MAYBE_EMPTY_WITH_PREAMBLE(env, b, napi_generic_failure); + + *result = v8impl::JsValueFromV8LocalValue(b.ToLocalChecked()); + return GET_RETURN_STATUS(env); } napi_status napi_get_boolean(napi_env env, bool value, napi_value* result) { diff --git a/test/js-native-api/test_bigint/test.js b/test/js-native-api/test_bigint/test.js index 85a183171743c7..bf9ce5066d6d2a 100644 --- a/test/js-native-api/test_bigint/test.js +++ b/test/js-native-api/test_bigint/test.js @@ -7,6 +7,7 @@ const { TestUint64, TestWords, CreateTooBigBigInt, + MakeBigIntWordsThrow, } = require(`./build/${common.buildType}/test_bigint`); [ @@ -43,3 +44,9 @@ assert.throws(CreateTooBigBigInt, { name: 'Error', message: 'Invalid argument', }); + +// Test that we correctly forward exceptions from the engine. +assert.throws(MakeBigIntWordsThrow, { + name: 'RangeError', + message: 'Maximum BigInt size exceeded' +}); diff --git a/test/js-native-api/test_bigint/test_bigint.c b/test/js-native-api/test_bigint/test_bigint.c index c62a0a6a6c2bbc..181f9103fa3399 100644 --- a/test/js-native-api/test_bigint/test_bigint.c +++ b/test/js-native-api/test_bigint/test_bigint.c @@ -1,3 +1,4 @@ +#include #include #include #include @@ -122,6 +123,22 @@ static napi_value CreateTooBigBigInt(napi_env env, napi_callback_info info) { return output; } +// Test that we correctly forward exceptions from the engine. +static napi_value MakeBigIntWordsThrow(napi_env env, napi_callback_info info) { + uint64_t words[10]; + napi_value output; + + napi_status status = napi_create_bigint_words(env, + 0, + INT_MAX, + words, + &output); + if (status != napi_pending_exception) + napi_throw_error(env, NULL, "Expected status `napi_pending_exception`"); + + return NULL; +} + EXTERN_C_START napi_value Init(napi_env env, napi_value exports) { napi_property_descriptor descriptors[] = { @@ -130,6 +147,7 @@ napi_value Init(napi_env env, napi_value exports) { DECLARE_NAPI_PROPERTY("TestUint64", TestUint64), DECLARE_NAPI_PROPERTY("TestWords", TestWords), DECLARE_NAPI_PROPERTY("CreateTooBigBigInt", CreateTooBigBigInt), + DECLARE_NAPI_PROPERTY("MakeBigIntWordsThrow", MakeBigIntWordsThrow), }; NAPI_CALL(env, napi_define_properties( From 6cab3b0e261dfaedd42ca46ee04957d4ec6d1be0 Mon Sep 17 00:00:00 2001 From: Mary Marchini Date: Sat, 27 Jun 2020 12:38:02 -0700 Subject: [PATCH 06/19] build: auto start Jenkins CI via PR labels Add an Action that will find every PR with the `request-ci` label and will start a Jenkins CI for each of these Pull Requests. The scheduler event is used to circumvent GitHub Actions limitations on Pull Requests from forks (where secrets are not accessible and the GITHUB_TOKEN is read-only). If the Action fails to start a CI, it will add a `request-ci-failed` label and will leave a comment with the error message from NCU. Fixes: https://github.com/nodejs/github-bot/issues/234 PR-URL: https://github.com/nodejs/node/pull/34089 Reviewed-By: Christian Clauss --- .github/workflows/auto-start-ci.yml | 65 +++++++++++++++++++++++++++++ tools/actions/start-ci.sh | 53 +++++++++++++++++++++++ 2 files changed, 118 insertions(+) create mode 100644 .github/workflows/auto-start-ci.yml create mode 100755 tools/actions/start-ci.sh diff --git a/.github/workflows/auto-start-ci.yml b/.github/workflows/auto-start-ci.yml new file mode 100644 index 00000000000000..d4c364e2bb2f52 --- /dev/null +++ b/.github/workflows/auto-start-ci.yml @@ -0,0 +1,65 @@ +--- +name: Auto Start CI + +on: + push: + schedule: + # `schedule` event is used instead of `pull_request` because when a + # `pull_requesst` event is triggered on a PR from a fork, GITHUB_TOKEN will + # be read-only, and the Action won't have access to any other repository + # secrets, which it needs to access Jenkins API. Runs every five minutes + # (fastest the scheduler can run). Five minutes is optimistic, it can take + # longer to run. + - cron: "*/5 * * * *" + +jobs: + commitQueue: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@master + + # Install dependencies + - name: Install jq + run: sudo apt-get install jq -y + - name: Install Node.js + uses: actions/setup-node@v2-beta + with: + node-version: '12' + - name: Install node-core-utils + run: npm install -g node-core-utils + + - name: Set variables + run: | + echo "::set-env name=REPOSITORY::$(echo ${{ github.repository }} | cut -d/ -f2)" + echo "::set-env name=OWNER::${{ github.repository_owner }}" + + # Get Pull Requests + - name: Get Pull Requests + uses: octokit/graphql-action@v2.x + id: get_prs_for_ci + with: + query: | + query prs($owner:String!, $repo:String!) { + repository(owner:$owner, name:$repo) { + pullRequests(labels: ["request-ci"], states: OPEN, last: 100) { + nodes { + number + } + } + } + } + owner: ${{ env.OWNER }} + repo: ${{ env.REPOSITORY }} + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + + - name: Setup node-core-utils + run: | + ncu-config set username ${{ secrets.JENKINS_USER }} + ncu-config set token none + ncu-config set jenkins_token ${{ secrets.JENKINS_TOKEN }} + ncu-config set owner ${{ env.OWNER }} + ncu-config set repo ${{ env.REPOSITORY }} + + - name: Start CI + run: ./tools/start-ci.sh ${{ secrets.GITHUB_TOKEN }} ${{ env.OWNER }} ${{ env.REPOSITORY }} $(echo '${{ steps.get_prs_for_ci.outputs.data }}' | jq '.repository.pullRequests.nodes | map(.number) | .[]') diff --git a/tools/actions/start-ci.sh b/tools/actions/start-ci.sh new file mode 100755 index 00000000000000..528c3544b17fa9 --- /dev/null +++ b/tools/actions/start-ci.sh @@ -0,0 +1,53 @@ +#!/bin/bash + +set -xe + +GITHUB_TOKEN=$1 +OWNER=$2 +REPOSITORY=$3 +API_URL=https://api.github.com +REQUEST_CI_LABEL='request-ci' +REQUEST_CI_FAILED_LABEL='request-ci-failed' +shift 3 + +function issueUrl() { + echo "$API_URL/repos/${OWNER}/${REPOSITORY}/issues/${1}" +} + +function labelsUrl() { + echo "$(issueUrl "${1}")/labels" +} + +function commentsUrl() { + echo "$(issueUrl "${1}")/comments" +} + +for pr in "$@"; do + curl -sL --request DELETE \ + --url "$(labelsUrl "$pr")"/"$REQUEST_CI_LABEL" \ + --header "authorization: Bearer ${GITHUB_TOKEN}" \ + --header 'content-type: application/json' + + ci_started=yes + rm -f output; + ncu-ci run "$pr" >output 2>&1 || ci_started=no + + if [ "$ci_started" == "no" ]; then + # Do we need to reset? + curl -sL --request PUT \ + --url "$(labelsUrl "$pr")" \ + --header "authorization: Bearer ${GITHUB_TOKEN}" \ + --header 'content-type: application/json' \ + --data '{"labels": ["'"${REQUEST_CI_FAILED_LABEL}"'"]}' + + jq -n --arg content "
Couldn't start CI
$(cat output)
" '{body: $content}' > output.json + + curl -sL --request POST \ + --url "$(commentsUrl "$pr")" \ + --header "authorization: Bearer ${GITHUB_TOKEN}" \ + --header 'content-type: application/json' \ + --data @output.json + + rm output.json; + fi +done; From c8c2f4d0ae6162ab29ab7eabe87a409c7f836a66 Mon Sep 17 00:00:00 2001 From: Mary Marchini Date: Fri, 31 Jul 2020 12:58:38 -0700 Subject: [PATCH 07/19] doc: update .mailmap for mmarchini PR-URL: https://github.com/nodejs/node/pull/34586 Reviewed-By: Colin Ihrig Reviewed-By: Anna Henningsen Reviewed-By: James M Snell --- .mailmap | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/.mailmap b/.mailmap index 4a73a2b352a2e7..64f74f4b08a4b9 100644 --- a/.mailmap +++ b/.mailmap @@ -248,10 +248,11 @@ Marti Martz Martial James Jefferson Martijn Schrage Oblosys Masato Ohba -Matheus Marchini -Matheus Marchini -Matheus Marchini -Matheus Marchini +Mary Marchini +Mary Marchini +Mary Marchini +Mary Marchini +Mary Marchini Matt Lang matt-in-a-hat Matt Reed matthewreed26 Matteo Collina From f5df3c25826ad8b9d9e18b3ef449f8d40593a418 Mon Sep 17 00:00:00 2001 From: Mary Marchini Date: Fri, 31 Jul 2020 13:14:22 -0700 Subject: [PATCH 08/19] doc: update mmarchini contact info PR-URL: https://github.com/nodejs/node/pull/34586 Reviewed-By: Colin Ihrig Reviewed-By: Anna Henningsen Reviewed-By: James M Snell --- AUTHORS | 2 +- README.md | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/AUTHORS b/AUTHORS index e6af564013f31a..839039c541e56d 100644 --- a/AUTHORS +++ b/AUTHORS @@ -1983,7 +1983,7 @@ Pierre-Loic Doulcet Fran Herrero Francois KY suman-mitra -Matheus Marchini +Mary Marchini neta Whien Chiahao Lin diff --git a/README.md b/README.md index 84ab9cd5862138..334b6e864bc1be 100644 --- a/README.md +++ b/README.md @@ -184,7 +184,7 @@ For information about the governance of the Node.js project, see * [mhdawson](https://github.com/mhdawson) - **Michael Dawson** <michael_dawson@ca.ibm.com> (he/him) * [mmarchini](https://github.com/mmarchini) - -**Matheus Marchini** <mat@mmarchini.me> +**Mary Marchini** <oss@mmarchini.me> * [MylesBorins](https://github.com/MylesBorins) - **Myles Borins** <myles.borins@gmail.com> (he/him) * [targos](https://github.com/targos) - @@ -360,7 +360,7 @@ For information about the governance of the Node.js project, see * [misterdjules](https://github.com/misterdjules) - **Julien Gilli** <jgilli@nodejs.org> * [mmarchini](https://github.com/mmarchini) - -**Matheus Marchini** <mat@mmarchini.me> +**Mary Marchini** <oss@mmarchini.me> * [mscdex](https://github.com/mscdex) - **Brian White** <mscdex@mscdex.net> * [MylesBorins](https://github.com/MylesBorins) - From 8b3ad75b03ee49688ac0e9e4aa2481231fd4ff96 Mon Sep 17 00:00:00 2001 From: Mary Marchini Date: Fri, 31 Jul 2020 13:43:38 -0700 Subject: [PATCH 09/19] doc: add mmarchini pronouns PR-URL: https://github.com/nodejs/node/pull/34586 Reviewed-By: Colin Ihrig Reviewed-By: Anna Henningsen Reviewed-By: James M Snell --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 334b6e864bc1be..4e972c8d005d7f 100644 --- a/README.md +++ b/README.md @@ -184,7 +184,7 @@ For information about the governance of the Node.js project, see * [mhdawson](https://github.com/mhdawson) - **Michael Dawson** <michael_dawson@ca.ibm.com> (he/him) * [mmarchini](https://github.com/mmarchini) - -**Mary Marchini** <oss@mmarchini.me> +**Mary Marchini** <oss@mmarchini.me> (she/her) * [MylesBorins](https://github.com/MylesBorins) - **Myles Borins** <myles.borins@gmail.com> (he/him) * [targos](https://github.com/targos) - @@ -360,7 +360,7 @@ For information about the governance of the Node.js project, see * [misterdjules](https://github.com/misterdjules) - **Julien Gilli** <jgilli@nodejs.org> * [mmarchini](https://github.com/mmarchini) - -**Mary Marchini** <oss@mmarchini.me> +**Mary Marchini** <oss@mmarchini.me> (she/her) * [mscdex](https://github.com/mscdex) - **Brian White** <mscdex@mscdex.net> * [MylesBorins](https://github.com/MylesBorins) - From cc7ec889e863433c248bc4b5c8e33f61ccc40f29 Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Fri, 14 Jun 2019 16:44:18 -0700 Subject: [PATCH 10/19] n-api: support type-tagging objects `napi_instanceof()` is insufficient for reliably establishing the data type to which a pointer stored with `napi_wrap()` or `napi_create_external()` inside a JavaScript object points. Thus, we need a way to "mark" an object with a value that, when later retrieved, can unambiguously tell us whether it is safe to cast the pointer stored inside it to a certain structure. Such a check must survive loading/unloading/multiple instances of an addon, so we use UUIDs chosen *a priori*. Fixes: https://github.com/nodejs/node/issues/28164 Co-authored-by: Anna Henningsen PR-URL: https://github.com/nodejs/node/pull/28237 Reviewed-By: Anna Henningsen Reviewed-By: Michael Dawson Reviewed-By: Colin Ihrig Signed-off-by: Gabriel Schulhof --- benchmark/napi/type-tag-check/binding.gyp | 8 + benchmark/napi/type-tag-check/index.js | 18 ++ benchmark/napi/type-tag/binding.c | 84 ++++++++ benchmark/napi/type-tag/binding.gyp | 8 + benchmark/napi/type-tag/check-object-tag.js | 18 ++ benchmark/napi/type-tag/index.js | 18 ++ doc/api/n-api.md | 213 +++++++++++++++++++ src/env.h | 1 + src/js_native_api.h | 10 + src/js_native_api_types.h | 7 + src/js_native_api_v8.cc | 69 ++++++ src/js_native_api_v8.h | 32 +++ test/js-native-api/test_object/test.js | 18 ++ test/js-native-api/test_object/test_object.c | 41 ++++ 14 files changed, 545 insertions(+) create mode 100644 benchmark/napi/type-tag-check/binding.gyp create mode 100644 benchmark/napi/type-tag-check/index.js create mode 100644 benchmark/napi/type-tag/binding.c create mode 100644 benchmark/napi/type-tag/binding.gyp create mode 100644 benchmark/napi/type-tag/check-object-tag.js create mode 100644 benchmark/napi/type-tag/index.js diff --git a/benchmark/napi/type-tag-check/binding.gyp b/benchmark/napi/type-tag-check/binding.gyp new file mode 100644 index 00000000000000..595ab325233661 --- /dev/null +++ b/benchmark/napi/type-tag-check/binding.gyp @@ -0,0 +1,8 @@ +{ + 'targets': [ + { + 'target_name': 'binding', + 'sources': [ '../type-tag/binding.c' ] + } + ] +} diff --git a/benchmark/napi/type-tag-check/index.js b/benchmark/napi/type-tag-check/index.js new file mode 100644 index 00000000000000..346dfb7812dea1 --- /dev/null +++ b/benchmark/napi/type-tag-check/index.js @@ -0,0 +1,18 @@ +'use strict'; +const common = require('../../common.js'); + +let binding; +try { + binding = require(`./build/${common.buildType}/binding`); +} catch { + console.error(`${__filename}: Binding failed to load`); + process.exit(0); +} + +const bench = common.createBenchmark(main, { + n: [1e5, 1e6, 1e7], +}); + +function main({ n }) { + binding.checkObjectTag(n, bench, bench.start, bench.end); +} diff --git a/benchmark/napi/type-tag/binding.c b/benchmark/napi/type-tag/binding.c new file mode 100644 index 00000000000000..7bc8b5d7502e8b --- /dev/null +++ b/benchmark/napi/type-tag/binding.c @@ -0,0 +1,84 @@ +#include +#define NAPI_EXPERIMENTAL +#include + +#define NAPI_CALL(call) \ + do { \ + napi_status status = call; \ + assert(status == napi_ok && #call " failed"); \ + } while (0); + +#define EXPORT_FUNC(env, exports, name, func) \ + do { \ + napi_value js_func; \ + NAPI_CALL(napi_create_function((env), \ + (name), \ + NAPI_AUTO_LENGTH, \ + (func), \ + NULL, \ + &js_func)); \ + NAPI_CALL(napi_set_named_property((env), \ + (exports), \ + (name), \ + js_func)); \ + } while (0); + +static const napi_type_tag tag = { + 0xe7ecbcd5954842f6, 0x9e75161c9bf27282 +}; + +static napi_value TagObject(napi_env env, napi_callback_info info) { + size_t argc = 4; + napi_value argv[4]; + uint32_t n; + uint32_t index; + napi_handle_scope scope; + + NAPI_CALL(napi_get_cb_info(env, info, &argc, argv, NULL, NULL)); + NAPI_CALL(napi_get_value_uint32(env, argv[0], &n)); + NAPI_CALL(napi_open_handle_scope(env, &scope)); + napi_value objects[n]; + for (index = 0; index < n; index++) { + NAPI_CALL(napi_create_object(env, &objects[index])); + } + + // Time the object tag creation. + NAPI_CALL(napi_call_function(env, argv[1], argv[2], 0, NULL, NULL)); + for (index = 0; index < n; index++) { + NAPI_CALL(napi_type_tag_object(env, objects[index], &tag)); + } + NAPI_CALL(napi_call_function(env, argv[1], argv[3], 1, &argv[0], NULL)); + + NAPI_CALL(napi_close_handle_scope(env, scope)); + return NULL; +} + +static napi_value CheckObjectTag(napi_env env, napi_callback_info info) { + size_t argc = 4; + napi_value argv[4]; + uint32_t n; + uint32_t index; + bool is_of_type; + + NAPI_CALL(napi_get_cb_info(env, info, &argc, argv, NULL, NULL)); + NAPI_CALL(napi_get_value_uint32(env, argv[0], &n)); + napi_value object; + NAPI_CALL(napi_create_object(env, &object)); + NAPI_CALL(napi_type_tag_object(env, object, &tag)); + + // Time the object tag checking. + NAPI_CALL(napi_call_function(env, argv[1], argv[2], 0, NULL, NULL)); + for (index = 0; index < n; index++) { + NAPI_CALL(napi_check_object_type_tag(env, object, &tag, &is_of_type)); + assert(is_of_type && " type mismatch"); + } + NAPI_CALL(napi_call_function(env, argv[1], argv[3], 1, &argv[0], NULL)); + + return NULL; +} + +NAPI_MODULE_INIT() { + EXPORT_FUNC(env, exports, "tagObject", TagObject); + EXPORT_FUNC(env, exports, "checkObjectTag", CheckObjectTag); + return exports; +} diff --git a/benchmark/napi/type-tag/binding.gyp b/benchmark/napi/type-tag/binding.gyp new file mode 100644 index 00000000000000..413621ade335a1 --- /dev/null +++ b/benchmark/napi/type-tag/binding.gyp @@ -0,0 +1,8 @@ +{ + 'targets': [ + { + 'target_name': 'binding', + 'sources': [ 'binding.c' ] + } + ] +} diff --git a/benchmark/napi/type-tag/check-object-tag.js b/benchmark/napi/type-tag/check-object-tag.js new file mode 100644 index 00000000000000..346dfb7812dea1 --- /dev/null +++ b/benchmark/napi/type-tag/check-object-tag.js @@ -0,0 +1,18 @@ +'use strict'; +const common = require('../../common.js'); + +let binding; +try { + binding = require(`./build/${common.buildType}/binding`); +} catch { + console.error(`${__filename}: Binding failed to load`); + process.exit(0); +} + +const bench = common.createBenchmark(main, { + n: [1e5, 1e6, 1e7], +}); + +function main({ n }) { + binding.checkObjectTag(n, bench, bench.start, bench.end); +} diff --git a/benchmark/napi/type-tag/index.js b/benchmark/napi/type-tag/index.js new file mode 100644 index 00000000000000..3f85b9af8e7d59 --- /dev/null +++ b/benchmark/napi/type-tag/index.js @@ -0,0 +1,18 @@ +'use strict'; +const common = require('../../common.js'); + +let binding; +try { + binding = require(`./build/${common.buildType}/binding`); +} catch { + console.error(`${__filename}: Binding failed to load`); + process.exit(0); +} + +const bench = common.createBenchmark(main, { + n: [1e3, 1e4, 1e5], +}); + +function main({ n }) { + binding.tagObject(n, bench, bench.start, bench.end); +} diff --git a/doc/api/n-api.md b/doc/api/n-api.md index f45dd2dd4c6951..8dff1e0e15f1bf 100644 --- a/doc/api/n-api.md +++ b/doc/api/n-api.md @@ -602,6 +602,27 @@ minimum lifetimes explicitly. For more details, review the [Object lifetime management][]. +#### napi_type_tag + + +A 128-bit value stored as two unsigned 64-bit integers. It serves as a UUID +with which JavaScript objects can be "tagged" in order to ensure that they are +of a certain type. This is a stronger check than [`napi_instanceof`][], because +the latter can report a false positive if the object's prototype has been +manipulated. Type-tagging is most useful in conjunction with [`napi_wrap`][] +because it ensures that the pointer retrieved from a wrapped object can be +safely cast to the native type corresponding to the type tag that had been +previously applied to the JavaScript object. + +```c +typedef struct { + uint64_t lower; + uint64_t upper; +} napi_type_tag; +``` + ### N-API callback types #### napi_callback_info @@ -4288,6 +4309,143 @@ if (is_instance) { The reference must be freed once it is no longer needed. +There are occasions where `napi_instanceof()` is insufficient for ensuring that +a JavaScript object is a wrapper for a certain native type. This is the case +especially when wrapped JavaScript objects are passed back into the addon via +static methods rather than as the `this` value of prototype methods. In such +cases there is a chance that they may be unwrapped incorrectly. + +```js +const myAddon = require('./build/Release/my_addon.node'); + +// `openDatabase()` returns a JavaScript object that wraps a native database +// handle. +const dbHandle = myAddon.openDatabase(); + +// `query()` returns a JavaScript object that wraps a native query handle. +const queryHandle = myAddon.query(dbHandle, 'Gimme ALL the things!'); + +// There is an accidental error in the line below. The first parameter to +// `myAddon.queryHasRecords()` should be the database handle (`dbHandle`), not +// the query handle (`query`), so the correct condition for the while-loop +// should be +// +// myAddon.queryHasRecords(dbHandle, queryHandle) +// +while (myAddon.queryHasRecords(queryHandle, dbHandle)) { + // retrieve records +} +``` + +In the above example `myAddon.queryHasRecords()` is a method that accepts two +arguments. The first is a database handle and the second is a query handle. +Internally, it unwraps the first argument and casts the resulting pointer to a +native database handle. It then unwraps the second argument and casts the +resulting pointer to a query handle. If the arguments are passed in the wrong +order, the casts will work, however, there is a good chance that the underlying +database operation will fail, or will even cause an invalid memory access. + +To ensure that the pointer retrieved from the first argument is indeed a pointer +to a database handle and, similarly, that the pointer retrieved from the second +argument is indeed a pointer to a query handle, the implementation of +`queryHasRecords()` has to perform a type validation. Retaining the JavaScript +class constructor from which the database handle was instantiated and the +constructor from which the query handle was instantiated in `napi_ref`s can +help, because `napi_instanceof()` can then be used to ensure that the instances +passed into `queryHashRecords()` are indeed of the correct type. + +Unfortunately, `napi_instanceof()` does not protect against prototype +manipulation. For example, the prototype of the database handle instance can be +set to the prototype of the constructor for query handle instances. In this +case, the database handle instance can appear as a query handle instance, and it +will pass the `napi_instanceof()` test for a query handle instance, while still +containing a pointer to a database handle. + +To this end, N-API provides type-tagging capabilities. + +A type tag is a 128-bit integer unique to the addon. N-API provides the +`napi_type_tag` structure for storing a type tag. When such a value is passed +along with a JavaScript object stored in a `napi_value` to +`napi_type_tag_object()`, the JavaScript object will be "marked" with the +type tag. The "mark" is invisible on the JavaScript side. When a JavaScript +object arrives into a native binding, `napi_check_object_type_tag()` can be used +along with the original type tag to determine whether the JavaScript object was +previously "marked" with the type tag. This creates a type-checking capability +of a higher fidelity than `napi_instanceof()` can provide, because such type- +tagging survives prototype manipulation and addon unloading/reloading. + +Continuing the above example, the following skeleton addon implementation +illustrates the use of `napi_type_tag_object()` and +`napi_check_object_type_tag()`. + +```c +// This value is the type tag for a database handle. The command +// +// uuidgen | sed -r -e 's/-//g' -e 's/(.{16})(.*)/0x\1, 0x\2/' +// +// can be used to obtain the two values with which to initialize the structure. +static const napi_type_tag DatabaseHandleTypeTag = { + 0x1edf75a38336451d, 0xa5ed9ce2e4c00c38 +}; + +// This value is the type tag for a query handle. +static const napi_type_tag QueryHandleTypeTag = { + 0x9c73317f9fad44a3, 0x93c3920bf3b0ad6a +}; + +static napi_value +openDatabase(napi_env env, napi_callback_info info) { + napi_status status; + napi_value result; + + // Perform the underlying action which results in a database handle. + DatabaseHandle* dbHandle = open_database(); + + // Create a new, empty JS object. + status = napi_create_object(env, &result); + if (status != napi_ok) return NULL; + + // Tag the object to indicate that it holds a pointer to a `DatabaseHandle`. + status = napi_type_tag_object(env, result, &DatabaseHandleTypeTag); + if (status != napi_ok) return NULL; + + // Store the pointer to the `DatabaseHandle` structure inside the JS object. + status = napi_wrap(env, result, dbHandle, NULL, NULL, NULL); + if (status != napi_ok) return NULL; + + return result; +} + +// Later when we receive a JavaScript object purporting to be a database handle +// we can use `napi_check_object_type_tag()` to ensure that it is indeed such a +// handle. + +static napi_value +query(napi_env env, napi_callback_info info) { + napi_status status; + size_t argc = 2; + napi_value argv[2]; + bool is_db_handle; + + status = napi_get_cb_info(env, info, &argc, argv, NULL, NULL); + if (status != napi_ok) return NULL; + + // Check that the object passed as the first parameter has the previously + // applied tag. + status = napi_check_object_type_tag(env, + argv[0], + &DatabaseHandleTypeTag, + &is_db_handle); + if (status != napi_ok) return NULL; + + // Throw a `TypeError` if it doesn't. + if (!is_db_handle) { + // Throw a TypeError. + return NULL; + } +} +``` + ### napi_define_class + +> Stability: 1 - Experimental + +```c +napi_status napi_type_tag_object(napi_env env, + napi_value js_object, + const napi_type_tag* type_tag); +``` + +* `[in] env`: The environment that the API is invoked under. +* `[in] js_object`: The JavaScript object to be marked. +* `[in] type_tag`: The tag with which the object is to be marked. + +Returns `napi_ok` if the API succeeded. + +Associates the value of the `type_tag` pointer with the JavaScript object. +`napi_check_object_type_tag()` can then be used to compare the tag that was +attached to the object with one owned by the addon to ensure that the object +has the right type. + +If the object already has an associated type tag, this API will return +`napi_invalid_arg`. + +### napi_check_object_type_tag + + +> Stability: 1 - Experimental + +```c +napi_status napi_check_object_type_tag(napi_env env, + napi_value js_object, + const napi_type_tag* type_tag, + bool* result); +``` + +* `[in] env`: The environment that the API is invoked under. +* `[in] js_object`: The JavaScript object whose type tag to examine. +* `[in] type_tag`: The tag with which to compare any tag found on the object. +* `[out] result`: Whether the type tag given matched the type tag on the +object. `false` is also returned if no type tag was found on the object. + +Returns `napi_ok` if the API succeeded. + +Compares the pointer given as `type_tag` with any that can be found on +`js_object`. If no tag is found on `js_object` or, if a tag is found but it does +not match `type_tag`, then `result` is set to `false`. If a tag is found and it +matches `type_tag`, then `result` is set to `true`. + ### napi_add_finalizer -Addons are dynamically-linked shared objects written in C++. The -[`require()`][require] function can load Addons as ordinary Node.js modules. +_Addons_ are dynamically-linked shared objects written in C++. The +[`require()`][require] function can load addons as ordinary Node.js modules. Addons provide an interface between JavaScript and C/C++ libraries. -There are three options for implementing Addons: N-API, nan, or direct +There are three options for implementing addons: N-API, nan, or direct use of internal V8, libuv and Node.js libraries. Unless there is a need for direct access to functionality which is not exposed by N-API, use N-API. -Refer to [C/C++ Addons with N-API](n-api.html) for more information on N-API. +Refer to [C/C++ addons with N-API](n-api.html) for more information on N-API. -When not using N-API, implementing Addons is complicated, +When not using N-API, implementing addons is complicated, involving knowledge of several components and APIs: * V8: the C++ library Node.js uses to provide the @@ -27,27 +27,27 @@ involving knowledge of several components and APIs: access across all major operating systems to many common system tasks, such as interacting with the filesystem, sockets, timers, and system events. libuv also provides a pthreads-like threading abstraction that may be used to - power more sophisticated asynchronous Addons that need to move beyond the + power more sophisticated asynchronous addons that need to move beyond the standard event loop. Addon authors are encouraged to think about how to avoid blocking the event loop with I/O or other time-intensive tasks by off-loading work via libuv to non-blocking system operations, worker threads or a custom use of libuv's threads. -* Internal Node.js libraries. Node.js itself exports C++ APIs that Addons can +* Internal Node.js libraries. Node.js itself exports C++ APIs that addons can use, the most important of which is the `node::ObjectWrap` class. * Node.js includes other statically linked libraries including OpenSSL. These other libraries are located in the `deps/` directory in the Node.js source tree. Only the libuv, OpenSSL, V8 and zlib symbols are purposefully - re-exported by Node.js and may be used to various extents by Addons. See + re-exported by Node.js and may be used to various extents by addons. See [Linking to libraries included with Node.js][] for additional information. All of the following examples are available for [download][] and may -be used as the starting-point for an Addon. +be used as the starting-point for an addon. ## Hello world -This "Hello world" example is a simple Addon, written in C++, that is the +This "Hello world" example is a simple addon, written in C++, that is the equivalent of the following JavaScript code: ```js @@ -84,7 +84,7 @@ NODE_MODULE(NODE_GYP_MODULE_NAME, Initialize) } // namespace demo ``` -All Node.js Addons must export an initialization function following +All Node.js addons must export an initialization function following the pattern: ```cpp @@ -315,7 +315,7 @@ Once the source code has been written, it must be compiled into the binary `addon.node` file. To do so, create a file called `binding.gyp` in the top-level of the project describing the build configuration of the module using a JSON-like format. This file is used by [node-gyp][], a tool written -specifically to compile Node.js Addons. +specifically to compile Node.js addons. ```json { @@ -331,7 +331,7 @@ specifically to compile Node.js Addons. A version of the `node-gyp` utility is bundled and distributed with Node.js as part of `npm`. This version is not made directly available for developers to use and is intended only to support the ability to use the -`npm install` command to compile and install Addons. Developers who wish to +`npm install` command to compile and install addons. Developers who wish to use `node-gyp` directly can install it using the command `npm install -g node-gyp`. See the `node-gyp` [installation instructions][] for more information, including platform-specific requirements. @@ -344,11 +344,11 @@ will generate either a `Makefile` (on Unix platforms) or a `vcxproj` file Next, invoke the `node-gyp build` command to generate the compiled `addon.node` file. This will be put into the `build/Release/` directory. -When using `npm install` to install a Node.js Addon, npm uses its own bundled +When using `npm install` to install a Node.js addon, npm uses its own bundled version of `node-gyp` to perform this same set of actions, generating a -compiled version of the Addon for the user's platform on demand. +compiled version of the addon for the user's platform on demand. -Once built, the binary Addon can be used from within Node.js by pointing +Once built, the binary addon can be used from within Node.js by pointing [`require()`][require] to the built `addon.node` module: ```js @@ -359,12 +359,12 @@ console.log(addon.hello()); // Prints: 'world' ``` -Because the exact path to the compiled Addon binary can vary depending on how -it is compiled (i.e. sometimes it may be in `./build/Debug/`), Addons can use +Because the exact path to the compiled addon binary can vary depending on how +it is compiled (i.e. sometimes it may be in `./build/Debug/`), addons can use the [bindings][] package to load the compiled module. While the `bindings` package implementation is more sophisticated in how it -locates Addon modules, it is essentially using a `try…catch` pattern similar to: +locates addon modules, it is essentially using a `try…catch` pattern similar to: ```js try { @@ -377,7 +377,7 @@ try { ### Linking to libraries included with Node.js Node.js uses statically linked libraries such as V8, libuv and OpenSSL. All -Addons are required to link to V8 and may link to any of the other dependencies +addons are required to link to V8 and may link to any of the other dependencies as well. Typically, this is as simple as including the appropriate `#include <...>` statements (e.g. `#include `) and `node-gyp` will locate the appropriate headers automatically. However, there are a few caveats to be @@ -385,23 +385,23 @@ aware of: * When `node-gyp` runs, it will detect the specific release version of Node.js and download either the full source tarball or just the headers. If the full -source is downloaded, Addons will have complete access to the full set of +source is downloaded, addons will have complete access to the full set of Node.js dependencies. However, if only the Node.js headers are downloaded, then only the symbols exported by Node.js will be available. * `node-gyp` can be run using the `--nodedir` flag pointing at a local Node.js -source image. Using this option, the Addon will have access to the full set of +source image. Using this option, the addon will have access to the full set of dependencies. ### Loading addons using `require()` -The filename extension of the compiled Addon binary is `.node` (as opposed +The filename extension of the compiled addon binary is `.node` (as opposed to `.dll` or `.so`). The [`require()`][require] function is written to look for files with the `.node` file extension and initialize those as dynamically-linked libraries. When calling [`require()`][require], the `.node` extension can usually be -omitted and Node.js will still find and initialize the Addon. One caveat, +omitted and Node.js will still find and initialize the addon. One caveat, however, is that Node.js will first attempt to locate and load modules or JavaScript files that happen to share the same base name. For instance, if there is a file `addon.js` in the same directory as the binary `addon.node`, @@ -411,15 +411,15 @@ and load it instead. ## Native abstractions for Node.js Each of the examples illustrated in this document make direct use of the -Node.js and V8 APIs for implementing Addons. The V8 API can, and has, changed +Node.js and V8 APIs for implementing addons. The V8 API can, and has, changed dramatically from one V8 release to the next (and one major Node.js release to -the next). With each change, Addons may need to be updated and recompiled in +the next). With each change, addons may need to be updated and recompiled in order to continue functioning. The Node.js release schedule is designed to minimize the frequency and impact of such changes but there is little that Node.js can do to ensure stability of the V8 APIs. The [Native Abstractions for Node.js][] (or `nan`) provide a set of tools that -Addon developers are recommended to use to keep compatibility between past and +addon developers are recommended to use to keep compatibility between past and future releases of V8 and Node.js. See the `nan` [examples][] for an illustration of how it can be used. @@ -427,10 +427,10 @@ illustration of how it can be used. > Stability: 2 - Stable -N-API is an API for building native Addons. It is independent from +N-API is an API for building native addons. It is independent from the underlying JavaScript runtime (e.g. V8) and is maintained as part of Node.js itself. This API will be Application Binary Interface (ABI) stable -across versions of Node.js. It is intended to insulate Addons from +across versions of Node.js. It is intended to insulate addons from changes in the underlying JavaScript engine and allow modules compiled for one version to run on later versions of Node.js without recompilation. Addons are built/packaged with the same approach/tools @@ -479,11 +479,11 @@ NAPI_MODULE(NODE_GYP_MODULE_NAME, init) ``` The functions available and how to use them are documented in -[C/C++ Addons with N-API](n-api.html). +[C/C++ addons with N-API](n-api.html). ## Addon examples -Following are some example Addons intended to help developers get started. The +Following are some example addons intended to help developers get started. The examples make use of the V8 APIs. Refer to the online [V8 reference][v8-docs] for help with the various V8 calls, and V8's [Embedder's Guide][] for an explanation of several concepts used such as handles, scopes, function @@ -509,7 +509,7 @@ filename to the `sources` array: "sources": ["addon.cc", "myexample.cc"] ``` -Once the `binding.gyp` file is ready, the example Addons can be configured and +Once the `binding.gyp` file is ready, the example addons can be configured and built using `node-gyp`: ```console @@ -583,7 +583,7 @@ NODE_MODULE(NODE_GYP_MODULE_NAME, Init) } // namespace demo ``` -Once compiled, the example Addon can be required and used from within Node.js: +Once compiled, the example addon can be required and used from within Node.js: ```js // test.js @@ -594,7 +594,7 @@ console.log('This should be eight:', addon.add(3, 5)); ### Callbacks -It is common practice within Addons to pass JavaScript functions to a C++ +It is common practice within addons to pass JavaScript functions to a C++ function and execute them from there. The following example illustrates how to invoke such callbacks: @@ -635,7 +635,7 @@ NODE_MODULE(NODE_GYP_MODULE_NAME, Init) ``` This example uses a two-argument form of `Init()` that receives the full -`module` object as the second argument. This allows the Addon to completely +`module` object as the second argument. This allows the addon to completely overwrite `exports` with a single function instead of adding the function as a property of `exports`. From 3b9a252dccb0ca296f18379ae8a5716d892a2cca Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Tue, 28 Jul 2020 21:42:36 -0700 Subject: [PATCH 12/19] doc: simplify and clarify console.assert() documentation PR-URL: https://github.com/nodejs/node/pull/34544 Reviewed-By: James M Snell Reviewed-By: Trivikram Kamat Reviewed-By: Luigi Pinca --- doc/api/console.md | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/doc/api/console.md b/doc/api/console.md index a50d09aae87515..9e223e6ee9d2be 100644 --- a/doc/api/console.md +++ b/doc/api/console.md @@ -152,24 +152,23 @@ changes: * `value` {any} The value tested for being truthy. * `...message` {any} All arguments besides `value` are used as error message. -A simple assertion test that verifies whether `value` is truthy. If it is not, -or `value` is not passed, -`Assertion failed` is logged. If provided, the error `message` is formatted -using [`util.format()`][] by passing along all message arguments. The output is -used as the error message. +`console.assert()` writes a message if `value` is [falsy][] or omitted. It only +writes a message and does not otherwise affect execution. The output always +starts with `"Assertion failed"`. If provided, `message` is formatted using +[`util.format()`][]. + +If `value` is [truthy][], nothing happens. ```js console.assert(true, 'does nothing'); -// OK + console.assert(false, 'Whoops %s work', 'didn\'t'); // Assertion failed: Whoops didn't work + console.assert(); // Assertion failed ``` -Calling `console.assert()` with a falsy assertion will only cause the `message` -to be printed to the console without interrupting execution of subsequent code. - ### `console.clear()` From 849d9e7b90ebaae930a28a7994b3a56ec4f280c6 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Thu, 30 Jul 2020 16:58:08 +0200 Subject: [PATCH 14/19] http: provide keep-alive timeout response header In http 1.1 persistent connection protocol there is a timing race where the client sends the request and then the server kills the connection (due to inactivity) before receiving the client's request. By providing a keep-alive header it is possible to provide the client a hint of when idle timeout would occur and avoid the race. Fixes: https://github.com/nodejs/node/issues/34560 PR-URL: https://github.com/nodejs/node/pull/34561 Reviewed-By: Anna Henningsen Reviewed-By: Matteo Collina Reviewed-By: Luigi Pinca Reviewed-By: Zeyu Yang Reviewed-By: Trivikram Kamat Reviewed-By: Pranshu Srivastava --- lib/_http_outgoing.js | 7 +++++ lib/_http_server.js | 1 + test/parallel/test-http-keep-alive-timeout.js | 28 +++++++++++++++++++ 3 files changed, 36 insertions(+) create mode 100644 test/parallel/test-http-keep-alive-timeout.js diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js index 868c74d3518cc0..f444be8687db34 100644 --- a/lib/_http_outgoing.js +++ b/lib/_http_outgoing.js @@ -28,6 +28,7 @@ const { ObjectKeys, ObjectPrototypeHasOwnProperty, ObjectSetPrototypeOf, + MathFloor, Symbol, } = primordials; @@ -123,6 +124,8 @@ function OutgoingMessage() { this._header = null; this[kOutHeaders] = null; + this._keepAliveTimeout = 0; + this._onPendingData = noopPendingOutput; } ObjectSetPrototypeOf(OutgoingMessage.prototype, Stream.prototype); @@ -424,6 +427,10 @@ function _storeHeader(firstLine, headers) { (state.contLen || this.useChunkedEncodingByDefault || this.agent); if (shouldSendKeepAlive) { header += 'Connection: keep-alive\r\n'; + if (this._keepAliveTimeout) { + const timeoutSeconds = MathFloor(this._keepAliveTimeout) / 1000; + header += `Keep-Alive: timeout=${timeoutSeconds}\r\n`; + } } else { this._last = true; header += 'Connection: close\r\n'; diff --git a/lib/_http_server.js b/lib/_http_server.js index 084b67b6b3a6fe..0da43c3e3a8159 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -766,6 +766,7 @@ function parserOnIncoming(server, socket, state, req, keepAlive) { } const res = new server[kServerResponse](req); + res._keepAliveTimeout = server.keepAliveTimeout; res._onPendingData = updateOutgoingData.bind(undefined, socket, state); res.shouldKeepAlive = keepAlive; diff --git a/test/parallel/test-http-keep-alive-timeout.js b/test/parallel/test-http-keep-alive-timeout.js new file mode 100644 index 00000000000000..fccb267b8e9ee2 --- /dev/null +++ b/test/parallel/test-http-keep-alive-timeout.js @@ -0,0 +1,28 @@ +'use strict'; + +const common = require('../common'); +const http = require('http'); +const assert = require('assert'); + +const server = http.createServer(common.mustCall((req, res) => { + const body = 'hello world\n'; + + res.writeHead(200, { 'Content-Length': body.length }); + res.write(body); + res.end(); +})); +server.keepAliveTimeout = 12000; + +const agent = new http.Agent({ maxSockets: 1, keepAlive: true }); + +server.listen(0, common.mustCall(function() { + http.get({ + path: '/', port: this.address().port, agent: agent + }, common.mustCall((response) => { + response.resume(); + assert.strictEqual( + response.headers['keep-alive'], 'timeout=12'); + server.close(); + agent.destroy(); + })); +})); From 4268c6112feaf82d7ca171c381b269ebfcf403f7 Mon Sep 17 00:00:00 2001 From: Mary Marchini Date: Fri, 31 Jul 2020 16:14:41 -0700 Subject: [PATCH 15/19] build: fix auto-start-ci script path PR-URL: https://github.com/nodejs/node/pull/34588 Reviewed-By: Rich Trott Reviewed-By: Richard Lau --- .github/workflows/auto-start-ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/auto-start-ci.yml b/.github/workflows/auto-start-ci.yml index d4c364e2bb2f52..826b80caef0c58 100644 --- a/.github/workflows/auto-start-ci.yml +++ b/.github/workflows/auto-start-ci.yml @@ -62,4 +62,4 @@ jobs: ncu-config set repo ${{ env.REPOSITORY }} - name: Start CI - run: ./tools/start-ci.sh ${{ secrets.GITHUB_TOKEN }} ${{ env.OWNER }} ${{ env.REPOSITORY }} $(echo '${{ steps.get_prs_for_ci.outputs.data }}' | jq '.repository.pullRequests.nodes | map(.number) | .[]') + run: ./tools/actions/start-ci.sh ${{ secrets.GITHUB_TOKEN }} ${{ env.OWNER }} ${{ env.REPOSITORY }} $(echo '${{ steps.get_prs_for_ci.outputs.data }}' | jq '.repository.pullRequests.nodes | map(.number) | .[]') From 9b6c16c5acb4e47794fec4f2c7aa4d79fecda46d Mon Sep 17 00:00:00 2001 From: Mary Marchini Date: Fri, 31 Jul 2020 17:01:21 -0700 Subject: [PATCH 16/19] build: don't run auto-start-ci on push PR-URL: https://github.com/nodejs/node/pull/34588 Reviewed-By: Rich Trott Reviewed-By: Richard Lau --- .github/workflows/auto-start-ci.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/workflows/auto-start-ci.yml b/.github/workflows/auto-start-ci.yml index 826b80caef0c58..b6be44b6e9c65d 100644 --- a/.github/workflows/auto-start-ci.yml +++ b/.github/workflows/auto-start-ci.yml @@ -2,7 +2,6 @@ name: Auto Start CI on: - push: schedule: # `schedule` event is used instead of `pull_request` because when a # `pull_requesst` event is triggered on a PR from a fork, GITHUB_TOKEN will From 45fa216ee533abc189d4b9bf0fc8f40430af7ef4 Mon Sep 17 00:00:00 2001 From: Derek Lewis Date: Tue, 7 Jul 2020 09:32:01 -0400 Subject: [PATCH 17/19] esm: fix hook mistypes and links to types Prior to this commit, the custom loader hooks were: * missing the Node.js API ref docs types * missing the function signature from their sections * linking directly to the specification (not customary) * had an inconsistent non-nullable JSDoc promise return * had JSDoc object properties that weren't alpha-sorted * designated set of non-nullable types when single was fine Notes: https://www.typescriptlang.org/play/index.html?strictNullChecks=true&useJavaScript=true#code/PQKhCgAIUgBAHAhgJ0QW0gbwM4BdkCWAdgOYC+k28ApgMYEBmB1yUMCK6WmbkftAeyIATArgJDsALkgBCAILJUATwA8eQqQB8AGl58kyakVwBVAEoAZGbIAUG4iUgAfSAFcR1JkWrCAlHrQkGQUgibUAB64vByoGJgAYh604kIUwl6IbgA2uObU2ALZAG7UMUa4bshE2FgACsgCaATY1KqY7sjZMg6kwVpkbMDgkfACyLiQiNjKRLSQDMmpRJBGhSXU9jT0TCw6kGG4kbj7GQxZufnrpX5YUAeSkx2GxmZWkAC8kEQ52cGfD3CUQA3PdGJBbABZRC4AAWADpUCImrZblpIAAGeEAVluHWAwEgAGUmtRAaJlvD7nwCZAEuNKKTIAzENk-lQ6IxmMhsKcBIy0GTaG48E1INkBCQCPMGAy1kVio4qXwaYT5NkAO6IZS1CpVFaIFYCABGACs6JMBAxIHCybLkPEqt1IOp8I4BsqVXrqncVSqnTIXiYLJZIAB+al+77UDWQENbTm7ZD7INvSx+eGwozWqSRv0+WPxjk7bkZrNeQIqsigviDPjgqEwhFI4Qo26qTE4vGQWnyIgCW3IcliCRET2qyAAdVhxgOrOyjkgAAMzhc8gUFdQl-tbVNkCQ3IKTLVaIbIMayWgBKJdsJ4ZAAJIrOEtSO00+tSBiADktUQwlEPpDQHGch2KVk3DtBk0BhWhYUXMIKVHVlIFGcZcGwcdVmoSofVXHJ12uTZiy5PZfSjeFKMOY5Kz9RCR0kGQAG1KPhaiojYoQkMkfZv2AwcAFp6OWb8AF1aOCPwa2Ce5aQAES8FgbX5AA5a9qHhU1antKY2WZQdKG2UieU9b0Vnwy4Nw2BMSzI9iTkgCzCM3KTwDIIA https://closure-compiler.appspot.com/home#code%3D%252F%252F%2520%253D%253DClosureCompiler%253D%253D%250A%252F%252F%2520%2540compilation_level%2520ADVANCED_OPTIMIZATIONS%250A%252F%252F%2520%2540warning_level%2520VERBOSE%250A%252F%252F%2520%2540jscomp_error%2520strictCheckTypes%250A%252F%252F%2520%2540language_out%2520ECMASCRIPT_NEXT%250A%252F%252F%2520%2540checks_only%250A%252F%252F%2520%253D%253D%252FClosureCompiler%253D%253D%250A%250A%252F**%250A%2520*%2520%2540param%2520%257Bstring%257D%2520specifier%250A%2520*%2520%2540param%2520%257B%257B%250A%2520*%2520%2520%2520conditions%253A%2520!Array%253Cstring%253E%252C%250A%2520*%2520%2520%2520parentURL%253A%2520!(string%2520%257C%2520undefined)%252C%250A%2520*%2520%257D%257D%2520context%250A%2520*%2520%2540param%2520%257BFunction%257D%2520defaultResolve%250A%2520*%2520%2540returns%2520%257BPromise%253C%257B%2520url%253A%2520string%2520%257D%253E%257D%250A%2520*%252F%250Aexport%2520async%2520function%2520resolve(specifier%252C%2520context%252C%2520defaultResolve)%2520%257B%250A%2520%2520const%2520%257B%2520parentURL%2520%253D%2520null%2520%257D%2520%253D%2520context%253B%250A%2520%2520if%2520(Math.random()%2520%253E%25200.5)%2520%257B%2520%252F%252F%2520Some%2520condition.%250A%2520%2520%2520%2520%252F%252F%2520For%2520some%2520or%2520all%2520specifiers%252C%2520do%2520some%2520custom%2520logic%2520for%2520resolving.%250A%2520%2520%2520%2520%252F%252F%2520Always%2520return%2520an%2520object%2520of%2520the%2520form%2520%257Burl%253A%2520%253Cstring%253E%257D.%250A%2520%2520%2520%2520return%2520%257B%250A%2520%2520%2520%2520%2520%2520url%253A%2520parentURL%2520%253F%250A%2520%2520%2520%2520%2520%2520%2520%2520new%2520URL(specifier%252C%2520parentURL).href%2520%253A%250A%2520%2520%2520%2520%2520%2520%2520%2520new%2520URL(specifier).href%252C%250A%2520%2520%2520%2520%257D%253B%250A%2520%2520%257D%250A%2520%2520if%2520(Math.random()%2520%253C%25200.5)%2520%257B%2520%252F%252F%2520Another%2520condition.%250A%2520%2520%2520%2520%252F%252F%2520When%2520calling%2520%2560defaultResolve%2560%252C%2520the%2520arguments%2520can%2520be%2520modified.%2520In%2520this%250A%2520%2520%2520%2520%252F%252F%2520case%2520it's%2520adding%2520another%2520value%2520for%2520matching%2520conditional%2520exports.%250A%2520%2520%2520%2520return%2520defaultResolve(specifier%252C%2520%257B%250A%2520%2520%2520%2520%2520%2520...context%252C%250A%2520%2520%2520%2520%2520%2520conditions%253A%2520%255B...context.conditions%252C%2520'another-condition'%255D%252C%250A%2520%2520%2520%2520%257D)%253B%250A%2520%2520%257D%250A%2520%2520%252F%252F%2520Defer%2520to%2520Node.js%2520for%2520all%2520other%2520specifiers.%250A%2520%2520return%2520defaultResolve(specifier%252C%2520context%252C%2520defaultResolve)%253B%250A%257D PR-URL: https://github.com/nodejs/node/pull/34240 Reviewed-By: Rich Trott Reviewed-By: Guy Bedford --- doc/api/esm.md | 83 ++++++++++++++++++++++++++++++++++---------------- 1 file changed, 56 insertions(+), 27 deletions(-) diff --git a/doc/api/esm.md b/doc/api/esm.md index c9bb7473a8e689..e98cae376f6c5b 100644 --- a/doc/api/esm.md +++ b/doc/api/esm.md @@ -1184,11 +1184,19 @@ CommonJS modules loaded. ### Hooks -#### resolve hook +#### `resolve(specifier, context, defaultResolve)` > Note: The loaders API is being redesigned. This hook may disappear or its > signature may change. Do not rely on the API described below. +* `specifier` {string} +* `context` {Object} + * `conditions` {string[]} + * `parentURL` {string} +* `defaultResolve` {Function} +* Returns: {Object} + * `url` {string} + The `resolve` hook returns the resolved file URL for a given module specifier and parent URL. The module specifier is the string in an `import` statement or `import()` expression, and the parent URL is the URL of the module that imported @@ -1209,11 +1217,11 @@ Node.js module specifier resolution behavior_ when calling `defaultResolve`, the /** * @param {string} specifier * @param {{ + * conditions: !Array, * parentURL: !(string | undefined), - * conditions: !(Array), * }} context * @param {Function} defaultResolve - * @returns {!(Promise<{ url: string }>)} + * @returns {Promise<{ url: string }>} */ export async function resolve(specifier, context, defaultResolve) { const { parentURL = null } = context; @@ -1239,29 +1247,34 @@ export async function resolve(specifier, context, defaultResolve) { } ``` -#### getFormat hook +#### `getFormat(url, context, defaultGetFormat)` > Note: The loaders API is being redesigned. This hook may disappear or its > signature may change. Do not rely on the API described below. +* `url` {string} +* `context` {Object} +* `defaultGetFormat` {Function} +* Returns: {Object} + * `format` {string} + The `getFormat` hook provides a way to define a custom method of determining how a URL should be interpreted. The `format` returned also affects what the acceptable forms of source values are for a module when parsing. This can be one of the following: -| `format` | Description | Acceptable Types For `source` Returned by `getSource` or `transformSource` | -| --- | --- | --- | -| `'builtin'` | Load a Node.js builtin module | Not applicable | -| `'commonjs'` | Load a Node.js CommonJS module | Not applicable | -| `'json'` | Load a JSON file | { [ArrayBuffer][], [string][], [TypedArray][] } | -| `'module'` | Load an ES module | { [ArrayBuffer][], [string][], [TypedArray][] } | -| `'wasm'` | Load a WebAssembly module | { [ArrayBuffer][], [string][], [TypedArray][] } | +| `format` | Description | Acceptable Types For `source` Returned by `getSource` or `transformSource` | +| ------------ | ------------------------------ | -------------------------------------------------------------------------- | +| `'builtin'` | Load a Node.js builtin module | Not applicable | +| `'commonjs'` | Load a Node.js CommonJS module | Not applicable | +| `'json'` | Load a JSON file | { [`string`][], [`ArrayBuffer`][], [`TypedArray`][] } | +| `'module'` | Load an ES module | { [`string`][], [`ArrayBuffer`][], [`TypedArray`][] } | +| `'wasm'` | Load a WebAssembly module | { [`ArrayBuffer`][], [`TypedArray`][] } | Note: These types all correspond to classes defined in ECMAScript. -* The specific [ArrayBuffer][] object is a [SharedArrayBuffer][]. -* The specific [string][] object is not the class constructor, but an instance. -* The specific [TypedArray][] object is a [Uint8Array][]. +* The specific [`ArrayBuffer`][] object is a [`SharedArrayBuffer`][]. +* The specific [`TypedArray`][] object is a [`Uint8Array`][]. Note: If the source value of a text-based format (i.e., `'json'`, `'module'`) is not a string, it will be converted to a string using [`util.TextDecoder`][]. @@ -1287,11 +1300,18 @@ export async function getFormat(url, context, defaultGetFormat) { } ``` -#### getSource hook +#### `getSource(url, context, defaultGetSource)` > Note: The loaders API is being redesigned. This hook may disappear or its > signature may change. Do not rely on the API described below. +* `url` {string} +* `context` {Object} + * `format` {string} +* `defaultGetSource` {Function} +* Returns: {Object} + * `source` {string|SharedArrayBuffer|Uint8Array} + The `getSource` hook provides a way to define a custom method for retrieving the source code of an ES module specifier. This would allow a loader to potentially avoid reading files from disk. @@ -1301,7 +1321,7 @@ potentially avoid reading files from disk. * @param {string} url * @param {{ format: string }} context * @param {Function} defaultGetSource - * @returns {Promise<{ source: !(SharedArrayBuffer | string | Uint8Array) }>} + * @returns {Promise<{ source: !(string | SharedArrayBuffer | Uint8Array) }>} */ export async function getSource(url, context, defaultGetSource) { const { format } = context; @@ -1317,11 +1337,18 @@ export async function getSource(url, context, defaultGetSource) { } ``` -#### transformSource hook +#### `transformSource(source, context, defaultTransformSource)` > Note: The loaders API is being redesigned. This hook may disappear or its > signature may change. Do not rely on the API described below. +* `source` {string|SharedArrayBuffer|Uint8Array} +* `context` {Object} + * `format` {string} + * `url` {string} +* Returns: {Object} + * `source` {string|SharedArrayBuffer|Uint8Array} + The `transformSource` hook provides a way to modify the source code of a loaded ES module file after the source string has been loaded but before Node.js has done anything with it. @@ -1332,13 +1359,13 @@ unknown-to-Node.js file extensions. See the [transpiler loader example][] below. ```js /** - * @param {!(SharedArrayBuffer | string | Uint8Array)} source + * @param {!(string | SharedArrayBuffer | Uint8Array)} source * @param {{ - * url: string, * format: string, + * url: string, * }} context * @param {Function} defaultTransformSource - * @returns {Promise<{ source: !(SharedArrayBuffer | string | Uint8Array) }>} + * @returns {Promise<{ source: !(string | SharedArrayBuffer | Uint8Array) }>} */ export async function transformSource(source, context, defaultTransformSource) { const { url, format } = context; @@ -1354,11 +1381,13 @@ export async function transformSource(source, context, defaultTransformSource) { } ``` -#### getGlobalPreloadCode hook +#### `getGlobalPreloadCode()` > Note: The loaders API is being redesigned. This hook may disappear or its > signature may change. Do not rely on the API described below. +* Returns: {string} + Sometimes it can be necessary to run some code inside of the same global scope that the application will run in. This hook allows to return a string that will be ran as sloppy-mode script on startup. @@ -1909,12 +1938,12 @@ success! [`import`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/import [`module.createRequire()`]: modules.html#modules_module_createrequire_filename [`module.syncBuiltinESMExports()`]: modules.html#modules_module_syncbuiltinesmexports -[`transformSource` hook]: #esm_code_transformsource_code_hook -[ArrayBuffer]: https://www.ecma-international.org/ecma-262/6.0/#sec-arraybuffer-constructor -[SharedArrayBuffer]: https://tc39.es/ecma262/#sec-sharedarraybuffer-constructor -[string]: https://www.ecma-international.org/ecma-262/6.0/#sec-string-constructor -[TypedArray]: https://www.ecma-international.org/ecma-262/6.0/#sec-typedarray-objects -[Uint8Array]: https://www.ecma-international.org/ecma-262/6.0/#sec-uint8array +[`transformSource` hook]: #esm_transformsource_source_context_defaulttransformsource +[`ArrayBuffer`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/ArrayBuffer +[`SharedArrayBuffer`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SharedArrayBuffer +[`string`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String +[`TypedArray`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/TypedArray +[`Uint8Array`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Uint8Array [`util.TextDecoder`]: util.html#util_class_util_textdecoder [import an ES or CommonJS module for its side effects only]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/import#Import_a_module_for_its_side_effects_only [special scheme]: https://url.spec.whatwg.org/#special-scheme From aeaf16194bac6510a48019b4b55481006f11f714 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Thu, 30 Jul 2020 10:33:33 -0700 Subject: [PATCH 18/19] doc: revise N-API versions matrix text Revise text for clarity, brevity, and conformance with our style guide. PR-URL: https://github.com/nodejs/node/pull/34566 Reviewed-By: Gus Caplan Reviewed-By: Michael Dawson Reviewed-By: Richard Lau Reviewed-By: James M Snell --- doc/api/n-api.md | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/doc/api/n-api.md b/doc/api/n-api.md index 8dff1e0e15f1bf..6b84719362311f 100644 --- a/doc/api/n-api.md +++ b/doc/api/n-api.md @@ -251,13 +251,11 @@ listed as supporting a later version. | v13.x | v13.0.0 | v13.0.0 | v13.0.0 | v13.0.0 | v13.0.0 | | | v14.x | v14.0.0 | v14.0.0 | v14.0.0 | v14.0.0 | v14.0.0 | v14.0.0 | -\* Indicates that the N-API version was released as experimental +\* N-API was experimental. -\*\* First version which matches version 1 in later releases. While v8.0.0 -included N-API as experimental, version 1 continued to evolve until -v8.6.0 and therefore the shape of the API in earlier versions is not -truly version 1 (in hindsight we should have called it version 0). -We recommend version 3 or later. +\*\* Node.js 8.0.0 included N-API as experimental. It was released as N-API +version 1 but continued to evolve until Node.js 8.6.0. The API is different in +versions prior to Node.js 8.6.0. We recommend N-API version 3 or later. The N-APIs associated strictly with accessing ECMAScript features from native code can be found separately in `js_native_api.h` and `js_native_api_types.h`. From 73d713b16e764a2b52028803e14ce4ed8343a0a5 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Thu, 30 Jul 2020 10:46:22 -0700 Subject: [PATCH 19/19] test: change Fixes: to Refs: Tests don't fix things generally, so use "Refs:" to refer people to GitHub issues. PR-URL: https://github.com/nodejs/node/pull/34568 Reviewed-By: Richard Lau Reviewed-By: James M Snell Reviewed-By: Luigi Pinca --- test/parallel/test-fs-symlink.js | 2 +- test/parallel/test-http-outgoing-message-inheritance.js | 4 ++-- test/parallel/test-http-server-response-standalone.js | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/test/parallel/test-fs-symlink.js b/test/parallel/test-fs-symlink.js index 28938f41d66010..45bc99b48980a8 100644 --- a/test/parallel/test-fs-symlink.js +++ b/test/parallel/test-fs-symlink.js @@ -58,7 +58,7 @@ fs.symlink(linkData, linkPath, common.mustCall(function(err) { })); tmpdir.refresh(); - // Fixes: https://github.com/nodejs/node/issues/34514 + // Refs: https://github.com/nodejs/node/issues/34514 fs.symlinkSync(Buffer.from(linkData), linkPath); })); diff --git a/test/parallel/test-http-outgoing-message-inheritance.js b/test/parallel/test-http-outgoing-message-inheritance.js index 335a9a28956108..d0da4c68c3118f 100644 --- a/test/parallel/test-http-outgoing-message-inheritance.js +++ b/test/parallel/test-http-outgoing-message-inheritance.js @@ -6,8 +6,8 @@ const { Writable } = require('stream'); const assert = require('assert'); // Check that OutgoingMessage can be used without a proper Socket -// Fixes: https://github.com/nodejs/node/issues/14386 -// Fixes: https://github.com/nodejs/node/issues/14381 +// Refs: https://github.com/nodejs/node/issues/14386 +// Refs: https://github.com/nodejs/node/issues/14381 class Response extends OutgoingMessage { _implicitHeader() {} diff --git a/test/parallel/test-http-server-response-standalone.js b/test/parallel/test-http-server-response-standalone.js index 3c91dd0889b066..ec6d1e89e38525 100644 --- a/test/parallel/test-http-server-response-standalone.js +++ b/test/parallel/test-http-server-response-standalone.js @@ -6,8 +6,8 @@ const { Writable } = require('stream'); const assert = require('assert'); // Check that ServerResponse can be used without a proper Socket -// Fixes: https://github.com/nodejs/node/issues/14386 -// Fixes: https://github.com/nodejs/node/issues/14381 +// Refs: https://github.com/nodejs/node/issues/14386 +// Refs: https://github.com/nodejs/node/issues/14381 const res = new ServerResponse({ method: 'GET',