From a4fdb1abe0844d86b4cbfcc4051794656e7d746e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C3=ABl=20Zasso?= Date: Thu, 28 Sep 2023 14:50:20 +0200 Subject: [PATCH 01/31] lib,test: do not hardcode Buffer.kMaxLength V8 will soon support typed arrays as large as the maximum array buffer length. This patch replaces hardcoded values related to Buffer.kMaxLength with the actual constant. It also fixes a test that was passing by accident. Refs: https://github.com/v8/v8/commit/44b299590083b888637c79fb5632806e607ab861 PR-URL: https://github.com/nodejs/node/pull/49876 Reviewed-By: Richard Lau Reviewed-By: Luigi Pinca Reviewed-By: Yagiz Nizipli --- lib/internal/blob.js | 8 +++++--- test/parallel/test-blob-buffer-too-large.js | 6 +++--- test/parallel/test-buffer-alloc.js | 9 ++++++--- test/parallel/test-buffer-over-max-length.js | 10 ---------- .../parallel/test-buffer-tostring-rangeerror.js | 17 +++++++++++------ 5 files changed, 25 insertions(+), 25 deletions(-) diff --git a/lib/internal/blob.js b/lib/internal/blob.js index a54adb615fbc179..b68037612ab8b30 100644 --- a/lib/internal/blob.js +++ b/lib/internal/blob.js @@ -24,6 +24,9 @@ const { concat, getDataObject, } = internalBinding('blob'); +const { + kMaxLength, +} = internalBinding('buffer'); const { TextDecoder, @@ -62,7 +65,6 @@ const { } = require('internal/errors'); const { - isUint32, validateDictionary, } = require('internal/validators'); @@ -160,8 +162,8 @@ class Blob { return src; }); - if (!isUint32(length)) - throw new ERR_BUFFER_TOO_LARGE(0xFFFFFFFF); + if (length > kMaxLength) + throw new ERR_BUFFER_TOO_LARGE(kMaxLength); this[kHandle] = _createBlob(sources_, length); this[kLength] = length; diff --git a/test/parallel/test-blob-buffer-too-large.js b/test/parallel/test-blob-buffer-too-large.js index 2fd8b8754bd593a..a9cf53b025bbff5 100644 --- a/test/parallel/test-blob-buffer-too-large.js +++ b/test/parallel/test-blob-buffer-too-large.js @@ -3,17 +3,17 @@ const common = require('../common'); const assert = require('assert'); -const { Blob } = require('buffer'); +const { Blob, kMaxLength } = require('buffer'); if (common.isFreeBSD) common.skip('Oversized buffer make the FreeBSD CI runner crash'); try { - new Blob([new Uint8Array(0xffffffff), [1]]); + new Blob([new Uint8Array(kMaxLength), [1]]); } catch (e) { if ( e.message === 'Array buffer allocation failed' || - e.message === 'Invalid typed array length: 4294967295' + e.message === `Invalid typed array length: ${kMaxLength}` ) { common.skip( 'Insufficient memory on this platform for oversized buffer test.' diff --git a/test/parallel/test-buffer-alloc.js b/test/parallel/test-buffer-alloc.js index c6b728027057ece..aad9c6bcab69e97 100644 --- a/test/parallel/test-buffer-alloc.js +++ b/test/parallel/test-buffer-alloc.js @@ -4,13 +4,16 @@ const common = require('../common'); const assert = require('assert'); const vm = require('vm'); -const SlowBuffer = require('buffer').SlowBuffer; +const { + SlowBuffer, + kMaxLength, +} = require('buffer'); // Verify the maximum Uint8Array size. There is no concrete limit by spec. The // internal limits should be updated if this fails. assert.throws( - () => new Uint8Array(2 ** 32 + 1), - { message: 'Invalid typed array length: 4294967297' } + () => new Uint8Array(kMaxLength + 1), + { message: `Invalid typed array length: ${kMaxLength + 1}` }, ); const b = Buffer.allocUnsafe(1024); diff --git a/test/parallel/test-buffer-over-max-length.js b/test/parallel/test-buffer-over-max-length.js index d2df358cc00ca4b..f29d6b62d4aa408 100644 --- a/test/parallel/test-buffer-over-max-length.js +++ b/test/parallel/test-buffer-over-max-length.js @@ -12,18 +12,8 @@ const bufferMaxSizeMsg = { name: 'RangeError', }; -assert.throws(() => Buffer((-1 >>> 0) + 2), bufferMaxSizeMsg); -assert.throws(() => SlowBuffer((-1 >>> 0) + 2), bufferMaxSizeMsg); -assert.throws(() => Buffer.alloc((-1 >>> 0) + 2), bufferMaxSizeMsg); -assert.throws(() => Buffer.allocUnsafe((-1 >>> 0) + 2), bufferMaxSizeMsg); -assert.throws(() => Buffer.allocUnsafeSlow((-1 >>> 0) + 2), bufferMaxSizeMsg); - assert.throws(() => Buffer(kMaxLength + 1), bufferMaxSizeMsg); assert.throws(() => SlowBuffer(kMaxLength + 1), bufferMaxSizeMsg); assert.throws(() => Buffer.alloc(kMaxLength + 1), bufferMaxSizeMsg); assert.throws(() => Buffer.allocUnsafe(kMaxLength + 1), bufferMaxSizeMsg); assert.throws(() => Buffer.allocUnsafeSlow(kMaxLength + 1), bufferMaxSizeMsg); - -// issue GH-4331 -assert.throws(() => Buffer.allocUnsafe(0x100000001), bufferMaxSizeMsg); -assert.throws(() => Buffer.allocUnsafe(0xFFFFFFFFF), bufferMaxSizeMsg); diff --git a/test/parallel/test-buffer-tostring-rangeerror.js b/test/parallel/test-buffer-tostring-rangeerror.js index d2e1e0d6e46438f..0ebea759b5c42be 100644 --- a/test/parallel/test-buffer-tostring-rangeerror.js +++ b/test/parallel/test-buffer-tostring-rangeerror.js @@ -1,17 +1,22 @@ 'use strict'; require('../common'); -// This test ensures that Node.js throws a RangeError when trying to convert a -// gigantic buffer into a string. +// This test ensures that Node.js throws an Error when trying to convert a +// large buffer into a string. // Regression test for https://github.com/nodejs/node/issues/649. const assert = require('assert'); -const SlowBuffer = require('buffer').SlowBuffer; +const { + SlowBuffer, + constants: { + MAX_STRING_LENGTH, + }, +} = require('buffer'); -const len = 1422561062959; +const len = MAX_STRING_LENGTH + 1; const message = { - code: 'ERR_OUT_OF_RANGE', - name: 'RangeError', + code: 'ERR_STRING_TOO_LONG', + name: 'Error', }; assert.throws(() => Buffer(len).toString('utf8'), message); assert.throws(() => SlowBuffer(len).toString('utf8'), message); From e28dbe1c2bcbd0d4eb82f22823ee045a2a8466eb Mon Sep 17 00:00:00 2001 From: Khafra Date: Thu, 28 Sep 2023 09:01:30 -0400 Subject: [PATCH 02/31] lib: add WebSocket client fixup add test lint fixup update doc/node.1 PR-URL: https://github.com/nodejs/node/pull/49830 Reviewed-By: Matteo Collina Reviewed-By: Robert Nagy --- .eslintrc.js | 1 + doc/api/cli.md | 10 ++++++ doc/api/globals.md | 13 ++++++++ doc/node.1 | 3 ++ lib/internal/process/pre_execution.js | 48 ++++++++++++++++----------- src/node_options.cc | 5 +++ src/node_options.h | 1 + test/common/globals.js | 1 + test/common/index.js | 3 ++ test/parallel/test-websocket.js | 7 ++++ 10 files changed, 72 insertions(+), 20 deletions(-) create mode 100644 test/parallel/test-websocket.js diff --git a/.eslintrc.js b/.eslintrc.js index f46a64bcbf2acd5..8b462e0777c5d42 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -361,5 +361,6 @@ module.exports = { WritableStream: 'readable', WritableStreamDefaultWriter: 'readable', WritableStreamDefaultController: 'readable', + WebSocket: 'readable', }, }; diff --git a/doc/api/cli.md b/doc/api/cli.md index 1da38c1739126bf..013256d776bac7a 100644 --- a/doc/api/cli.md +++ b/doc/api/cli.md @@ -738,6 +738,14 @@ added: v12.3.0 Enable experimental WebAssembly module support. +### `--experimental-websocket` + + + +Enable experimental [`WebSocket`][] support. + ### `--force-context-aware` + +> Stability: 1 - Experimental. + +A browser-compatible implementation of [`WebSocket`][]. Enable this API +with the [`--experimental-websocket`][] CLI flag. + ## Class: `WritableStream` diff --git a/tools/doc/allhtml.mjs b/tools/doc/allhtml.mjs index cdf7140f7284699..ccf3a10ea7f95bb 100644 --- a/tools/doc/allhtml.mjs +++ b/tools/doc/allhtml.mjs @@ -27,7 +27,7 @@ for (const link of toc.match(//g)) { const data = fs.readFileSync(new URL(`./${href}`, source), 'utf8'); // Split the doc. - const match = /(<\/ul>\s*)?<\/\w+>\s*<\w+ id="apicontent">/.exec(data); + const match = /(<\/ul>\s*)?<\/\w+>\s*<\w+ role="main" id="apicontent">/.exec(data); // Get module name const moduleName = href.replace(/\.html$/, ''); @@ -89,7 +89,7 @@ all = all.slice(0, tocStart.index + tocStart[0].length) + all.slice(tocStart.index + tocStart[0].length); // Replace apicontent with the concatenated set of apicontents from each source. -const apiStart = /<\w+ id="apicontent">\s*/.exec(all); +const apiStart = /<\w+ role="main" id="apicontent">\s*/.exec(all); const apiEnd = all.lastIndexOf(''); all = all.slice(0, apiStart.index + apiStart[0].length) .replace( From 7f06c270c6a423e62e997650832f91a7ed96c5e4 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Tue, 26 Sep 2023 10:54:02 -0700 Subject: [PATCH 07/31] tools: add navigation ARIA landmark to generated API ToC As an accessibility improvement, specify the navigation landmark for the element of our docs that contains the table of contents generated for the specific API page. Ref: https://www.w3.org/WAI/ARIA/apg/practices/landmark-regions/ Ref: https://www.w3.org/WAI/WCAG21/Techniques/aria/ARIA20.html PR-URL: https://github.com/nodejs/node/pull/49882 Reviewed-By: Luigi Pinca Reviewed-By: Claudio Wunder Reviewed-By: LiviaMedeiros --- tools/doc/html.mjs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/doc/html.mjs b/tools/doc/html.mjs index 168f68c1b03f212..95782efe03d5544 100644 --- a/tools/doc/html.mjs +++ b/tools/doc/html.mjs @@ -467,7 +467,7 @@ export function buildToc({ filename, apilinks }) { .use(htmlStringify) .processSync(toc).toString(); - file.toc = `
Table of contents${inner}
`; + file.toc = ``; file.tocPicker = `
${inner}
`; } else { file.toc = file.tocPicker = ''; From 0fe673c7e6f879577034e31f9290f2c596db3e06 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Tue, 26 Sep 2023 11:27:31 -0700 Subject: [PATCH 08/31] meta: update website team with new name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pings for the website team aren't working because the team name changed but it did not get updated in CODEOWNERS. PR-URL: https://github.com/nodejs/node/pull/49883 Reviewed-By: Yagiz Nizipli Reviewed-By: Antoine du Hamel Reviewed-By: Claudio Wunder Reviewed-By: Moshe Atlow Reviewed-By: Darshan Sen Reviewed-By: Tobias Nießen Reviewed-By: Luigi Pinca --- .github/CODEOWNERS | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 1a62e3560a23ef3..3e7c3f62aa752f9 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -20,10 +20,10 @@ /LICENSE @nodejs/tsc /onboarding.md @nodejs/tsc -# website -/doc/api_assets @nodejs/website -/doc/template.html @nodejs/website -/tools/doc @nodejs/website +# nodejs.org website +/doc/api_assets @nodejs/nodejs-website +/doc/template.html @nodejs/nodejs-website +/tools/doc @nodejs/web-infra # streams From 5570c29780c0ac2f556269dd5f40fc3ef9b08a0f Mon Sep 17 00:00:00 2001 From: Ruy Adorno Date: Thu, 28 Sep 2023 16:21:45 -0400 Subject: [PATCH 09/31] 2023-09-28, Version 20.8.0 (Current) Notable changes: deps: * add v8::Object::SetInternalFieldForNodeCore() (Joyee Cheung) https://github.com/nodejs/node/pull/49874 doc: * deprecate `fs.F_OK`, `fs.R_OK`, `fs.W_OK`, `fs.X_OK` (Livia Medeiros) https://github.com/nodejs/node/pull/49683 * deprecate `util.toUSVString` (Yagiz Nizipli) https://github.com/nodejs/node/pull/49725 * deprecate calling `promisify` on a function that returns a promise (Antoine du Hamel) https://github.com/nodejs/node/pull/49647 esm: * set all hooks as release candidate (Geoffrey Booth) https://github.com/nodejs/node/pull/49597 module: * fix the leak in SourceTextModule and ContextifySript (Joyee Cheung) https://github.com/nodejs/node/pull/48510 * fix leak of vm.SyntheticModule (Joyee Cheung) https://github.com/nodejs/node/pull/48510 * use symbol in WeakMap to manage host defined options (Joyee Cheung) https://github.com/nodejs/node/pull/48510 src: * (SEMVER-MINOR) allow embedders to override NODE_MODULE_VERSION (Cheng Zhao) https://github.com/nodejs/node/pull/49279 stream: * use bitmap in writable state (Raz Luvaton) https://github.com/nodejs/node/pull/49834 * use bitmap in readable state (Benjamin Gruenbaum) https://github.com/nodejs/node/pull/49745 * improve webstream readable async iterator performance (Raz Luvaton) https://github.com/nodejs/node/pull/49662 test_runner: * (SEMVER-MINOR) accept `testOnly` in `run` (Moshe Atlow) https://github.com/nodejs/node/pull/49753 * (SEMVER-MINOR) add junit reporter (Moshe Atlow) https://github.com/nodejs/node/pull/49614 PR-URL: https://github.com/nodejs/node/pull/49932 --- CHANGELOG.md | 3 +- doc/api/deprecations.md | 6 +- doc/api/fs.md | 2 +- doc/api/module.md | 2 +- doc/api/util.md | 2 +- doc/changelogs/CHANGELOG_V20.md | 200 ++++++++++++++++++++++++++++++++ 6 files changed, 208 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d730e6feeff82e7..7d3999d1291b0a8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,7 +34,8 @@ release. -20.7.0
+20.8.0
+20.7.0
20.6.1
20.6.0
20.5.1
diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md index a6b9ee98bd8f65b..d9e57b4a2c17f5e 100644 --- a/doc/api/deprecations.md +++ b/doc/api/deprecations.md @@ -3390,7 +3390,7 @@ changes: - version: REPLACEME pr-url: https://github.com/nodejs/node/pull/49609 description: Runtime deprecation. - - version: REPLACEME + - version: v20.8.0 pr-url: https://github.com/nodejs/node/pull/49647 description: Documentation-only deprecation. --> @@ -3404,7 +3404,7 @@ the result of said promise, which can lead to unhandled promise rejections. @@ -3418,7 +3418,7 @@ The [`util.toUSVString()`][] API is deprecated. Please use diff --git a/doc/api/fs.md b/doc/api/fs.md index 98abd7eadd87144..66613dde55c19c2 100644 --- a/doc/api/fs.md +++ b/doc/api/fs.md @@ -1830,7 +1830,7 @@ concurrent modifications on the same file or data corruption may occur. diff --git a/doc/api/util.md b/doc/api/util.md index 50048beac89b80e..bf154500f64e3e6 100644 --- a/doc/api/util.md +++ b/doc/api/util.md @@ -1598,7 +1598,7 @@ $ node negate.js --no-logfile --logfile=test.log --color --no-color + +> Stability: 1.0 - Early development + +Define which module system, `module` or `commonjs`, to use for the following: + +* String input provided via `--eval` or STDIN, if `--input-type` is unspecified. + +* Files ending in `.js` or with no extension, if there is no `package.json` file + present in the same folder or any parent folder. + +* Files ending in `.js` or with no extension, if the nearest parent + `package.json` field lacks a `"type"` field; unless the `package.json` folder + or any parent folder is inside a `node_modules` folder. + +In other words, `--experimental-default-type=module` flips all the places where +Node.js currently defaults to CommonJS to instead default to ECMAScript modules, +with the exception of folders and subfolders below `node_modules`, for backward +compatibility. + +Under `--experimental-default-type=module` and `--experimental-wasm-modules`, +files with no extension will be treated as WebAssembly if they begin with the +WebAssembly magic number (`\0asm`); otherwise they will be treated as ES module +JavaScript. + ### `--experimental-import-meta-resolve` @@ -1059,6 +1060,7 @@ resolution for ESM specifiers is [commonjs-extension-resolution-loader][]. [URL]: https://url.spec.whatwg.org/ [`"exports"`]: packages.md#exports [`"type"`]: packages.md#type +[`--experimental-default-type`]: cli.md#--experimental-default-typetype [`--input-type`]: cli.md#--input-typetype [`data:` URLs]: https://developer.mozilla.org/en-US/docs/Web/HTTP/Basics_of_HTTP/Data_URIs [`export`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/export diff --git a/doc/api/packages.md b/doc/api/packages.md index a42e1e041a00b67..9f55cbbb15939fc 100644 --- a/doc/api/packages.md +++ b/doc/api/packages.md @@ -55,6 +55,8 @@ along with a reference for the [`package.json`][] fields defined by Node.js. ## Determining module system +### Introduction + Node.js will treat the following as [ES modules][] when passed to `node` as the initial input, or when referenced by `import` statements or `import()` expressions: @@ -67,14 +69,9 @@ expressions: * Strings passed in as an argument to `--eval`, or piped to `node` via `STDIN`, with the flag `--input-type=module`. -Node.js will treat as [CommonJS][] all other forms of input, such as `.js` files -where the nearest parent `package.json` file contains no top-level `"type"` -field, or string input without the flag `--input-type`. This behavior is to -preserve backward compatibility. However, now that Node.js supports both -CommonJS and ES modules, it is best to be explicit whenever possible. Node.js -will treat the following as CommonJS when passed to `node` as the initial input, -or when referenced by `import` statements, `import()` expressions, or -`require()` expressions: +Node.js will treat the following as [CommonJS][] when passed to `node` as the +initial input, or when referenced by `import` statements or `import()` +expressions: * Files with a `.cjs` extension. @@ -84,11 +81,30 @@ or when referenced by `import` statements, `import()` expressions, or * Strings passed in as an argument to `--eval` or `--print`, or piped to `node` via `STDIN`, with the flag `--input-type=commonjs`. -Package authors should include the [`"type"`][] field, even in packages where -all sources are CommonJS. Being explicit about the `type` of the package will -future-proof the package in case the default type of Node.js ever changes, and -it will also make things easier for build tools and loaders to determine how the -files in the package should be interpreted. +Aside from these explicit cases, there are other cases where Node.js defaults to +one module system or the other based on the value of the +[`--experimental-default-type`][] flag: + +* Files ending in `.js` or with no extension, if there is no `package.json` file + present in the same folder or any parent folder. + +* Files ending in `.js` or with no extension, if the nearest parent + `package.json` field lacks a `"type"` field; unless the folder is inside a + `node_modules` folder. (Package scopes under `node_modules` are always treated + as CommonJS when the `package.json` file lacks a `"type"` field, regardless + of `--experimental-default-type`, for backward compatibility.) + +* Strings passed in as an argument to `--eval` or piped to `node` via `STDIN`, + when `--input-type` is unspecified. + +This flag currently defaults to `"commonjs"`, but it may change in the future to +default to `"module"`. For this reason it is best to be explicit wherever +possible; in particular, package authors should always include the [`"type"`][] +field in their `package.json` files, even in packages where all sources are +CommonJS. Being explicit about the `type` of the package will future-proof the +package in case the default type of Node.js ever changes, and it will also make +things easier for build tools and loaders to determine how the files in the +package should be interpreted. ### Modules loaders @@ -1337,6 +1353,7 @@ This field defines [subpath imports][] for the current package. [`"packageManager"`]: #packagemanager [`"type"`]: #type [`--conditions` / `-C` flag]: #resolving-user-conditions +[`--experimental-default-type`]: cli.md#--experimental-default-typetype [`--no-addons` flag]: cli.md#--no-addons [`ERR_PACKAGE_PATH_NOT_EXPORTED`]: errors.md#err_package_path_not_exported [`esm`]: https://github.com/standard-things/esm#readme diff --git a/doc/node.1 b/doc/node.1 index dd10330d4f567aa..715cf0fdc4bc2b8 100644 --- a/doc/node.1 +++ b/doc/node.1 @@ -152,6 +152,11 @@ Requires Node.js to be built with .It Fl -enable-source-maps Enable Source Map V3 support for stack traces. . +.It Fl -experimental-default-type Ns = Ns Ar type +Interpret as either ES modules or CommonJS modules input via --eval or STDIN, when --input-type is unspecified; +.js or extensionless files with no sibling or parent package.json; +.js or extensionless files whose nearest parent package.json lacks a "type" field, unless under node_modules. +. .It Fl -experimental-global-webcrypto Expose the Web Crypto API on the global scope. . diff --git a/lib/internal/main/check_syntax.js b/lib/internal/main/check_syntax.js index 50e0b7d6de67c42..9a19c1809fe102a 100644 --- a/lib/internal/main/check_syntax.js +++ b/lib/internal/main/check_syntax.js @@ -60,7 +60,8 @@ function loadESMIfNeeded(cb) { async function checkSyntax(source, filename) { let isModule = true; if (filename === '[stdin]' || filename === '[eval]') { - isModule = getOptionValue('--input-type') === 'module'; + isModule = getOptionValue('--input-type') === 'module' || + (getOptionValue('--experimental-default-type') === 'module' && getOptionValue('--input-type') !== 'commonjs'); } else { const { defaultResolve } = require('internal/modules/esm/resolve'); const { defaultGetFormat } = require('internal/modules/esm/get_format'); diff --git a/lib/internal/main/eval_stdin.js b/lib/internal/main/eval_stdin.js index d947af49a6a9427..d71751e781b9b52 100644 --- a/lib/internal/main/eval_stdin.js +++ b/lib/internal/main/eval_stdin.js @@ -25,12 +25,14 @@ readStdin((code) => { const print = getOptionValue('--print'); const loadESM = getOptionValue('--import').length > 0; - if (getOptionValue('--input-type') === 'module') + if (getOptionValue('--input-type') === 'module' || + (getOptionValue('--experimental-default-type') === 'module' && getOptionValue('--input-type') !== 'commonjs')) { evalModule(code, print); - else + } else { evalScript('[stdin]', code, getOptionValue('--inspect-brk'), print, loadESM); + } }); diff --git a/lib/internal/main/eval_string.js b/lib/internal/main/eval_string.js index dc59a2ce4f77098..908532b0b1865ac 100644 --- a/lib/internal/main/eval_string.js +++ b/lib/internal/main/eval_string.js @@ -25,9 +25,10 @@ markBootstrapComplete(); const source = getOptionValue('--eval'); const print = getOptionValue('--print'); const loadESM = getOptionValue('--import').length > 0 || getOptionValue('--experimental-loader').length > 0; -if (getOptionValue('--input-type') === 'module') +if (getOptionValue('--input-type') === 'module' || + (getOptionValue('--experimental-default-type') === 'module' && getOptionValue('--input-type') !== 'commonjs')) { evalModule(source, print); -else { +} else { // For backward compatibility, we want the identifier crypto to be the // `node:crypto` module rather than WebCrypto. const isUsingCryptoIdentifier = diff --git a/lib/internal/modules/esm/formats.js b/lib/internal/modules/esm/formats.js index 4ab9aa6f032b7e3..b4e8d7a69d306b7 100644 --- a/lib/internal/modules/esm/formats.js +++ b/lib/internal/modules/esm/formats.js @@ -2,9 +2,12 @@ const { RegExpPrototypeExec, + Uint8Array, } = primordials; const { getOptionValue } = require('internal/options'); +const { closeSync, openSync, readSync } = require('fs'); + const experimentalWasmModules = getOptionValue('--experimental-wasm-modules'); const extensionFormatMap = { @@ -35,7 +38,33 @@ function mimeToFormat(mime) { return null; } +/** + * For extensionless files in a `module` package scope, or a default `module` scope enabled by the + * `--experimental-default-type` flag, we check the file contents to disambiguate between ES module JavaScript and Wasm. + * We do this by taking advantage of the fact that all Wasm files start with the header `0x00 0x61 0x73 0x6d` (`_asm`). + * @param {URL} url + */ +function getFormatOfExtensionlessFile(url) { + if (!experimentalWasmModules) { return 'module'; } + + const magic = new Uint8Array(4); + let fd; + try { + // TODO(@anonrig): Optimize the following by having a single C++ call + fd = openSync(url); + readSync(fd, magic, 0, 4); // Only read the first four bytes + if (magic[0] === 0x00 && magic[1] === 0x61 && magic[2] === 0x73 && magic[3] === 0x6d) { + return 'wasm'; + } + } finally { + if (fd !== undefined) { closeSync(fd); } + } + + return 'module'; +} + module.exports = { extensionFormatMap, + getFormatOfExtensionlessFile, mimeToFormat, }; diff --git a/lib/internal/modules/esm/get_format.js b/lib/internal/modules/esm/get_format.js index b3c8a56c06c1cc5..59ab89f6f763776 100644 --- a/lib/internal/modules/esm/get_format.js +++ b/lib/internal/modules/esm/get_format.js @@ -1,9 +1,11 @@ 'use strict'; + const { RegExpPrototypeExec, ObjectPrototypeHasOwnProperty, PromisePrototypeThen, PromiseResolve, + StringPrototypeIncludes, StringPrototypeCharCodeAt, StringPrototypeSlice, } = primordials; @@ -11,11 +13,15 @@ const { basename, relative } = require('path'); const { getOptionValue } = require('internal/options'); const { extensionFormatMap, + getFormatOfExtensionlessFile, mimeToFormat, } = require('internal/modules/esm/formats'); const experimentalNetworkImports = getOptionValue('--experimental-network-imports'); +const defaultTypeFlag = getOptionValue('--experimental-default-type'); +// The next line is where we flip the default to ES modules someday. +const defaultType = defaultTypeFlag === 'module' ? 'module' : 'commonjs'; const { getPackageType, getPackageScopeConfig } = require('internal/modules/esm/resolve'); const { fileURLToPath } = require('internal/url'); const { ERR_UNKNOWN_FILE_EXTENSION } = require('internal/errors').codes; @@ -66,6 +72,18 @@ function extname(url) { return ''; } +/** + * Determine whether the given file URL is under a `node_modules` folder. + * This function assumes that the input has already been verified to be a `file:` URL, + * and is a file rather than a folder. + * @param {URL} url + */ +function underNodeModules(url) { + if (url.protocol !== 'file:') { return false; } // We determine module types for other protocols based on MIME header + + return StringPrototypeIncludes(url.pathname, '/node_modules/'); +} + /** * @param {URL} url * @param {{parentURL: string}} context @@ -74,8 +92,37 @@ function extname(url) { */ function getFileProtocolModuleFormat(url, context, ignoreErrors) { const ext = extname(url); + if (ext === '.js') { - return getPackageType(url) === 'module' ? 'module' : 'commonjs'; + const packageType = getPackageType(url); + if (packageType !== 'none') { + return packageType; + } + // The controlling `package.json` file has no `type` field. + if (defaultType === 'module') { + // An exception to the type flag making ESM the default everywhere is that package scopes under `node_modules` + // should retain the assumption that a lack of a `type` field means CommonJS. + return underNodeModules(url) ? 'commonjs' : 'module'; + } + return 'commonjs'; + } + + if (ext === '') { + const packageType = getPackageType(url); + if (defaultType === 'commonjs') { // Legacy behavior + if (packageType === 'none' || packageType === 'commonjs') { + return 'commonjs'; + } + // If package type is `module`, fall through to the error case below + } else { // Else defaultType === 'module' + if (underNodeModules(url)) { // Exception for package scopes under `node_modules` + return 'commonjs'; + } + if (packageType === 'none' || packageType === 'module') { + return getFormatOfExtensionlessFile(url); + } // Else packageType === 'commonjs' + return 'commonjs'; + } } const format = extensionFormatMap[ext]; @@ -89,12 +136,10 @@ function getFileProtocolModuleFormat(url, context, ignoreErrors) { const config = getPackageScopeConfig(url); const fileBasename = basename(filepath); const relativePath = StringPrototypeSlice(relative(config.pjsonPath, filepath), 1); - suggestion = 'Loading extensionless files is not supported inside of ' + - '"type":"module" package.json contexts. The package.json file ' + - `${config.pjsonPath} caused this "type":"module" context. Try ` + - `changing ${filepath} to have a file extension. Note the "bin" ` + - 'field of package.json can point to a file with an extension, for example ' + - `{"type":"module","bin":{"${fileBasename}":"${relativePath}.js"}}`; + suggestion = 'Loading extensionless files is not supported inside of "type":"module" package.json contexts ' + + `without --experimental-default-type=module. The package.json file ${config.pjsonPath} caused this "type":"module" ` + + `context. Try changing ${filepath} to have a file extension. Note the "bin" field of package.json can point ` + + `to a file with an extension, for example {"type":"module","bin":{"${fileBasename}":"${relativePath}.js"}}`; } throw new ERR_UNKNOWN_FILE_EXTENSION(ext, filepath, suggestion); } diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js index 0d277915b3a01f5..5aea5ca5460199f 100644 --- a/lib/internal/modules/esm/resolve.js +++ b/lib/internal/modules/esm/resolve.js @@ -35,7 +35,7 @@ const preserveSymlinks = getOptionValue('--preserve-symlinks'); const preserveSymlinksMain = getOptionValue('--preserve-symlinks-main'); const experimentalNetworkImports = getOptionValue('--experimental-network-imports'); -const typeFlag = getOptionValue('--input-type'); +const inputTypeFlag = getOptionValue('--input-type'); const { URL, pathToFileURL, fileURLToPath, isURL } = require('internal/url'); const { getCWDURL } = require('internal/util'); const { canParse: URLCanParse } = internalBinding('url'); @@ -1112,7 +1112,7 @@ function defaultResolve(specifier, context = {}) { // input, to avoid user confusion over how expansive the effect of the // flag should be (i.e. entry point only, package scope surrounding the // entry point, etc.). - if (typeFlag) { throw new ERR_INPUT_TYPE_NOT_ALLOWED(); } + if (inputTypeFlag) { throw new ERR_INPUT_TYPE_NOT_ALLOWED(); } } conditions = getConditionsSet(conditions); diff --git a/lib/internal/modules/run_main.js b/lib/internal/modules/run_main.js index ac1ffef0412b174..2e4dabd503a883f 100644 --- a/lib/internal/modules/run_main.js +++ b/lib/internal/modules/run_main.js @@ -43,12 +43,15 @@ function shouldUseESMLoader(mainPath) { */ const userImports = getOptionValue('--import'); if (userLoaders.length > 0 || userImports.length > 0) { return true; } - const { readPackageScope } = require('internal/modules/cjs/loader'); - // Determine the module format of the main + + // Determine the module format of the entry point. if (mainPath && StringPrototypeEndsWith(mainPath, '.mjs')) { return true; } if (!mainPath || StringPrototypeEndsWith(mainPath, '.cjs')) { return false; } + + const { readPackageScope } = require('internal/modules/cjs/loader'); const pkg = readPackageScope(mainPath); - return pkg && pkg.data.type === 'module'; + // No need to guard `pkg` as it can only be an object or `false`. + return pkg.data?.type === 'module' || getOptionValue('--experimental-default-type') === 'module'; } /** diff --git a/src/node_options.cc b/src/node_options.cc index 0285f422dfd62c5..cf44003538f42fc 100644 --- a/src/node_options.cc +++ b/src/node_options.cc @@ -114,12 +114,19 @@ void EnvironmentOptions::CheckOptions(std::vector* errors, errors->push_back("--policy-integrity cannot be empty"); } - if (!module_type.empty()) { - if (module_type != "commonjs" && module_type != "module") { + if (!input_type.empty()) { + if (input_type != "commonjs" && input_type != "module") { errors->push_back("--input-type must be \"module\" or \"commonjs\""); } } + if (!type.empty()) { + if (type != "commonjs" && type != "module") { + errors->push_back("--experimental-default-type must be " + "\"module\" or \"commonjs\""); + } + } + if (syntax_check_only && has_eval_string) { errors->push_back("either --check or --eval can be used, not both"); } @@ -474,7 +481,7 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() { kAllowedInEnvvar); AddOption("--input-type", "set module type for string input", - &EnvironmentOptions::module_type, + &EnvironmentOptions::input_type, kAllowedInEnvvar); AddOption( "--experimental-specifier-resolution", "", NoOp{}, kAllowedInEnvvar); @@ -646,6 +653,10 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() { "show stack traces on process warnings", &EnvironmentOptions::trace_warnings, kAllowedInEnvvar); + AddOption("--experimental-default-type", + "set module system to use by default", + &EnvironmentOptions::type, + kAllowedInEnvvar); AddOption("--extra-info-on-fatal-exception", "hide extra information on fatal exception that causes exit", &EnvironmentOptions::extra_info_on_fatal_exception, diff --git a/src/node_options.h b/src/node_options.h index 2b9f3d3084651a4..eed6216deb67b25 100644 --- a/src/node_options.h +++ b/src/node_options.h @@ -117,7 +117,8 @@ class EnvironmentOptions : public Options { bool experimental_https_modules = false; bool experimental_wasm_modules = false; bool experimental_import_meta_resolve = false; - std::string module_type; + std::string input_type; // Value of --input-type + std::string type; // Value of --experimental-default-type std::string experimental_policy; std::string experimental_policy_integrity; bool has_policy_integrity_string = false; diff --git a/test/common/package.json b/test/common/package.json new file mode 100644 index 000000000000000..5bbefffbabee392 --- /dev/null +++ b/test/common/package.json @@ -0,0 +1,3 @@ +{ + "type": "commonjs" +} diff --git a/test/es-module/test-esm-type-flag-errors.js b/test/es-module/test-esm-type-field-errors.js similarity index 100% rename from test/es-module/test-esm-type-flag-errors.js rename to test/es-module/test-esm-type-field-errors.js diff --git a/test/es-module/test-esm-type-flag.mjs b/test/es-module/test-esm-type-field.mjs similarity index 100% rename from test/es-module/test-esm-type-flag.mjs rename to test/es-module/test-esm-type-field.mjs diff --git a/test/es-module/test-esm-type-flag-errors.mjs b/test/es-module/test-esm-type-flag-errors.mjs new file mode 100644 index 000000000000000..6d54eff94763efc --- /dev/null +++ b/test/es-module/test-esm-type-flag-errors.mjs @@ -0,0 +1,31 @@ +import { spawnPromisified } from '../common/index.mjs'; +import * as fixtures from '../common/fixtures.mjs'; +import { describe, it } from 'node:test'; +import { match, strictEqual } from 'node:assert'; + +describe('--experimental-default-type=module should not affect the interpretation of files with unknown extensions', + { concurrency: true }, () => { + it('should error on an entry point with an unknown extension', async () => { + const { code, signal, stdout, stderr } = await spawnPromisified(process.execPath, [ + '--experimental-default-type=module', + fixtures.path('es-modules/package-type-module/extension.unknown'), + ]); + + match(stderr, /ERR_UNKNOWN_FILE_EXTENSION/); + strictEqual(stdout, ''); + strictEqual(code, 1); + strictEqual(signal, null); + }); + + it('should error on an import with an unknown extension', async () => { + const { code, signal, stdout, stderr } = await spawnPromisified(process.execPath, [ + '--experimental-default-type=module', + fixtures.path('es-modules/package-type-module/imports-unknownext.mjs'), + ]); + + match(stderr, /ERR_UNKNOWN_FILE_EXTENSION/); + strictEqual(stdout, ''); + strictEqual(code, 1); + strictEqual(signal, null); + }); + }); diff --git a/test/es-module/test-esm-type-flag-loose-files.mjs b/test/es-module/test-esm-type-flag-loose-files.mjs new file mode 100644 index 000000000000000..ed95e1807f57c76 --- /dev/null +++ b/test/es-module/test-esm-type-flag-loose-files.mjs @@ -0,0 +1,75 @@ +// Flags: --experimental-default-type=module --experimental-wasm-modules +import { spawnPromisified } from '../common/index.mjs'; +import * as fixtures from '../common/fixtures.mjs'; +import { describe, it } from 'node:test'; +import { strictEqual } from 'node:assert'; + +describe('the type flag should change the interpretation of certain files outside of any package scope', + { concurrency: true }, () => { + it('should run as ESM a .js file that is outside of any package scope', async () => { + const { code, signal, stdout, stderr } = await spawnPromisified(process.execPath, [ + '--experimental-default-type=module', + fixtures.path('es-modules/loose.js'), + ]); + + strictEqual(stderr, ''); + strictEqual(stdout, 'executed\n'); + strictEqual(code, 0); + strictEqual(signal, null); + }); + + it('should run as ESM an extensionless JavaScript file that is outside of any package scope', async () => { + const { code, signal, stdout, stderr } = await spawnPromisified(process.execPath, [ + '--experimental-default-type=module', + fixtures.path('es-modules/noext-esm'), + ]); + + strictEqual(stderr, ''); + strictEqual(stdout, 'executed\n'); + strictEqual(code, 0); + strictEqual(signal, null); + }); + + it('should run as Wasm an extensionless Wasm file that is outside of any package scope', async () => { + const { code, signal, stdout, stderr } = await spawnPromisified(process.execPath, [ + '--experimental-default-type=module', + '--experimental-wasm-modules', + '--no-warnings', + fixtures.path('es-modules/noext-wasm'), + ]); + + strictEqual(stderr, ''); + strictEqual(stdout, ''); + strictEqual(code, 0); + strictEqual(signal, null); + }); + + it('should import as ESM a .js file that is outside of any package scope', async () => { + const { default: defaultExport } = await import(fixtures.fileURL('es-modules/loose.js')); + strictEqual(defaultExport, 'module'); + }); + + it('should import as ESM an extensionless JavaScript file that is outside of any package scope', + async () => { + const { default: defaultExport } = await import(fixtures.fileURL('es-modules/noext-esm')); + strictEqual(defaultExport, 'module'); + }); + + it('should import as Wasm an extensionless Wasm file that is outside of any package scope', async () => { + const { add } = await import(fixtures.fileURL('es-modules/noext-wasm')); + strictEqual(add(1, 2), 3); + }); + + it('should check as ESM input passed via --check', async () => { + const { code, signal, stdout, stderr } = await spawnPromisified(process.execPath, [ + '--experimental-default-type=module', + '--check', + fixtures.path('es-modules/loose.js'), + ]); + + strictEqual(stderr, ''); + strictEqual(stdout, ''); + strictEqual(code, 0); + strictEqual(signal, null); + }); + }); diff --git a/test/es-module/test-esm-type-flag-package-scopes.mjs b/test/es-module/test-esm-type-flag-package-scopes.mjs new file mode 100644 index 000000000000000..6b16e8a3ae223b3 --- /dev/null +++ b/test/es-module/test-esm-type-flag-package-scopes.mjs @@ -0,0 +1,150 @@ +// Flags: --experimental-default-type=module --experimental-wasm-modules +import { spawnPromisified } from '../common/index.mjs'; +import * as fixtures from '../common/fixtures.mjs'; +import { describe, it } from 'node:test'; +import { strictEqual } from 'node:assert'; + +describe('the type flag should change the interpretation of certain files within a "type": "module" package scope', + { concurrency: true }, () => { + it('should run as ESM an extensionless JavaScript file within a "type": "module" scope', async () => { + const { code, signal, stdout, stderr } = await spawnPromisified(process.execPath, [ + '--experimental-default-type=module', + fixtures.path('es-modules/package-type-module/noext-esm'), + ]); + + strictEqual(stderr, ''); + strictEqual(stdout, 'executed\n'); + strictEqual(code, 0); + strictEqual(signal, null); + }); + + it('should import an extensionless JavaScript file within a "type": "module" scope', async () => { + const { default: defaultExport } = + await import(fixtures.fileURL('es-modules/package-type-module/noext-esm')); + strictEqual(defaultExport, 'module'); + }); + + it('should run as Wasm an extensionless Wasm file within a "type": "module" scope', async () => { + const { code, signal, stdout, stderr } = await spawnPromisified(process.execPath, [ + '--experimental-default-type=module', + '--experimental-wasm-modules', + '--no-warnings', + fixtures.path('es-modules/package-type-module/noext-wasm'), + ]); + + strictEqual(stderr, ''); + strictEqual(stdout, 'executed\n'); + strictEqual(code, 0); + strictEqual(signal, null); + }); + + it('should import as Wasm an extensionless Wasm file within a "type": "module" scope', async () => { + const { add } = await import(fixtures.fileURL('es-modules/package-type-module/noext-wasm')); + strictEqual(add(1, 2), 3); + }); + }); + +describe(`the type flag should change the interpretation of certain files within a package scope that lacks a +"type" field and is not under node_modules`, { concurrency: true }, () => { + it('should run as ESM a .js file within package scope that has no defined "type" and is not under node_modules', + async () => { + const { code, signal, stdout, stderr } = await spawnPromisified(process.execPath, [ + '--experimental-default-type=module', + fixtures.path('es-modules/package-without-type/module.js'), + ]); + + strictEqual(stderr, ''); + strictEqual(stdout, 'executed\n'); + strictEqual(code, 0); + strictEqual(signal, null); + }); + + it(`should run as ESM an extensionless JavaScript file within a package scope that has no defined "type" and is not +under node_modules`, async () => { + const { code, signal, stdout, stderr } = await spawnPromisified(process.execPath, [ + '--experimental-default-type=module', + fixtures.path('es-modules/package-without-type/noext-esm'), + ]); + + strictEqual(stderr, ''); + strictEqual(stdout, 'executed\n'); + strictEqual(code, 0); + strictEqual(signal, null); + }); + + it(`should run as Wasm an extensionless Wasm file within a package scope that has no defined "type" and is not under + node_modules`, async () => { + const { code, signal, stdout, stderr } = await spawnPromisified(process.execPath, [ + '--experimental-default-type=module', + '--experimental-wasm-modules', + '--no-warnings', + fixtures.path('es-modules/noext-wasm'), + ]); + + strictEqual(stderr, ''); + strictEqual(stdout, ''); + strictEqual(code, 0); + strictEqual(signal, null); + }); + + it('should import as ESM a .js file within package scope that has no defined "type" and is not under node_modules', + async () => { + const { default: defaultExport } = await import(fixtures.fileURL('es-modules/package-without-type/module.js')); + strictEqual(defaultExport, 'module'); + }); + + it(`should import as ESM an extensionless JavaScript file within a package scope that has no defined "type" and is + not under node_modules`, async () => { + const { default: defaultExport } = await import(fixtures.fileURL('es-modules/package-without-type/noext-esm')); + strictEqual(defaultExport, 'module'); + }); + + it(`should import as Wasm an extensionless Wasm file within a package scope that has no defined "type" and is not + under node_modules`, async () => { + const { add } = await import(fixtures.fileURL('es-modules/noext-wasm')); + strictEqual(add(1, 2), 3); + }); +}); + +describe(`the type flag should NOT change the interpretation of certain files within a package scope that lacks a +"type" field and is under node_modules`, { concurrency: true }, () => { + it('should run as CommonJS a .js file within package scope that has no defined "type" and is under node_modules', + async () => { + const { code, signal, stdout, stderr } = await spawnPromisified(process.execPath, [ + '--experimental-default-type=module', + fixtures.path('es-modules/package-type-module/node_modules/dep-with-package-json/run.js'), + ]); + + strictEqual(stderr, ''); + strictEqual(stdout, 'executed\n'); + strictEqual(code, 0); + strictEqual(signal, null); + }); + + it(`should import as CommonJS a .js file within a package scope that has no defined "type" and is under + node_modules`, async () => { + const { default: defaultExport } = + await import(fixtures.fileURL('es-modules/package-type-module/node_modules/dep-with-package-json/run.js')); + strictEqual(defaultExport, 42); + }); + + it(`should run as CommonJS an extensionless JavaScript file within a package scope that has no defined "type" and is + under node_modules`, async () => { + const { code, signal, stdout, stderr } = await spawnPromisified(process.execPath, [ + '--experimental-default-type=module', + fixtures.path('es-modules/package-type-module/node_modules/dep-with-package-json/noext-cjs'), + ]); + + strictEqual(stderr, ''); + strictEqual(stdout, 'executed\n'); + strictEqual(code, 0); + strictEqual(signal, null); + }); + + it(`should import as CommonJS an extensionless JavaScript file within a package scope that has no defined "type" and + is under node_modules`, async () => { + const { default: defaultExport } = + await import(fixtures.fileURL('es-modules/package-type-module/node_modules/dep-with-package-json/noext-cjs')); + strictEqual(defaultExport, 42); + }); +}); diff --git a/test/es-module/test-esm-type-flag-string-input.mjs b/test/es-module/test-esm-type-flag-string-input.mjs new file mode 100644 index 000000000000000..c4236c00c4f28f1 --- /dev/null +++ b/test/es-module/test-esm-type-flag-string-input.mjs @@ -0,0 +1,44 @@ +import { spawnPromisified } from '../common/index.mjs'; +import { spawn } from 'node:child_process'; +import { describe, it } from 'node:test'; +import { strictEqual, match } from 'node:assert'; + +describe('the type flag should change the interpretation of string input', { concurrency: true }, () => { + it('should run as ESM input passed via --eval', async () => { + const { code, signal, stdout, stderr } = await spawnPromisified(process.execPath, [ + '--experimental-default-type=module', + '--eval', + 'import "data:text/javascript,console.log(42)"', + ]); + + strictEqual(stderr, ''); + strictEqual(stdout, '42\n'); + strictEqual(code, 0); + strictEqual(signal, null); + }); + + // ESM is unsupported for --print via --input-type=module + + it('should run as ESM input passed via STDIN', async () => { + const child = spawn(process.execPath, [ + '--experimental-default-type=module', + ]); + child.stdin.end('console.log(typeof import.meta.resolve)'); + + match((await child.stdout.toArray()).toString(), /^function\r?\n$/); + }); + + it('should be overridden by --input-type', async () => { + const { code, signal, stdout, stderr } = await spawnPromisified(process.execPath, [ + '--experimental-default-type=module', + '--input-type=commonjs', + '--eval', + 'console.log(require("process").version)', + ]); + + strictEqual(stderr, ''); + strictEqual(stdout, `${process.version}\n`); + strictEqual(code, 0); + strictEqual(signal, null); + }); +}); diff --git a/test/es-module/test-esm-unknown-or-no-extension.js b/test/es-module/test-esm-unknown-or-no-extension.js index 3f0660e5aa92253..83ebcc6267bfc3a 100644 --- a/test/es-module/test-esm-unknown-or-no-extension.js +++ b/test/es-module/test-esm-unknown-or-no-extension.js @@ -26,10 +26,10 @@ describe('ESM: extensionless and unknown specifiers', { concurrency: true }, () assert.strictEqual(code, 1); assert.strictEqual(signal, null); assert.strictEqual(stdout, ''); - assert.ok(stderr.includes('ERR_UNKNOWN_FILE_EXTENSION')); + assert.match(stderr, /ERR_UNKNOWN_FILE_EXTENSION/); if (fixturePath.includes('noext')) { // Check for explanation to users - assert.ok(stderr.includes('extensionless')); + assert.match(stderr, /extensionless/); } }); } diff --git a/test/parallel/test-esm-url-extname.js b/test/es-module/test-esm-url-extname.js similarity index 100% rename from test/parallel/test-esm-url-extname.js rename to test/es-module/test-esm-url-extname.js diff --git a/test/parallel/test-wasm-memory-out-of-bound.js b/test/es-module/test-wasm-memory-out-of-bound.js similarity index 100% rename from test/parallel/test-wasm-memory-out-of-bound.js rename to test/es-module/test-wasm-memory-out-of-bound.js diff --git a/test/parallel/test-wasm-simple.js b/test/es-module/test-wasm-simple.js similarity index 100% rename from test/parallel/test-wasm-simple.js rename to test/es-module/test-wasm-simple.js diff --git a/test/parallel/test-wasm-web-api.js b/test/es-module/test-wasm-web-api.js similarity index 100% rename from test/parallel/test-wasm-web-api.js rename to test/es-module/test-wasm-web-api.js diff --git a/test/fixtures/es-modules/imports-loose.mjs b/test/fixtures/es-modules/imports-loose.mjs new file mode 100644 index 000000000000000..13831e5db03e822 --- /dev/null +++ b/test/fixtures/es-modules/imports-loose.mjs @@ -0,0 +1 @@ +import './loose.js'; diff --git a/test/fixtures/es-modules/imports-noext.mjs b/test/fixtures/es-modules/imports-noext.mjs new file mode 100644 index 000000000000000..96eca54521b9d31 --- /dev/null +++ b/test/fixtures/es-modules/imports-noext.mjs @@ -0,0 +1 @@ +import './noext-esm'; diff --git a/test/fixtures/es-modules/loose.js b/test/fixtures/es-modules/loose.js new file mode 100644 index 000000000000000..69147a3b8ca0270 --- /dev/null +++ b/test/fixtures/es-modules/loose.js @@ -0,0 +1,3 @@ +// This file can be run or imported only if `--experimental-default-type=module` is set. +export default 'module'; +console.log('executed'); diff --git a/test/fixtures/es-modules/noext-esm b/test/fixtures/es-modules/noext-esm new file mode 100644 index 000000000000000..251d6e538a1fcf4 --- /dev/null +++ b/test/fixtures/es-modules/noext-esm @@ -0,0 +1,2 @@ +export default 'module'; +console.log('executed'); diff --git a/test/fixtures/es-modules/noext-wasm b/test/fixtures/es-modules/noext-wasm new file mode 100644 index 0000000000000000000000000000000000000000..9e035904b2e4d0a30ce5a63bcf05cc3a2c8449db GIT binary patch literal 136 zcmZ9COA5k35Jam#kl=s>MAx~130^|TEhaEo*f23T0he=in=IYDDqa=lk_iA^G=gdb zBG>AL9Q@$(Fn;}VPs=uBD{AGr0)Mu(GOe%O7ZMd>X|61DN|4~3^7j7hOM should still be CommonJS as it is in node_modules +module.exports = 42; diff --git a/test/fixtures/es-modules/package-type-module/node_modules/dep-with-package-json/noext-cjs b/test/fixtures/es-modules/package-type-module/node_modules/dep-with-package-json/noext-cjs new file mode 100644 index 000000000000000..7712b3bad54497c --- /dev/null +++ b/test/fixtures/es-modules/package-type-module/node_modules/dep-with-package-json/noext-cjs @@ -0,0 +1,3 @@ +// No package.json -> should still be CommonJS as it is in node_modules +module.exports = 42; +console.log('executed'); diff --git a/test/fixtures/es-modules/package-type-module/node_modules/dep-with-package-json/package.json b/test/fixtures/es-modules/package-type-module/node_modules/dep-with-package-json/package.json new file mode 100644 index 000000000000000..5ee78b14c414b22 --- /dev/null +++ b/test/fixtures/es-modules/package-type-module/node_modules/dep-with-package-json/package.json @@ -0,0 +1,7 @@ +{ + "name": "dep-with-package-json", + "version": "1.0.0", + "exports": { + "./*": "./*" + } +} diff --git a/test/fixtures/es-modules/package-type-module/node_modules/dep-with-package-json/run.js b/test/fixtures/es-modules/package-type-module/node_modules/dep-with-package-json/run.js new file mode 100644 index 000000000000000..7712b3bad54497c --- /dev/null +++ b/test/fixtures/es-modules/package-type-module/node_modules/dep-with-package-json/run.js @@ -0,0 +1,3 @@ +// No package.json -> should still be CommonJS as it is in node_modules +module.exports = 42; +console.log('executed'); diff --git a/test/fixtures/es-modules/package-type-module/node_modules/dep/dep.js b/test/fixtures/es-modules/package-type-module/node_modules/dep-without-package-json/dep.js similarity index 100% rename from test/fixtures/es-modules/package-type-module/node_modules/dep/dep.js rename to test/fixtures/es-modules/package-type-module/node_modules/dep-without-package-json/dep.js diff --git a/test/fixtures/es-modules/package-type-module/node_modules/dep-without-package-json/noext-cjs b/test/fixtures/es-modules/package-type-module/node_modules/dep-without-package-json/noext-cjs new file mode 100644 index 000000000000000..7712b3bad54497c --- /dev/null +++ b/test/fixtures/es-modules/package-type-module/node_modules/dep-without-package-json/noext-cjs @@ -0,0 +1,3 @@ +// No package.json -> should still be CommonJS as it is in node_modules +module.exports = 42; +console.log('executed'); diff --git a/test/fixtures/es-modules/package-type-module/node_modules/dep-without-package-json/run.js b/test/fixtures/es-modules/package-type-module/node_modules/dep-without-package-json/run.js new file mode 100644 index 000000000000000..7712b3bad54497c --- /dev/null +++ b/test/fixtures/es-modules/package-type-module/node_modules/dep-without-package-json/run.js @@ -0,0 +1,3 @@ +// No package.json -> should still be CommonJS as it is in node_modules +module.exports = 42; +console.log('executed'); diff --git a/test/fixtures/es-modules/package-type-module/noext-wasm b/test/fixtures/es-modules/package-type-module/noext-wasm new file mode 100644 index 0000000000000000000000000000000000000000..9e035904b2e4d0a30ce5a63bcf05cc3a2c8449db GIT binary patch literal 136 zcmZ9COA5k35Jam#kl=s>MAx~130^|TEhaEo*f23T0he=in=IYDDqa=lk_iA^G=gdb zBG>AL9Q@$(Fn;}VPs=uBD{AGr0)Mu(GOe%O7ZMd>X|61DN|4~3^7j7hOM Date: Fri, 29 Sep 2023 09:46:32 +0100 Subject: [PATCH 11/31] esm: fix cache collision on JSON files using file: URL PR-URL: https://github.com/nodejs/node/pull/49887 Fixes: https://github.com/nodejs/node/issues/49724 Reviewed-By: Geoffrey Booth Reviewed-By: LiviaMedeiros Reviewed-By: Jacob Smith Reviewed-By: Chemi Atlow --- lib/internal/modules/esm/translators.js | 10 ++-- test/es-module/test-esm-json.mjs | 62 +++++++++++++++++++++++- test/es-module/test-esm-virtual-json.mjs | 30 ++++++++++++ 3 files changed, 98 insertions(+), 4 deletions(-) create mode 100644 test/es-module/test-esm-virtual-json.mjs diff --git a/lib/internal/modules/esm/translators.js b/lib/internal/modules/esm/translators.js index bd67593f993e078..cf9afb741aab85a 100644 --- a/lib/internal/modules/esm/translators.js +++ b/lib/internal/modules/esm/translators.js @@ -11,6 +11,7 @@ const { SafeArrayIterator, SafeMap, SafeSet, + StringPrototypeIncludes, StringPrototypeReplaceAll, StringPrototypeSlice, StringPrototypeStartsWith, @@ -443,9 +444,12 @@ translators.set('json', function jsonStrategy(url, source) { debug(`Loading JSONModule ${url}`); const pathname = StringPrototypeStartsWith(url, 'file:') ? fileURLToPath(url) : null; + const shouldCheckAndPopulateCJSModuleCache = + // We want to involve the CJS loader cache only for `file:` URL with no search query and no hash. + pathname && !StringPrototypeIncludes(url, '?') && !StringPrototypeIncludes(url, '#'); let modulePath; let module; - if (pathname) { + if (shouldCheckAndPopulateCJSModuleCache) { modulePath = isWindows ? StringPrototypeReplaceAll(pathname, '/', '\\') : pathname; module = CJSModule._cache[modulePath]; @@ -457,7 +461,7 @@ translators.set('json', function jsonStrategy(url, source) { } } source = stringify(source); - if (pathname) { + if (shouldCheckAndPopulateCJSModuleCache) { // A require call could have been called on the same file during loading and // that resolves synchronously. To make sure we always return the identical // export, we have to check again if the module already exists or not. @@ -484,7 +488,7 @@ translators.set('json', function jsonStrategy(url, source) { err.message = errPath(url) + ': ' + err.message; throw err; } - if (pathname) { + if (shouldCheckAndPopulateCJSModuleCache) { CJSModule._cache[modulePath] = module; } cjsCache.set(url, module); diff --git a/test/es-module/test-esm-json.mjs b/test/es-module/test-esm-json.mjs index 2740c0097f77da0..82232838b791505 100644 --- a/test/es-module/test-esm-json.mjs +++ b/test/es-module/test-esm-json.mjs @@ -2,7 +2,10 @@ import { spawnPromisified } from '../common/index.mjs'; import * as fixtures from '../common/fixtures.mjs'; import assert from 'node:assert'; import { execPath } from 'node:process'; -import { describe, it } from 'node:test'; +import { describe, it, test } from 'node:test'; + +import { mkdir, rm, writeFile } from 'node:fs/promises'; +import * as tmpdir from '../common/tmpdir.js'; import secret from '../fixtures/experimental.json' assert { type: 'json' }; @@ -21,4 +24,61 @@ describe('ESM: importing JSON', () => { assert.strictEqual(code, 0); assert.strictEqual(signal, null); }); + + test('should load different modules when the URL is different', async (t) => { + const root = tmpdir.fileURL(`./test-esm-json-${Math.random()}/`); + try { + await mkdir(root, { recursive: true }); + + await t.test('json', async () => { + let i = 0; + const url = new URL('./foo.json', root); + await writeFile(url, JSON.stringify({ id: i++ })); + const absoluteURL = await import(`${url}`, { + assert: { type: 'json' }, + }); + await writeFile(url, JSON.stringify({ id: i++ })); + const queryString = await import(`${url}?a=2`, { + assert: { type: 'json' }, + }); + await writeFile(url, JSON.stringify({ id: i++ })); + const hash = await import(`${url}#a=2`, { + assert: { type: 'json' }, + }); + await writeFile(url, JSON.stringify({ id: i++ })); + const queryStringAndHash = await import(`${url}?a=2#a=2`, { + assert: { type: 'json' }, + }); + + assert.notDeepStrictEqual(absoluteURL, queryString); + assert.notDeepStrictEqual(absoluteURL, hash); + assert.notDeepStrictEqual(queryString, hash); + assert.notDeepStrictEqual(absoluteURL, queryStringAndHash); + assert.notDeepStrictEqual(queryString, queryStringAndHash); + assert.notDeepStrictEqual(hash, queryStringAndHash); + }); + + await t.test('js', async () => { + let i = 0; + const url = new URL('./foo.mjs', root); + await writeFile(url, `export default ${JSON.stringify({ id: i++ })}\n`); + const absoluteURL = await import(`${url}`); + await writeFile(url, `export default ${JSON.stringify({ id: i++ })}\n`); + const queryString = await import(`${url}?a=1`); + await writeFile(url, `export default ${JSON.stringify({ id: i++ })}\n`); + const hash = await import(`${url}#a=1`); + await writeFile(url, `export default ${JSON.stringify({ id: i++ })}\n`); + const queryStringAndHash = await import(`${url}?a=1#a=1`); + + assert.notDeepStrictEqual(absoluteURL, queryString); + assert.notDeepStrictEqual(absoluteURL, hash); + assert.notDeepStrictEqual(queryString, hash); + assert.notDeepStrictEqual(absoluteURL, queryStringAndHash); + assert.notDeepStrictEqual(queryString, queryStringAndHash); + assert.notDeepStrictEqual(hash, queryStringAndHash); + }); + } finally { + await rm(root, { force: true, recursive: true }); + } + }); }); diff --git a/test/es-module/test-esm-virtual-json.mjs b/test/es-module/test-esm-virtual-json.mjs new file mode 100644 index 000000000000000..8ff185a428ef019 --- /dev/null +++ b/test/es-module/test-esm-virtual-json.mjs @@ -0,0 +1,30 @@ +import '../common/index.mjs'; +import * as fixtures from '../common/fixtures.mjs'; +import { register } from 'node:module'; +import assert from 'node:assert'; + +async function resolve(referrer, context, next) { + const result = await next(referrer, context); + const url = new URL(result.url); + url.searchParams.set('randomSeed', Math.random()); + result.url = url.href; + return result; +} + +function load(url, context, next) { + if (context.importAssertions.type === 'json') { + return { + shortCircuit: true, + format: 'json', + source: JSON.stringify({ data: Math.random() }), + }; + } + return next(url, context); +} + +register(`data:text/javascript,export ${encodeURIComponent(resolve)};export ${encodeURIComponent(load)}`); + +assert.notDeepStrictEqual( + await import(fixtures.fileURL('empty.json'), { assert: { type: 'json' } }), + await import(fixtures.fileURL('empty.json'), { assert: { type: 'json' } }), +); From b3ec13d4497398a8a85738a64849570462660a4f Mon Sep 17 00:00:00 2001 From: Livia Medeiros Date: Fri, 29 Sep 2023 19:56:07 +0900 Subject: [PATCH 12/31] fs: adjust `position` validation in reading methods This prohibits invalid values (< -1 and non-integers) and allows `filehandle.read()` to handle position up to `2n ** 63n - 1n` PR-URL: https://github.com/nodejs/node/pull/42835 Reviewed-By: Antoine du Hamel Reviewed-By: James M Snell --- doc/api/fs.md | 42 ++++++---- lib/fs.js | 14 ++-- lib/internal/fs/promises.js | 7 +- lib/internal/fs/utils.js | 9 +- .../test-fs-read-position-validation.mjs | 3 +- ...t-fs-read-promises-position-validation.mjs | 84 +++++++++++++++++++ .../test-fs-readSync-position-validation.mjs | 5 +- 7 files changed, 133 insertions(+), 31 deletions(-) create mode 100644 test/parallel/test-fs-read-promises-position-validation.mjs diff --git a/doc/api/fs.md b/doc/api/fs.md index 66613dde55c19c2..34d10418b2acc8c 100644 --- a/doc/api/fs.md +++ b/doc/api/fs.md @@ -369,16 +369,20 @@ added: v10.0.0 * `buffer` {Buffer|TypedArray|DataView} A buffer that will be filled with the file data read. * `offset` {integer} The location in the buffer at which to start filling. * `length` {integer} The number of bytes to read. -* `position` {integer|null} The location where to begin reading data from the - file. If `null`, data will be read from the current file position, and - the position will be updated. If `position` is an integer, the current - file position will remain unchanged. +* `position` {integer|bigint|null} The location where to begin reading data + from the file. If `null` or `-1`, data will be read from the current file + position, and the position will be updated. If `position` is a non-negative + integer, the current file position will remain unchanged. * Returns: {Promise} Fulfills upon success with an object with two properties: * `bytesRead` {integer} The number of bytes read * `buffer` {Buffer|TypedArray|DataView} A reference to the passed in `buffer` @@ -395,6 +399,10 @@ number of bytes read is zero. added: - v13.11.0 - v12.17.0 +changes: + - version: REPLACEME + pr-url: https://github.com/nodejs/node/pull/42835 + description: Accepts bigint values as `position`. --> * `options` {Object} @@ -404,10 +412,11 @@ added: **Default:** `0` * `length` {integer} The number of bytes to read. **Default:** `buffer.byteLength - offset` - * `position` {integer|null} The location where to begin reading data from the - file. If `null`, data will be read from the current file position, and - the position will be updated. If `position` is an integer, the current - file position will remain unchanged. **Default:**: `null` + * `position` {integer|bigint|null} The location where to begin reading data + from the file. If `null` or `-1`, data will be read from the current file + position, and the position will be updated. If `position` is a non-negative + integer, the current file position will remain unchanged. + **Default:**: `null` * Returns: {Promise} Fulfills upon success with an object with two properties: * `bytesRead` {integer} The number of bytes read * `buffer` {Buffer|TypedArray|DataView} A reference to the passed in `buffer` @@ -424,6 +433,10 @@ number of bytes read is zero. added: - v18.2.0 - v16.17.0 +changes: + - version: REPLACEME + pr-url: https://github.com/nodejs/node/pull/42835 + description: Accepts bigint values as `position`. --> * `buffer` {Buffer|TypedArray|DataView} A buffer that will be filled with the @@ -433,10 +446,11 @@ added: **Default:** `0` * `length` {integer} The number of bytes to read. **Default:** `buffer.byteLength - offset` - * `position` {integer} The location where to begin reading data from the - file. If `null`, data will be read from the current file position, and - the position will be updated. If `position` is an integer, the current - file position will remain unchanged. **Default:**: `null` + * `position` {integer|bigint|null} The location where to begin reading data + from the file. If `null` or `-1`, data will be read from the current file + position, and the position will be updated. If `position` is a non-negative + integer, the current file position will remain unchanged. + **Default:**: `null` * Returns: {Promise} Fulfills upon success with an object with two properties: * `bytesRead` {integer} The number of bytes read * `buffer` {Buffer|TypedArray|DataView} A reference to the passed in `buffer` @@ -3514,8 +3528,8 @@ changes: * `length` {integer} The number of bytes to read. * `position` {integer|bigint|null} Specifies where to begin reading from in the file. If `position` is `null` or `-1 `, data will be read from the current - file position, and the file position will be updated. If `position` is an - integer, the file position will be unchanged. + file position, and the file position will be updated. If `position` is + a non-negative integer, the file position will be unchanged. * `callback` {Function} * `err` {Error} * `bytesRead` {integer} diff --git a/lib/fs.js b/lib/fs.js index 29f356a57cd22eb..3931216e9522f11 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -656,10 +656,11 @@ function read(fd, buffer, offsetOrOptions, length, position, callback) { validateOffsetLengthRead(offset, length, buffer.byteLength); - if (position == null) + if (position == null) { position = -1; - - validatePosition(position, 'position'); + } else { + validatePosition(position, 'position', length); + } function wrapper(err, bytesRead) { // Retain a reference to buffer so that it can't be GC'ed too soon. @@ -724,10 +725,11 @@ function readSync(fd, buffer, offsetOrOptions, length, position) { validateOffsetLengthRead(offset, length, buffer.byteLength); - if (position == null) + if (position == null) { position = -1; - - validatePosition(position, 'position'); + } else { + validatePosition(position, 'position', length); + } const ctx = {}; const result = binding.read(fd, buffer, offset, length, position, diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index bc506b9be82fbcc..a656ca411584b26 100644 --- a/lib/internal/fs/promises.js +++ b/lib/internal/fs/promises.js @@ -6,7 +6,6 @@ const { Error, MathMax, MathMin, - NumberIsSafeInteger, Promise, PromisePrototypeThen, PromiseResolve, @@ -67,6 +66,7 @@ const { validateCpOptions, validateOffsetLengthRead, validateOffsetLengthWrite, + validatePosition, validateRmOptions, validateRmdirOptions, validateStringAfterArrayBufferView, @@ -636,8 +636,11 @@ async function read(handle, bufferOrParams, offset, length, position) { validateOffsetLengthRead(offset, length, buffer.byteLength); - if (!NumberIsSafeInteger(position)) + if (position == null) { position = -1; + } else { + validatePosition(position, 'position', length); + } const bytesRead = (await binding.read(handle.fd, buffer, offset, length, position, kUsePromises)) || 0; diff --git a/lib/internal/fs/utils.js b/lib/internal/fs/utils.js index 2fc7bf61e9c4883..f84133296e86fbe 100644 --- a/lib/internal/fs/utils.js +++ b/lib/internal/fs/utils.js @@ -916,13 +916,14 @@ const validateStringAfterArrayBufferView = hideStackFrames((buffer, name) => { } }); -const validatePosition = hideStackFrames((position, name) => { +const validatePosition = hideStackFrames((position, name, length) => { if (typeof position === 'number') { - validateInteger(position, name); + validateInteger(position, name, -1); } else if (typeof position === 'bigint') { - if (!(position >= -(2n ** 63n) && position <= 2n ** 63n - 1n)) { + const maxPosition = 2n ** 63n - 1n - BigInt(length); + if (!(position >= -1n && position <= maxPosition)) { throw new ERR_OUT_OF_RANGE(name, - `>= ${-(2n ** 63n)} && <= ${2n ** 63n - 1n}`, + `>= -1 && <= ${maxPosition}`, position); } } else { diff --git a/test/parallel/test-fs-read-position-validation.mjs b/test/parallel/test-fs-read-position-validation.mjs index 5b90a3e2c0e8456..504f02c3374cd6e 100644 --- a/test/parallel/test-fs-read-position-validation.mjs +++ b/test/parallel/test-fs-read-position-validation.mjs @@ -75,8 +75,7 @@ async function testInvalid(code, position) { await testValid(2n ** 63n - 1n - BigInt(length), [ 'EFBIG', 'EOVERFLOW' ]); await testInvalid('ERR_OUT_OF_RANGE', 2n ** 63n); - - // TODO(LiviaMedeiros): test `2n ** 63n - BigInt(length)` + await testInvalid('ERR_OUT_OF_RANGE', 2n ** 63n - BigInt(length)); await testInvalid('ERR_OUT_OF_RANGE', NaN); await testInvalid('ERR_OUT_OF_RANGE', -Infinity); diff --git a/test/parallel/test-fs-read-promises-position-validation.mjs b/test/parallel/test-fs-read-promises-position-validation.mjs new file mode 100644 index 000000000000000..1531a002b01fb00 --- /dev/null +++ b/test/parallel/test-fs-read-promises-position-validation.mjs @@ -0,0 +1,84 @@ +import '../common/index.mjs'; +import * as fixtures from '../common/fixtures.mjs'; +import fs from 'fs'; +import assert from 'assert'; + +// This test ensures that "position" argument is correctly validated + +const filepath = fixtures.path('x.txt'); + +const buffer = Buffer.from('xyz\n'); +const offset = 0; +const length = buffer.byteLength; + +// allowedErrors is an array of acceptable internal errors +// For example, on some platforms read syscall might return -EFBIG +async function testValid(position, allowedErrors = []) { + let fh; + try { + fh = await fs.promises.open(filepath, 'r'); + await fh.read(buffer, offset, length, position); + await fh.read({ buffer, offset, length, position }); + await fh.read(buffer, { offset, length, position }); + } catch (err) { + if (!allowedErrors.includes(err.code)) { + assert.fail(err); + } + } finally { + await fh?.close(); + } +} + +async function testInvalid(code, position) { + let fh; + try { + fh = await fs.promises.open(filepath, 'r'); + await assert.rejects( + fh.read(buffer, offset, length, position), + { code } + ); + await assert.rejects( + fh.read({ buffer, offset, length, position }), + { code } + ); + await assert.rejects( + fh.read(buffer, { offset, length, position }), + { code } + ); + } finally { + await fh?.close(); + } +} + +{ + await testValid(undefined); + await testValid(null); + await testValid(-1); + await testValid(-1n); + + await testValid(0); + await testValid(0n); + await testValid(1); + await testValid(1n); + await testValid(9); + await testValid(9n); + await testValid(Number.MAX_SAFE_INTEGER, [ 'EFBIG' ]); + + await testValid(2n ** 63n - 1n - BigInt(length), [ 'EFBIG' ]); + await testInvalid('ERR_OUT_OF_RANGE', 2n ** 63n); + await testInvalid('ERR_OUT_OF_RANGE', 2n ** 63n - BigInt(length)); + + await testInvalid('ERR_OUT_OF_RANGE', NaN); + await testInvalid('ERR_OUT_OF_RANGE', -Infinity); + await testInvalid('ERR_OUT_OF_RANGE', Infinity); + await testInvalid('ERR_OUT_OF_RANGE', -0.999); + await testInvalid('ERR_OUT_OF_RANGE', -(2n ** 64n)); + await testInvalid('ERR_OUT_OF_RANGE', Number.MAX_SAFE_INTEGER + 1); + await testInvalid('ERR_OUT_OF_RANGE', Number.MAX_VALUE); + + for (const badTypeValue of [ + false, true, '1', Symbol(1), {}, [], () => {}, Promise.resolve(1), + ]) { + testInvalid('ERR_INVALID_ARG_TYPE', badTypeValue); + } +} diff --git a/test/parallel/test-fs-readSync-position-validation.mjs b/test/parallel/test-fs-readSync-position-validation.mjs index 93fe4be1f0b65d3..305e37778d9aac7 100644 --- a/test/parallel/test-fs-readSync-position-validation.mjs +++ b/test/parallel/test-fs-readSync-position-validation.mjs @@ -28,7 +28,7 @@ function testValid(position, allowedErrors = []) { } } -function testInvalid(code, position, internalCatch = false) { +function testInvalid(code, position) { let fdSync; try { fdSync = fs.openSync(filepath, 'r'); @@ -61,8 +61,7 @@ function testInvalid(code, position, internalCatch = false) { testValid(2n ** 63n - 1n - BigInt(length), [ 'EFBIG', 'EOVERFLOW' ]); testInvalid('ERR_OUT_OF_RANGE', 2n ** 63n); - - // TODO(LiviaMedeiros): test `2n ** 63n - BigInt(length)` + testInvalid('ERR_OUT_OF_RANGE', 2n ** 63n - BigInt(length)); testInvalid('ERR_OUT_OF_RANGE', NaN); testInvalid('ERR_OUT_OF_RANGE', -Infinity); From 7ace5aba758fb500ac93675ceab7afab21dd6d86 Mon Sep 17 00:00:00 2001 From: Deokjin Kim Date: Fri, 29 Sep 2023 19:56:20 +0900 Subject: [PATCH 13/31] events: validate options of `on` and `once` Check whether options is object or not to avoid passing invalid type as options to `on` and `once`. Refs: https://nodejs.org/dist/latest-v19.x/docs/api/events.html#eventsonceemitter-name-options PR-URL: https://github.com/nodejs/node/pull/46018 Reviewed-By: Antoine du Hamel Reviewed-By: Yagiz Nizipli Reviewed-By: Benjamin Gruenbaum --- lib/events.js | 3 +++ test/parallel/test-events-on-async-iterator.js | 9 +++++++++ test/parallel/test-events-once.js | 18 ++++++++---------- 3 files changed, 20 insertions(+), 10 deletions(-) diff --git a/lib/events.js b/lib/events.js index f42a11ab3e701df..a1837cc1a9107e4 100644 --- a/lib/events.js +++ b/lib/events.js @@ -81,6 +81,7 @@ const { validateBoolean, validateFunction, validateNumber, + validateObject, validateString, } = require('internal/validators'); @@ -960,6 +961,7 @@ function getMaxListeners(emitterOrTarget) { * @returns {Promise} */ async function once(emitter, name, options = kEmptyObject) { + validateObject(options, 'options'); const signal = options?.signal; validateAbortSignal(signal, 'options.signal'); if (signal?.aborted) @@ -1047,6 +1049,7 @@ function eventTargetAgnosticAddListener(emitter, name, listener, flags) { */ function on(emitter, event, options = kEmptyObject) { // Parameters validation + validateObject(options, 'options'); const signal = options.signal; validateAbortSignal(signal, 'options.signal'); if (signal?.aborted) diff --git a/test/parallel/test-events-on-async-iterator.js b/test/parallel/test-events-on-async-iterator.js index 94f66a81edb0c0c..057af8537f3275d 100644 --- a/test/parallel/test-events-on-async-iterator.js +++ b/test/parallel/test-events-on-async-iterator.js @@ -40,6 +40,15 @@ async function invalidArgType() { code: 'ERR_INVALID_ARG_TYPE', name: 'TypeError', })); + + const ee = new EventEmitter(); + + [1, 'hi', null, false, () => {}, Symbol(), 1n].map((options) => { + return assert.throws(() => on(ee, 'foo', options), common.expectsError({ + code: 'ERR_INVALID_ARG_TYPE', + name: 'TypeError', + })); + }); } async function error() { diff --git a/test/parallel/test-events-once.js b/test/parallel/test-events-once.js index be05028faaf0c27..1a82824d4ad922d 100644 --- a/test/parallel/test-events-once.js +++ b/test/parallel/test-events-once.js @@ -4,10 +4,10 @@ const common = require('../common'); const { once, EventEmitter } = require('events'); const { - strictEqual, deepStrictEqual, fail, rejects, + strictEqual, } = require('assert'); const { kEvents } = require('internal/event_target'); @@ -24,18 +24,16 @@ async function onceAnEvent() { strictEqual(ee.listenerCount('myevent'), 0); } -async function onceAnEventWithNullOptions() { +async function onceAnEventWithInvalidOptions() { const ee = new EventEmitter(); - process.nextTick(() => { - ee.emit('myevent', 42); - }); - - const [value] = await once(ee, 'myevent', null); - strictEqual(value, 42); + await Promise.all([1, 'hi', null, false, () => {}, Symbol(), 1n].map((options) => { + return rejects(once(ee, 'myevent', options), { + code: 'ERR_INVALID_ARG_TYPE', + }); + })); } - async function onceAnEventWithTwoArgs() { const ee = new EventEmitter(); @@ -267,7 +265,7 @@ async function eventTargetAbortSignalAfterEvent() { Promise.all([ onceAnEvent(), - onceAnEventWithNullOptions(), + onceAnEventWithInvalidOptions(), onceAnEventWithTwoArgs(), catchesErrors(), catchesErrorsWithAbortSignal(), From 17b9925393a925fd5b3e52db74e8d512878a041a Mon Sep 17 00:00:00 2001 From: Tim Perry <1526883+pimterry@users.noreply.github.com> Date: Fri, 29 Sep 2023 13:56:33 +0300 Subject: [PATCH 14/31] crypto: return clear errors when loading invalid PFX data PR-URL: https://github.com/nodejs/node/pull/49566 Reviewed-By: Ben Noordhuis --- src/crypto/crypto_context.cc | 76 ++++++++++++++++-------- test/fixtures/keys/cert-without-key.pfx | Bin 0 -> 1483 bytes test/parallel/test-tls-invalid-pfx.js | 23 +++++++ 3 files changed, 74 insertions(+), 25 deletions(-) create mode 100644 test/fixtures/keys/cert-without-key.pfx create mode 100644 test/parallel/test-tls-invalid-pfx.js diff --git a/src/crypto/crypto_context.cc b/src/crypto/crypto_context.cc index 3876adf7d72211d..6e5bbe07d0c337b 100644 --- a/src/crypto/crypto_context.cc +++ b/src/crypto/crypto_context.cc @@ -1052,34 +1052,60 @@ void SecureContext::LoadPKCS12(const FunctionCallbackInfo& args) { EVP_PKEY* pkey_ptr = nullptr; X509* cert_ptr = nullptr; STACK_OF(X509)* extra_certs_ptr = nullptr; - if (d2i_PKCS12_bio(in.get(), &p12_ptr) && - (p12.reset(p12_ptr), true) && // Move ownership to the smart pointer. - PKCS12_parse(p12.get(), pass.data(), - &pkey_ptr, - &cert_ptr, - &extra_certs_ptr) && - (pkey.reset(pkey_ptr), cert.reset(cert_ptr), - extra_certs.reset(extra_certs_ptr), true) && // Move ownership. - SSL_CTX_use_certificate_chain(sc->ctx_.get(), - std::move(cert), - extra_certs.get(), - &sc->cert_, - &sc->issuer_) && - SSL_CTX_use_PrivateKey(sc->ctx_.get(), pkey.get())) { - // Add CA certs too - for (int i = 0; i < sk_X509_num(extra_certs.get()); i++) { - X509* ca = sk_X509_value(extra_certs.get(), i); - - if (cert_store == GetOrCreateRootCertStore()) { - cert_store = NewRootCertStore(); - SSL_CTX_set_cert_store(sc->ctx_.get(), cert_store); - } - X509_STORE_add_cert(cert_store, ca); - SSL_CTX_add_client_CA(sc->ctx_.get(), ca); + + if (!d2i_PKCS12_bio(in.get(), &p12_ptr)) { + goto done; + } + + // Move ownership to the smart pointer: + p12.reset(p12_ptr); + + if (!PKCS12_parse( + p12.get(), pass.data(), &pkey_ptr, &cert_ptr, &extra_certs_ptr)) { + goto done; + } + + // Move ownership of the parsed data: + pkey.reset(pkey_ptr); + cert.reset(cert_ptr); + extra_certs.reset(extra_certs_ptr); + + if (!pkey) { + return THROW_ERR_CRYPTO_OPERATION_FAILED( + env, "Unable to load private key from PFX data"); + } + + if (!cert) { + return THROW_ERR_CRYPTO_OPERATION_FAILED( + env, "Unable to load certificate from PFX data"); + } + + if (!SSL_CTX_use_certificate_chain(sc->ctx_.get(), + std::move(cert), + extra_certs.get(), + &sc->cert_, + &sc->issuer_)) { + goto done; + } + + if (!SSL_CTX_use_PrivateKey(sc->ctx_.get(), pkey.get())) { + goto done; + } + + // Add CA certs too + for (int i = 0; i < sk_X509_num(extra_certs.get()); i++) { + X509* ca = sk_X509_value(extra_certs.get(), i); + + if (cert_store == GetOrCreateRootCertStore()) { + cert_store = NewRootCertStore(); + SSL_CTX_set_cert_store(sc->ctx_.get(), cert_store); } - ret = true; + X509_STORE_add_cert(cert_store, ca); + SSL_CTX_add_client_CA(sc->ctx_.get(), ca); } + ret = true; +done: if (!ret) { // TODO(@jasnell): Should this use ThrowCryptoError? unsigned long err = ERR_get_error(); // NOLINT(runtime/int) diff --git a/test/fixtures/keys/cert-without-key.pfx b/test/fixtures/keys/cert-without-key.pfx new file mode 100644 index 0000000000000000000000000000000000000000..6d3dfca11fe98446931033e13becbac383c4ffe0 GIT binary patch literal 1483 zcmV;+1vL6Ff(6F{0Ru3C1$_nyDuzgg_YDCD0ic2fZUlk_YA}KYW-x*UVg?B+hDe6@ z4FLxRpn?TcFoFe70s#Opf(1wh2`Yw2hW8Bt2LUiw1_>&LNQU{y72y2mmk)1_&yKNQU#O#UHS2`M*uHl*Y}gTW4Q|oITZZb;6sl8 z(4$~E$;+L?JrMzk8(BxR;3u%tVo07nMt4gkG}qqa&eGuX8Jmo0gyfwfC#ZZv7#Od> zf7Ms*50>9X^O`|_z%csxE%??5SQ`}<3H|n^MevmqNw||p0`xmubT1pdlzs~~0zM)p z1@-J>32N$x3_xU<)7aAWlH0acz*gg(3PyHh=0ZQj=dD$qXfsfPxV^wS=`Pif&Pi?E zvXX+YyS%WMbm?6JA4Bn$Y(!(vOQNi{tj+`B*4X_K$6&6UVxv6N?k>{B#WU#1YC-bm zxK=>q`{1xwwx^F`t0DN>0z-T+A-W9ho+f4=^kUF$McklhPYxMM7+q~lrO z$l@`7JSoQS)bY5nyqGtLipeawJbqhxYX1Ro90l&dUG*OCZ8I5dxg003PcVVJv?pNG zynCut%73q?{bv5~Ojsm};it&!X=wOgB1RL?v&%*yf!V}7<6tQ}-f`6fZ}nj?&ZmR0MPr4{Y`66nsSo6FrFtZHsE!}%3^IzQ|FLg5t_dSYR(W` zCQBF-bw|o=c41Sw{xV@QRZtl|Fy_elo%iw@O$d(Zxiw)dQ5OP+)V?^)B{78g<>`m; z?`0GJcLvrYR{!a#H7;7wn3qcNJSjUPvezmA3_jH$Kpg7WT?r3pXjPQ(~l|vF1!kwnm=ZE_FKb}?L%KJ5`%fSKnRIjZnrfBbTuY|g$l~k z)`xjW;3Xl`Gm5HcICGd3Bd$ggIk$Pu2yQ8Ap;p zi#xF3uFLq=^EZfVMTuX~Zg>2^14sU<7{ZP@a!kD+x=J0tz<^OaOcor%%YI>!O~A-V zUrp-(Fm?m4RFZRa!gxNTwB;8tK`=2e4F(BdhDZTr0|WvA1povfbA{bAVR(y9g{P(W l8fsA}>cmtx`2HsZuA { + assert.strictEqual(e.message, 'Unable to load private key from PFX data'); + cleanup(); +})); From 4f84a3d2004ec07d7d4683ed265beb925da8a830 Mon Sep 17 00:00:00 2001 From: Aras Abbasi Date: Fri, 29 Sep 2023 13:04:38 +0200 Subject: [PATCH 15/31] errors: improve formatList in errors.js PR-URL: https://github.com/nodejs/node/pull/49642 Reviewed-By: Yagiz Nizipli Reviewed-By: LiviaMedeiros Reviewed-By: Antoine du Hamel --- benchmark/error/format-list.js | 44 +++++++++++++++++++++++++ lib/internal/errors.js | 10 ++++-- test/parallel/test-error-format-list.js | 2 +- 3 files changed, 53 insertions(+), 3 deletions(-) create mode 100644 benchmark/error/format-list.js diff --git a/benchmark/error/format-list.js b/benchmark/error/format-list.js new file mode 100644 index 000000000000000..f35ca3593a743cb --- /dev/null +++ b/benchmark/error/format-list.js @@ -0,0 +1,44 @@ +'use strict'; + +const common = require('../common.js'); + +const bench = common.createBenchmark(main, { + n: [1e7], + input: [ + '', + 'a', + 'a,b', + 'a,b,c', + 'a,b,c,d', + ], + type: [ + 'undefined', + 'and', + 'or', + ], +}, { + flags: ['--expose-internals'], +}); + +function main({ n, input, type }) { + const { + formatList, + } = require('internal/errors'); + + const list = input.split(','); + + if (type === 'undefined') { + bench.start(); + for (let i = 0; i < n; ++i) { + formatList(list); + } + bench.end(n); + return; + } + + bench.start(); + for (let i = 0; i < n; ++i) { + formatList(list, type); + } + bench.end(n); +} diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 6bba8ec095e86e7..4928ac97d51537a 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -967,8 +967,14 @@ function determineSpecificType(value) { * @returns {string} */ function formatList(array, type = 'and') { - return array.length < 3 ? ArrayPrototypeJoin(array, ` ${type} `) : - `${ArrayPrototypeJoin(ArrayPrototypeSlice(array, 0, -1), ', ')}, ${type} ${array[array.length - 1]}`; + switch (array.length) { + case 0: return ''; + case 1: return `${array[0]}`; + case 2: return `${array[0]} ${type} ${array[1]}`; + case 3: return `${array[0]}, ${array[1]}, ${type} ${array[2]}`; + default: + return `${ArrayPrototypeJoin(ArrayPrototypeSlice(array, 0, -1), ', ')}, ${type} ${array[array.length - 1]}`; + } } module.exports = { diff --git a/test/parallel/test-error-format-list.js b/test/parallel/test-error-format-list.js index 54ae4e0aee714dc..2fd95584d22318a 100644 --- a/test/parallel/test-error-format-list.js +++ b/test/parallel/test-error-format-list.js @@ -11,7 +11,7 @@ if (!common.hasIntl) common.skip('missing Intl'); const and = new Intl.ListFormat('en', { style: 'long', type: 'conjunction' }); const or = new Intl.ListFormat('en', { style: 'long', type: 'disjunction' }); - const input = ['apple', 'banana', 'orange']; + const input = ['apple', 'banana', 'orange', 'pear']; for (let i = 0; i < input.length; i++) { const slicedInput = input.slice(0, i); strictEqual(formatList(slicedInput), and.format(slicedInput)); From 43500fa64652644914f3b4db6301c8c072e9ccd0 Mon Sep 17 00:00:00 2001 From: Jungku Lee Date: Fri, 29 Sep 2023 20:04:48 +0900 Subject: [PATCH 16/31] src: move const variable in `node_file.h` to `node_file.cc` PR-URL: https://github.com/nodejs/node/pull/49688 Refs: https://github.com/nodejs/node/pull/48325 Reviewed-By: Yagiz Nizipli --- src/node_file.cc | 23 ++++++++++++++++------- src/node_file.h | 8 -------- 2 files changed, 16 insertions(+), 15 deletions(-) diff --git a/src/node_file.cc b/src/node_file.cc index aca7ec82101a60e..0e03f4b1f4bd2de 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -3120,11 +3120,19 @@ BindingData::FilePathIsFileReturnType BindingData::FilePathIsFile( return BindingData::FilePathIsFileReturnType::kIsNotFile; } +namespace { + +// define the final index of the algorithm resolution +// when packageConfig.main is defined. +constexpr uint8_t legacy_main_extensions_with_main_end = 7; +// define the final index of the algorithm resolution +// when packageConfig.main is NOT defined +constexpr uint8_t legacy_main_extensions_package_fallback_end = 10; // the possible file extensions that should be tested // 0-6: when packageConfig.main is defined // 7-9: when packageConfig.main is NOT defined, // or when the previous case didn't found the file -const std::array BindingData::legacy_main_extensions = { +constexpr std::array legacy_main_extensions = { "", ".js", ".json", @@ -3136,6 +3144,8 @@ const std::array BindingData::legacy_main_extensions = { ".json", ".node"}; +} // namespace + void BindingData::LegacyMainResolve(const FunctionCallbackInfo& args) { CHECK_GE(args.Length(), 1); CHECK(args[0]->IsString()); @@ -3176,9 +3186,8 @@ void BindingData::LegacyMainResolve(const FunctionCallbackInfo& args) { FromNamespacedPath(&initial_file_path); - for (int i = 0; i < BindingData::legacy_main_extensions_with_main_end; - i++) { - file_path = initial_file_path + BindingData::legacy_main_extensions[i]; + for (int i = 0; i < legacy_main_extensions_with_main_end; i++) { + file_path = initial_file_path + std::string(legacy_main_extensions[i]); switch (FilePathIsFile(env, file_path)) { case BindingData::FilePathIsFileReturnType::kIsFile: @@ -3211,10 +3220,10 @@ void BindingData::LegacyMainResolve(const FunctionCallbackInfo& args) { FromNamespacedPath(&initial_file_path); - for (int i = BindingData::legacy_main_extensions_with_main_end; - i < BindingData::legacy_main_extensions_package_fallback_end; + for (int i = legacy_main_extensions_with_main_end; + i < legacy_main_extensions_package_fallback_end; i++) { - file_path = initial_file_path + BindingData::legacy_main_extensions[i]; + file_path = initial_file_path + std::string(legacy_main_extensions[i]); switch (FilePathIsFile(env, file_path)) { case BindingData::FilePathIsFileReturnType::kIsFile: diff --git a/src/node_file.h b/src/node_file.h index 0fa2da0b4f7f8ac..f608da5b7a17e21 100644 --- a/src/node_file.h +++ b/src/node_file.h @@ -102,14 +102,6 @@ class BindingData : public SnapshotableObject { static FilePathIsFileReturnType FilePathIsFile(Environment* env, const std::string& file_path); - - static const std::array legacy_main_extensions; - // define the final index of the algorithm resolution - // when packageConfig.main is defined. - static const uint8_t legacy_main_extensions_with_main_end = 7; - // define the final index of the algorithm resolution - // when packageConfig.main is NOT defined - static const uint8_t legacy_main_extensions_package_fallback_end = 10; }; // structure used to store state during a complex operation, e.g., mkdirp. From c935d4c8fa4fb2f59445285fbf7a2e09776ff6de Mon Sep 17 00:00:00 2001 From: Colin Ihrig Date: Fri, 29 Sep 2023 07:04:58 -0400 Subject: [PATCH 17/31] test_runner: replace spurious if with else There is an `if` statement that likely should have been an `else` in the original PR. Refs: https://github.com/nodejs/node/pull/48915 PR-URL: https://github.com/nodejs/node/pull/49943 Reviewed-By: Chemi Atlow Reviewed-By: Moshe Atlow Reviewed-By: Raz Luvaton Reviewed-By: Benjamin Gruenbaum Reviewed-By: LiviaMedeiros --- lib/internal/test_runner/test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js index 4afb93f4a60df08..4eb02c5d4e31183 100644 --- a/lib/internal/test_runner/test.js +++ b/lib/internal/test_runner/test.js @@ -86,7 +86,7 @@ function stopTest(timeout, signal) { if (timeout === kDefaultTimeout) { disposeFunction = abortListener[SymbolDispose]; - } if (timeout !== kDefaultTimeout) { + } else { timer = setTimeout(() => deferred.resolve(), timeout); timer.unref(); From 6aa7101960b7fd6af32d2cf9bd7b7dbd8b91f30c Mon Sep 17 00:00:00 2001 From: Jungku Lee Date: Fri, 29 Sep 2023 20:14:24 +0900 Subject: [PATCH 18/31] lib: update params in jsdoc for `HTTPRequestOptions` PR-URL: https://github.com/nodejs/node/pull/49872 Reviewed-By: Marco Ippolito Reviewed-By: Matteo Collina --- lib/http.js | 2 ++ lib/https.js | 2 ++ 2 files changed, 4 insertions(+) diff --git a/lib/http.js b/lib/http.js index d86d36d12c2b140..9fce02d6e3b3ac0 100644 --- a/lib/http.js +++ b/lib/http.js @@ -76,6 +76,7 @@ function createServer(opts, requestListener) { * @property {string} [host] A domain name or IP address of the server to issue the request to. * @property {string} [hostname] Alias for host. * @property {boolean} [insecureHTTPParser] Use an insecure HTTP parser that accepts invalid HTTP headers when true. + * @property {boolean} [joinDuplicateHeaders] Multiple header that joined with `,` field line values. * @property {string} [localAddress] Local interface to bind for network connections. * @property {number} [localPort] Local port to connect from. * @property {Function} [lookup] Custom lookup function. Default: dns.lookup(). @@ -88,6 +89,7 @@ function createServer(opts, requestListener) { * @property {AbortSignal} [signal] An AbortSignal that may be used to abort an ongoing request. * @property {string} [socketPath] Unix domain socket. * @property {number} [timeout] A number specifying the socket timeout in milliseconds. + * @property {Array} [uniqueHeaders] A list of request headers that should be sent only once. */ /** diff --git a/lib/https.js b/lib/https.js index 70ffa73ff1996b5..ca8c49d5bef3199 100644 --- a/lib/https.js +++ b/lib/https.js @@ -392,6 +392,7 @@ function request(...args) { * host?: string; * hostname?: string; * insecureHTTPParser?: boolean; + * joinDuplicateHeaders?: boolean; * localAddress?: string; * localPort?: number; * lookup?: Function; @@ -404,6 +405,7 @@ function request(...args) { * socketPath?: string; * timeout?: number; * signal?: AbortSignal; + * uniqueHeaders?: Array; * } | string | URL} [options] * @param {Function} [cb] * @returns {ClientRequest} From 1dc0667aa6096f10c5f95471dfe27e78db1dafd5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= Date: Fri, 29 Sep 2023 13:31:51 +0200 Subject: [PATCH 19/31] doc: document dangerous symlink behavior Much earlier, a design decision was made that the permission model should not prevent following symbolic links to presumably inaccessible locations. Recently, after some back and forth, it had been decided that it is indeed a vulnerability that symbolic links, which currently point to an accessible location, can potentially be re-targeted to point to a presumably inaccessible location. Nevertheless, months later, no solution has been found and the issue is deemed unfixable in the context of the current permission model implementation, so it was decided to disclose the vulnerability and to shift responsibiliy onto users who are now responsible for ensuring that no potentially dangerous symlinks exist in any directories that they grant access to. I believe that this design issue might be surprising and that it comes with significant security implications for users, so it should be documented. Original vulnerability report: https://hackerone.com/reports/1961655 PR-URL: https://github.com/nodejs/node/pull/49154 Reviewed-By: Benjamin Gruenbaum Reviewed-By: Antoine du Hamel --- doc/api/permissions.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/doc/api/permissions.md b/doc/api/permissions.md index 33e24f49dd5a7e7..e17bbb38e55cbcc 100644 --- a/doc/api/permissions.md +++ b/doc/api/permissions.md @@ -568,6 +568,11 @@ There are constraints you need to know before using this system: * Relative paths are not supported through the CLI (`--allow-fs-*`). * The model does not inherit to a child node process. * The model does not inherit to a worker thread. +* Symbolic links will be followed even to locations outside of the set of paths + that access has been granted to. Relative symbolic links may allow access to + arbitrary files and directories. When starting applications with the + permission model enabled, you must ensure that no paths to which access has + been granted contain relative symbolic links. * When creating symlinks the target (first argument) should have read and write access. * Permission changes are not retroactively applied to existing resources. From 1b96975f273ca92e6ded4d7f4280158f441c8da6 Mon Sep 17 00:00:00 2001 From: Sam Verschueren Date: Fri, 29 Sep 2023 15:58:42 +0200 Subject: [PATCH 20/31] lib: fix `primordials` typings PR-URL: https://github.com/nodejs/node/pull/49895 Reviewed-By: Antoine du Hamel Reviewed-By: Moshe Atlow Reviewed-By: Yagiz Nizipli Reviewed-By: Chemi Atlow --- typings/primordials.d.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/typings/primordials.d.ts b/typings/primordials.d.ts index c0a4dd2348bf03b..006ecace1ead1c7 100644 --- a/typings/primordials.d.ts +++ b/typings/primordials.d.ts @@ -427,8 +427,8 @@ declare namespace primordials { export const SymbolFor: typeof Symbol.for export const SymbolKeyFor: typeof Symbol.keyFor export const SymbolAsyncIterator: typeof Symbol.asyncIterator - export const SymbolDispose: typeof Symbol // TODO(MoLow): use typeof Symbol.dispose when it's available - export const SymbolAsyncDispose: typeof Symbol // TODO(MoLow): use typeof Symbol.asyncDispose when it's available + export const SymbolDispose: symbol // TODO(MoLow): use typeof Symbol.dispose when it's available + export const SymbolAsyncDispose: symbol // TODO(MoLow): use typeof Symbol.asyncDispose when it's available export const SymbolHasInstance: typeof Symbol.hasInstance export const SymbolIsConcatSpreadable: typeof Symbol.isConcatSpreadable export const SymbolIterator: typeof Symbol.iterator From 16ac5e1ca837f5f3f1bd2a365d0aed3d71b2803c Mon Sep 17 00:00:00 2001 From: MatteoBax <98881971+MatteoBax@users.noreply.github.com> Date: Fri, 29 Sep 2023 14:34:21 +0200 Subject: [PATCH 21/31] zlib: fix discovery of cpu-features.h for android Fixed cpu-features.h not found issue. Co-Authored-By: Luigi Pinca Fixes: https://github.com/nodejs/node/issues/49766 PR-URL: https://github.com/nodejs/node/pull/49828 Reviewed-By: Ben Noordhuis Reviewed-By: Luigi Pinca Reviewed-By: Richard Lau --- android_configure.py | 1 + common.gypi | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/android_configure.py b/android_configure.py index 57d940239150df2..a82bb56bc5f5b6a 100644 --- a/android_configure.py +++ b/android_configure.py @@ -70,6 +70,7 @@ def patch_android(): GYP_DEFINES += " v8_target_arch=" + arch GYP_DEFINES += " android_target_arch=" + arch GYP_DEFINES += " host_os=" + host_os + " OS=android" +GYP_DEFINES += " android_ndk_path=" + android_ndk_path os.environ['GYP_DEFINES'] = GYP_DEFINES if os.path.exists("./configure"): diff --git a/common.gypi b/common.gypi index 909d09934a1b258..bbf9522a2f8e003 100644 --- a/common.gypi +++ b/common.gypi @@ -230,7 +230,7 @@ ], },], ['OS == "android"', { - 'cflags': [ '-fPIC' ], + 'cflags': [ '-fPIC', '-I<(android_ndk_path)/sources/android/cpufeatures' ], 'ldflags': [ '-fPIC' ] }], ], From 4d0aeed4a64b92768c6aad9f3fb3bf054da6f996 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Wed, 27 Sep 2023 14:00:21 +0200 Subject: [PATCH 22/31] test: deflake test-perf-hooks.js Previously when checking the initial timing we did a lot of checks after accessing and copying timing.duration and before we check that timing.duration is roughly the same as performance.now(), which can lead to flakes if the overhead of the checking is big enough. Update the test to check timing.duration against performance.now() as soon as possible when it's copied instead of computed. :# PR-URL: https://github.com/nodejs/node/pull/49892 Refs: https://github.com/nodejs/reliability/issues/676 Reviewed-By: Chemi Atlow Reviewed-By: Richard Lau --- test/sequential/test-perf-hooks.js | 62 +++++++++++++++--------------- 1 file changed, 32 insertions(+), 30 deletions(-) diff --git a/test/sequential/test-perf-hooks.js b/test/sequential/test-perf-hooks.js index 5ed9ff22ce2d388..1e11f26571480dc 100644 --- a/test/sequential/test-perf-hooks.js +++ b/test/sequential/test-perf-hooks.js @@ -41,8 +41,40 @@ const epsilon = 50; assert.strictEqual(performance.nodeTiming.name, 'node'); assert.strictEqual(performance.nodeTiming.entryType, 'node'); +// If timing.duration gets copied into the argument instead of being computed +// via the getter, this should be called right after timing is created. +function checkNodeTiming(timing) { + // Calculate the difference between now() and duration as soon as possible. + const now = performance.now(); + const delta = Math.abs(now - timing.duration); + + log(JSON.stringify(timing, null, 2)); + // Check that the properties are still reasonable. + assert.strictEqual(timing.name, 'node'); + assert.strictEqual(timing.entryType, 'node'); + + // Check that duration is positive and practically the same as + // performance.now() i.e. measures Node.js instance up time. + assert.strictEqual(typeof timing.duration, 'number'); + assert(timing.duration > 0, `timing.duration ${timing.duration} <= 0`); + assert(delta < 10, + `now (${now}) - timing.duration (${timing.duration}) = ${delta} >= ${10}`); + + // Check that the following fields do not change. + assert.strictEqual(timing.startTime, initialTiming.startTime); + assert.strictEqual(timing.nodeStart, initialTiming.nodeStart); + assert.strictEqual(timing.v8Start, initialTiming.v8Start); + assert.strictEqual(timing.environment, initialTiming.environment); + assert.strictEqual(timing.bootstrapComplete, initialTiming.bootstrapComplete); + + assert.strictEqual(typeof timing.loopStart, 'number'); + assert.strictEqual(typeof timing.loopExit, 'number'); +} + +log('check initial nodeTiming'); // Copy all the values from the getters. const initialTiming = { ...performance.nodeTiming }; +checkNodeTiming(initialTiming); { const { @@ -87,36 +119,6 @@ const initialTiming = { ...performance.nodeTiming }; `bootstrapComplete ${bootstrapComplete} >= ${testStartTime}`); } -function checkNodeTiming(timing) { - // Calculate the difference between now() and duration as soon as possible. - const now = performance.now(); - const delta = Math.abs(now - timing.duration); - - log(JSON.stringify(timing, null, 2)); - // Check that the properties are still reasonable. - assert.strictEqual(timing.name, 'node'); - assert.strictEqual(timing.entryType, 'node'); - - // Check that duration is positive and practically the same as - // performance.now() i.e. measures Node.js instance up time. - assert.strictEqual(typeof timing.duration, 'number'); - assert(timing.duration > 0, `timing.duration ${timing.duration} <= 0`); - assert(delta < 10, - `now (${now}) - timing.duration (${timing.duration}) = ${delta} >= 10`); - - // Check that the following fields do not change. - assert.strictEqual(timing.startTime, initialTiming.startTime); - assert.strictEqual(timing.nodeStart, initialTiming.nodeStart); - assert.strictEqual(timing.v8Start, initialTiming.v8Start); - assert.strictEqual(timing.environment, initialTiming.environment); - assert.strictEqual(timing.bootstrapComplete, initialTiming.bootstrapComplete); - - assert.strictEqual(typeof timing.loopStart, 'number'); - assert.strictEqual(typeof timing.loopExit, 'number'); -} - -log('check initial nodeTiming'); -checkNodeTiming(initialTiming); assert.strictEqual(initialTiming.loopExit, -1); function checkValue(timing, name, min, max) { From 7b624c30b2902375c3c81885fc964b81da0891db Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Fri, 29 Sep 2023 18:20:46 +0200 Subject: [PATCH 23/31] doc: update CHANGELOG_V20 about vm fixes Jest users might need additional changes to unblock upgrade from v16. PR-URL: https://github.com/nodejs/node/pull/49951 Reviewed-By: Benjamin Gruenbaum Reviewed-By: Richard Lau --- doc/changelogs/CHANGELOG_V20.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/changelogs/CHANGELOG_V20.md b/doc/changelogs/CHANGELOG_V20.md index 7a15aea0a0eb4eb..27f295c830671b8 100644 --- a/doc/changelogs/CHANGELOG_V20.md +++ b/doc/changelogs/CHANGELOG_V20.md @@ -71,7 +71,7 @@ This rework addressed a series of long-standing memory leaks and use-after-free * `vm.SyntheticModule` * `vm.SourceTextModule` -This should enable affected users (in particular Jest users) to upgrade from older versions of Node.js. +This should enable affected users to upgrade from older versions of Node.js. Contributed by Joyee Cheung in [#48510](https://github.com/nodejs/node/pull/48510). From 53b554567271cf550b0004cad3b7d78545e5791f Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Fri, 29 Sep 2023 20:13:44 +0200 Subject: [PATCH 24/31] stream: writable state bitmap PR-URL: https://github.com/nodejs/node/pull/49899 Reviewed-By: Benjamin Gruenbaum Reviewed-By: Matteo Collina Reviewed-By: Raz Luvaton Reviewed-By: Yagiz Nizipli --- benchmark/streams/writable-manywrites.js | 4 +- lib/internal/streams/writable.js | 267 ++++++++++++++++------- 2 files changed, 192 insertions(+), 79 deletions(-) diff --git a/benchmark/streams/writable-manywrites.js b/benchmark/streams/writable-manywrites.js index e6ab65162c366cf..d244c7d606479e3 100644 --- a/benchmark/streams/writable-manywrites.js +++ b/benchmark/streams/writable-manywrites.js @@ -4,7 +4,7 @@ const common = require('../common'); const Writable = require('stream').Writable; const bench = common.createBenchmark(main, { - n: [2e6], + n: [1e5], sync: ['yes', 'no'], writev: ['yes', 'no'], callback: ['yes', 'no'], @@ -13,7 +13,7 @@ const bench = common.createBenchmark(main, { function main({ n, sync, writev, callback, len }) { const b = Buffer.allocUnsafe(len); - const s = new Writable(); + const s = new Writable({ highWaterMark: 16 * 1024 }); sync = sync === 'yes'; const writecb = (cb) => { diff --git a/lib/internal/streams/writable.js b/lib/internal/streams/writable.js index 595aadc23c8bece..7b1896baeb47c2d 100644 --- a/lib/internal/streams/writable.js +++ b/lib/internal/streams/writable.js @@ -72,7 +72,11 @@ ObjectSetPrototypeOf(Writable, Stream); function nop() {} -const kOnFinished = Symbol('kOnFinished'); +const kOnFinishedValue = Symbol('kOnFinishedValue'); +const kErroredValue = Symbol('kErroredValue'); +const kDefaultEncodingValue = Symbol('kDefaultEncodingValue'); +const kWriteCbValue = Symbol('kWriteCbValue'); +const kAfterWriteTickInfoValue = Symbol('kAfterWriteTickInfoValue'); const kObjectMode = 1 << 0; const kEnded = 1 << 1; @@ -94,6 +98,16 @@ const kBufferProcessing = 1 << 16; const kPrefinished = 1 << 17; const kAllBuffers = 1 << 18; const kAllNoop = 1 << 19; +const kOnFinished = 1 << 20; +const kErrored = 1 << 21; +const kHasWritable = 1 << 22; +const kWritable = 1 << 23; +const kCorked = 1 << 24; +const kDefaultUTF8Encoding = 1 << 25; +const kWriteCb = 1 << 26; +const kExpectWriteCb = 1 << 27; +const kAfterWriteTickInfo = 1 << 28; +const kAfterWritePending = 1 << 29; // TODO(benjamingr) it is likely slower to do it this way than with free functions function makeBitMapDescriptor(bit) { @@ -176,6 +190,85 @@ ObjectDefineProperties(WritableState.prototype, { allBuffers: makeBitMapDescriptor(kAllBuffers), allNoop: makeBitMapDescriptor(kAllNoop), + + // Indicates whether the stream has errored. When true all write() calls + // should return false. This is needed since when autoDestroy + // is disabled we need a way to tell whether the stream has failed. + // This is/should be a cold path. + errored: { + __proto__: null, + enumerable: false, + get() { return (this.state & kErrored) !== 0 ? this[kErroredValue] : null; }, + set(value) { + if (value) { + this[kErroredValue] = value; + this.state |= kErrored; + } else { + this.state &= ~kErrored; + } + }, + }, + + writable: { + __proto__: null, + enumerable: false, + get() { return (this.state & kHasWritable) !== 0 ? (this.state & kWritable) !== 0 : undefined; }, + set(value) { + if (value == null) { + this.state &= ~(kHasWritable | kWritable); + } else if (value) { + this.state |= (kHasWritable | kWritable); + } else { + this.state |= kHasWritable; + this.state &= ~kWritable; + } + }, + }, + + defaultEncoding: { + __proto__: null, + enumerable: false, + get() { return (this.state & kDefaultUTF8Encoding) !== 0 ? 'utf8' : this[kDefaultEncodingValue]; }, + set(value) { + if (value === 'utf8' || value === 'utf-8') { + this.state |= kDefaultUTF8Encoding; + } else { + this.state &= ~kDefaultUTF8Encoding; + this[kDefaultEncodingValue] = value; + } + }, + }, + + // The callback that the user supplies to write(chunk, encoding, cb). + writecb: { + __proto__: null, + enumerable: false, + get() { return (this.state & kWriteCb) !== 0 ? this[kWriteCbValue] : nop; }, + set(value) { + if (value) { + this[kWriteCbValue] = value; + this.state |= kWriteCb; + } else { + this.state &= ~kWriteCb; + } + }, + }, + + // Storage for data passed to the afterWrite() callback in case of + // synchronous _write() completion. + afterWriteTickInfo: { + __proto__: null, + enumerable: false, + get() { return (this.state & kAfterWriteTickInfo) !== 0 ? this[kAfterWriteTickInfoValue] : null; }, + set(value) { + if (value) { + this[kAfterWriteTickInfoValue] = value; + this.state |= kAfterWriteTickInfo; + } else { + this.state &= ~kAfterWriteTickInfo; + } + }, + }, }); function WritableState(options, stream, isDuplex) { @@ -213,10 +306,11 @@ function WritableState(options, stream, isDuplex) { // encoding is 'binary' so we have to make this configurable. // Everything else in the universe uses 'utf8', though. const defaultEncoding = options?.defaultEncoding; - if (defaultEncoding == null) { - this.defaultEncoding = 'utf8'; + if (defaultEncoding == null || defaultEncoding === 'utf8' || defaultEncoding === 'utf-8') { + this.state |= kDefaultUTF8Encoding; } else if (Buffer.isEncoding(defaultEncoding)) { - this.defaultEncoding = defaultEncoding; + this.state &= ~kDefaultUTF8Encoding; + this[kDefaultEncodingValue] = defaultEncoding; } else { throw new ERR_UNKNOWN_ENCODING(defaultEncoding); } @@ -232,28 +326,14 @@ function WritableState(options, stream, isDuplex) { // The callback that's passed to _write(chunk, cb). this.onwrite = onwrite.bind(undefined, stream); - // The callback that the user supplies to write(chunk, encoding, cb). - this.writecb = null; - // The amount that is being written when _write is called. this.writelen = 0; - // Storage for data passed to the afterWrite() callback in case of - // synchronous _write() completion. - this.afterWriteTickInfo = null; - resetBuffer(this); // Number of pending user-supplied write callbacks // this must be 0 before 'finish' can be emitted. this.pendingcb = 0; - - // Indicates whether the stream has errored. When true all write() calls - // should return false. This is needed since when autoDestroy - // is disabled we need a way to tell whether the stream has failed. - this.errored = null; - - this[kOnFinished] = []; } function resetBuffer(state) { @@ -344,10 +424,10 @@ function _write(stream, chunk, encoding, cb) { if (typeof encoding === 'function') { cb = encoding; - encoding = state.defaultEncoding; + encoding = (state.state & kDefaultUTF8Encoding) !== 0 ? 'utf8' : state.defaultEncoding; } else { if (!encoding) - encoding = state.defaultEncoding; + encoding = (state.state & kDefaultUTF8Encoding) !== 0 ? 'utf8' : state.defaultEncoding; else if (encoding !== 'buffer' && !Buffer.isEncoding(encoding)) throw new ERR_UNKNOWN_ENCODING(encoding); if (typeof cb !== 'function') @@ -394,7 +474,10 @@ Writable.prototype.write = function(chunk, encoding, cb) { }; Writable.prototype.cork = function() { - this._writableState.corked++; + const state = this._writableState; + + state.state |= kCorked; + state.corked++; }; Writable.prototype.uncork = function() { @@ -403,6 +486,10 @@ Writable.prototype.uncork = function() { if (state.corked) { state.corked--; + if (!state.corked) { + state.state &= ~kCorked; + } + if ((state.state & kWriting) === 0) clearBuffer(this, state); } @@ -428,11 +515,13 @@ function writeOrBuffer(stream, state, chunk, encoding, callback) { // stream._write resets state.length const ret = state.length < state.highWaterMark; + // We must ensure that previous needDrain will not be reset to false. - if (!ret) + if (!ret) { state.state |= kNeedDrain; + } - if ((state.state & kWriting) !== 0 || state.corked || state.errored || (state.state & kConstructed) === 0) { + if ((state.state & (kWriting | kErrored | kCorked | kConstructed)) !== kConstructed) { state.buffered.push({ chunk, encoding, callback }); if ((state.state & kAllBuffers) !== 0 && encoding !== 'buffer') { state.state &= ~kAllBuffers; @@ -442,21 +531,25 @@ function writeOrBuffer(stream, state, chunk, encoding, callback) { } } else { state.writelen = len; - state.writecb = callback; - state.state |= kWriting | kSync; + if (callback !== nop) { + state.writecb = callback; + } + state.state |= kWriting | kSync | kExpectWriteCb; stream._write(chunk, encoding, state.onwrite); state.state &= ~kSync; } // Return false if errored or destroyed in order to break // any synchronous while(stream.write(data)) loops. - return ret && !state.errored && (state.state & kDestroyed) === 0; + return ret && (state.state & (kDestroyed | kErrored)) === 0; } function doWrite(stream, state, writev, len, chunk, encoding, cb) { state.writelen = len; - state.writecb = cb; - state.state |= kWriting | kSync; + if (cb !== nop) { + state.writecb = cb; + } + state.state |= kWriting | kSync | kExpectWriteCb; if ((state.state & kDestroyed) !== 0) state.onwrite(new ERR_STREAM_DESTROYED('write')); else if (writev) @@ -481,16 +574,16 @@ function onwriteError(stream, state, er, cb) { function onwrite(stream, er) { const state = stream._writableState; - const sync = (state.state & kSync) !== 0; - const cb = state.writecb; - if (typeof cb !== 'function') { + if ((state.state & kExpectWriteCb) === 0) { errorOrDestroy(stream, new ERR_MULTIPLE_CALLBACK()); return; } - state.state &= ~kWriting; - state.writecb = null; + const sync = (state.state & kSync) !== 0; + const cb = (state.state & kWriteCb) !== 0 ? state[kWriteCbValue] : nop; + + state.state &= ~(kWriting | kExpectWriteCb | kWriteCb); state.length -= state.writelen; state.writelen = 0; @@ -523,12 +616,20 @@ function onwrite(stream, er) { // the same. In that case, we do not schedule a new nextTick(), but // rather just increase a counter, to improve performance and avoid // memory allocations. - if (state.afterWriteTickInfo !== null && - state.afterWriteTickInfo.cb === cb) { + if (cb === nop) { + if ((state.state & kAfterWritePending) === 0) { + process.nextTick(afterWrite, stream, state, 1, cb); + state.state |= kAfterWritePending; + } else { + state.pendingcb -= 1; + } + } else if (state.afterWriteTickInfo !== null && + state.afterWriteTickInfo.cb === cb) { state.afterWriteTickInfo.count++; } else { state.afterWriteTickInfo = { count: 1, cb, stream, state }; process.nextTick(afterWriteTick, state.afterWriteTickInfo); + state.state |= kAfterWritePending; } } else { afterWrite(stream, state, 1, cb); @@ -542,7 +643,9 @@ function afterWriteTick({ stream, state, count, cb }) { } function afterWrite(stream, state, count, cb) { - const needDrain = (state.state & (kEnding | kNeedDrain)) === kNeedDrain && !stream.destroyed && state.length === 0; + state.state &= ~kAfterWritePending; + + const needDrain = (state.state & (kEnding | kNeedDrain | kDestroyed)) === kNeedDrain && state.length === 0; if (needDrain) { state.state &= ~kNeedDrain; stream.emit('drain'); @@ -573,19 +676,16 @@ function errorBuffer(state) { callback(state.errored ?? new ERR_STREAM_DESTROYED('write')); } - const onfinishCallbacks = state[kOnFinished].splice(0); - for (let i = 0; i < onfinishCallbacks.length; i++) { - onfinishCallbacks[i](state.errored ?? new ERR_STREAM_DESTROYED('end')); - } + + callFinishedCallbacks(state, state.errored ?? new ERR_STREAM_DESTROYED('end')); resetBuffer(state); } // If there's something in the buffer waiting, then process it. function clearBuffer(stream, state) { - if (state.corked || - (state.state & (kDestroyed | kBufferProcessing)) !== 0 || - (state.state & kConstructed) === 0) { + if ((state.state & (kDestroyed | kBufferProcessing | kCorked)) !== 0 || + (state.state & kConstructed) === 0) { return; } @@ -661,7 +761,7 @@ Writable.prototype.end = function(chunk, encoding, cb) { let err; - if (chunk !== null && chunk !== undefined) { + if (chunk != null) { const ret = _write(this, chunk, encoding); if (ret instanceof Error) { err = ret; @@ -669,14 +769,14 @@ Writable.prototype.end = function(chunk, encoding, cb) { } // .end() fully uncorks. - if (state.corked) { + if ((state.state & kCorked) !== 0) { state.corked = 1; this.uncork(); } if (err) { // Do nothing... - } else if (!state.errored && (state.state & kEnding) === 0) { + } else if ((state.state & (kEnding | kErrored)) === 0) { // This is forgiving in terms of unnecessary calls to end() and can hide // logic errors. However, usually such errors are harmless and causing a // hard error can be disproportionately destructive. It is not always @@ -698,7 +798,9 @@ Writable.prototype.end = function(chunk, encoding, cb) { } else if ((state.state & kFinished) !== 0) { process.nextTick(cb, null); } else { - state[kOnFinished].push(cb); + state.state |= kOnFinished; + state[kOnFinishedValue] ??= []; + state[kOnFinishedValue].push(cb); } } @@ -715,10 +817,10 @@ function needFinish(state) { kFinished | kWriting | kErrorEmitted | - kCloseEmitted + kCloseEmitted | + kErrored )) === (kEnding | kConstructed) && state.length === 0 && - !state.errored && state.buffered.length === 0); } @@ -734,10 +836,7 @@ function callFinal(stream, state) { state.pendingcb--; if (err) { - const onfinishCallbacks = state[kOnFinished].splice(0); - for (let i = 0; i < onfinishCallbacks.length; i++) { - onfinishCallbacks[i](err); - } + callFinishedCallbacks(state, err); errorOrDestroy(stream, err, (state.state & kSync) !== 0); } else if (needFinish(state)) { state.state |= kPrefinished; @@ -799,10 +898,7 @@ function finish(stream, state) { state.pendingcb--; state.state |= kFinished; - const onfinishCallbacks = state[kOnFinished].splice(0); - for (let i = 0; i < onfinishCallbacks.length; i++) { - onfinishCallbacks[i](null); - } + callFinishedCallbacks(state, null); stream.emit('finish'); @@ -822,8 +918,20 @@ function finish(stream, state) { } } -ObjectDefineProperties(Writable.prototype, { +function callFinishedCallbacks(state, err) { + if ((state.state & kOnFinished) === 0) { + return; + } + + const onfinishCallbacks = state[kOnFinishedValue]; + state[kOnFinishedValue] = null; + state.state &= ~kOnFinished; + for (let i = 0; i < onfinishCallbacks.length; i++) { + onfinishCallbacks[i](err); + } +} +ObjectDefineProperties(Writable.prototype, { closed: { __proto__: null, get() { @@ -867,60 +975,64 @@ ObjectDefineProperties(Writable.prototype, { writableFinished: { __proto__: null, get() { - return this._writableState ? (this._writableState.state & kFinished) !== 0 : false; + const state = this._writableState; + return state ? (state.state & kFinished) !== 0 : false; }, }, writableObjectMode: { __proto__: null, get() { - return this._writableState ? (this._writableState.state & kObjectMode) !== 0 : false; + const state = this._writableState; + return state ? (state.state & kObjectMode) !== 0 : false; }, }, writableBuffer: { __proto__: null, get() { - return this._writableState && this._writableState.getBuffer(); + const state = this._writableState; + return state && state.getBuffer(); }, }, writableEnded: { __proto__: null, get() { - return this._writableState ? (this._writableState.state & kEnding) !== 0 : false; + const state = this._writableState; + return state ? (state.state & kEnding) !== 0 : false; }, }, writableNeedDrain: { __proto__: null, get() { - const wState = this._writableState; - if (!wState) return false; - - // !destroyed && !ending && needDrain - return (wState.state & (kDestroyed | kEnding | kNeedDrain)) === kNeedDrain; + const state = this._writableState; + return state ? (state.state & (kDestroyed | kEnding | kNeedDrain)) === kNeedDrain : false; }, }, writableHighWaterMark: { __proto__: null, get() { - return this._writableState && this._writableState.highWaterMark; + const state = this._writableState; + return state && state.highWaterMark; }, }, writableCorked: { __proto__: null, get() { - return this._writableState ? this._writableState.corked : 0; + const state = this._writableState; + return state ? state.corked : 0; }, }, writableLength: { __proto__: null, get() { - return this._writableState && this._writableState.length; + const state = this._writableState; + return state && state.length; }, }, @@ -928,18 +1040,19 @@ ObjectDefineProperties(Writable.prototype, { __proto__: null, enumerable: false, get() { - return this._writableState ? this._writableState.errored : null; + const state = this._writableState; + return state ? state.errored : null; }, }, writableAborted: { __proto__: null, - enumerable: false, get: function() { - return !!( - this._writableState.writable !== false && - ((this._writableState.state & kDestroyed) !== 0 || this._writableState.errored) && - (this._writableState.state & kFinished) === 0 + const state = this._writableState; + return ( + (state.state & (kHasWritable | kWritable)) !== kHasWritable && + (state.state & (kDestroyed | kErrored)) !== 0 && + (state.state & kFinished) === 0 ); }, }, @@ -952,7 +1065,7 @@ Writable.prototype.destroy = function(err, cb) { // Invoke pending callbacks. if ((state.state & kDestroyed) === 0 && (state.bufferedIndex < state.buffered.length || - state[kOnFinished].length)) { + (state.state & kOnFinished) !== 0)) { process.nextTick(errorBuffer, state); } From 035e06317ac7a8117f01005c78e9030b8e6e26f0 Mon Sep 17 00:00:00 2001 From: Richard Lau Date: Thu, 28 Sep 2023 13:51:44 +0100 Subject: [PATCH 25/31] test: disambiguate AIX and IBM i When built with Python 3.9 on IBM i, `process.platform` will return `os400` instead of `aix`. In preparation for this, make `common.isAIX` only return true for AIX and update the tests to add checks for `common.isIBMi` where they were missing. PR-URL: https://github.com/nodejs/node/pull/48056 Refs: https://github.com/nodejs/node/pull/46739 Refs: https://github.com/nodejs/build/pull/3358 Reviewed-By: Moshe Atlow Reviewed-By: Michael Dawson Reviewed-By: Luigi Pinca --- test/abort/test-addon-uv-handle-leak.js | 1 + test/common/index.js | 9 ++++++--- test/known_issues/test-cwd-enoent-file.js | 2 +- test/parallel/test-cwd-enoent-preload.js | 2 +- test/parallel/test-cwd-enoent-repl.js | 2 +- test/parallel/test-cwd-enoent.js | 2 +- test/parallel/test-fs-readfile-pipe-large.js | 2 +- test/parallel/test-fs-readfile-pipe.js | 2 +- test/parallel/test-fs-readfilesync-pipe-large.js | 2 +- test/parallel/test-fs-realpath-pipe.js | 2 +- test/parallel/test-fs-utimes.js | 2 +- test/parallel/test-os.js | 13 ++++++++----- .../test-process-dlopen-error-message-crash.js | 2 +- 13 files changed, 25 insertions(+), 18 deletions(-) diff --git a/test/abort/test-addon-uv-handle-leak.js b/test/abort/test-addon-uv-handle-leak.js index e494b12f2de5586..d2c4f8e646f4578 100644 --- a/test/abort/test-addon-uv-handle-leak.js +++ b/test/abort/test-addon-uv-handle-leak.js @@ -78,6 +78,7 @@ if (process.argv[2] === 'child') { if (!(common.isFreeBSD || common.isAIX || + common.isIBMi || (common.isLinux && !isGlibc()) || common.isWindows)) { assert(stderr.includes('ExampleOwnerClass'), stderr); diff --git a/test/common/index.js b/test/common/index.js index 9d8accf939a1ff1..cdb14b3c70cc303 100644 --- a/test/common/index.js +++ b/test/common/index.js @@ -123,7 +123,6 @@ if (process.argv.length === 2 && } const isWindows = process.platform === 'win32'; -const isAIX = process.platform === 'aix'; const isSunOS = process.platform === 'sunos'; const isFreeBSD = process.platform === 'freebsd'; const isOpenBSD = process.platform === 'openbsd'; @@ -274,7 +273,7 @@ function platformTimeout(ms) { if (process.features.debug) ms = multipliers.two * ms; - if (isAIX) + if (exports.isAIX || exports.isIBMi) return multipliers.two * ms; // Default localhost speed is slower on AIX if (isPi) @@ -928,7 +927,6 @@ const common = { hasQuic, hasMultiLocalhost, invalidArgTypeHelper, - isAIX, isAlive, isAsan, isDumbTerminal, @@ -999,7 +997,12 @@ const common = { }, // On IBMi, process.platform and os.platform() both return 'aix', + // when built with Python versions earlier than 3.9. // It is not enough to differentiate between IBMi and real AIX system. + get isAIX() { + return require('os').type() === 'AIX'; + }, + get isIBMi() { return require('os').type() === 'OS400'; }, diff --git a/test/known_issues/test-cwd-enoent-file.js b/test/known_issues/test-cwd-enoent-file.js index 0f75896134f7e30..6d99987895baf44 100644 --- a/test/known_issues/test-cwd-enoent-file.js +++ b/test/known_issues/test-cwd-enoent-file.js @@ -5,7 +5,7 @@ const common = require('../common'); const assert = require('assert'); -if (common.isSunOS || common.isWindows || common.isAIX) { +if (common.isSunOS || common.isWindows || common.isAIX || common.isIBMi) { // The current working directory cannot be removed on these platforms. // Change this to common.skip() when this is no longer a known issue test. assert.fail('cannot rmdir current working directory'); diff --git a/test/parallel/test-cwd-enoent-preload.js b/test/parallel/test-cwd-enoent-preload.js index 2077d9c1478335e..21b20d6d035672c 100644 --- a/test/parallel/test-cwd-enoent-preload.js +++ b/test/parallel/test-cwd-enoent-preload.js @@ -1,7 +1,7 @@ 'use strict'; const common = require('../common'); // Fails with EINVAL on SmartOS, EBUSY on Windows, EBUSY on AIX. -if (common.isSunOS || common.isWindows || common.isAIX) +if (common.isSunOS || common.isWindows || common.isAIX || common.isIBMi) common.skip('cannot rmdir current working directory'); if (!common.isMainThread) common.skip('process.chdir is not available in Workers'); diff --git a/test/parallel/test-cwd-enoent-repl.js b/test/parallel/test-cwd-enoent-repl.js index 5ea8abc7e42b523..0a61cbfbced9b45 100644 --- a/test/parallel/test-cwd-enoent-repl.js +++ b/test/parallel/test-cwd-enoent-repl.js @@ -1,7 +1,7 @@ 'use strict'; const common = require('../common'); // Fails with EINVAL on SmartOS, EBUSY on Windows, EBUSY on AIX. -if (common.isSunOS || common.isWindows || common.isAIX) +if (common.isSunOS || common.isWindows || common.isAIX || common.isIBMi) common.skip('cannot rmdir current working directory'); if (!common.isMainThread) common.skip('process.chdir is not available in Workers'); diff --git a/test/parallel/test-cwd-enoent.js b/test/parallel/test-cwd-enoent.js index 8beb1e3fbe0a058..876888bc2be5189 100644 --- a/test/parallel/test-cwd-enoent.js +++ b/test/parallel/test-cwd-enoent.js @@ -1,7 +1,7 @@ 'use strict'; const common = require('../common'); // Fails with EINVAL on SmartOS, EBUSY on Windows, EBUSY on AIX. -if (common.isSunOS || common.isWindows || common.isAIX) +if (common.isSunOS || common.isWindows || common.isAIX || common.isIBMi) common.skip('cannot rmdir current working directory'); if (!common.isMainThread) common.skip('process.chdir is not available in Workers'); diff --git a/test/parallel/test-fs-readfile-pipe-large.js b/test/parallel/test-fs-readfile-pipe-large.js index 1a685ebc612b85c..4376774bb411d6e 100644 --- a/test/parallel/test-fs-readfile-pipe-large.js +++ b/test/parallel/test-fs-readfile-pipe-large.js @@ -3,7 +3,7 @@ const common = require('../common'); // Simulate `cat readfile.js | node readfile.js` -if (common.isWindows || common.isAIX) +if (common.isWindows || common.isAIX || common.isIBMi) common.skip(`No /dev/stdin on ${process.platform}.`); const assert = require('assert'); diff --git a/test/parallel/test-fs-readfile-pipe.js b/test/parallel/test-fs-readfile-pipe.js index 0cffbd0f5aa162d..79d5699fef64a41 100644 --- a/test/parallel/test-fs-readfile-pipe.js +++ b/test/parallel/test-fs-readfile-pipe.js @@ -24,7 +24,7 @@ const common = require('../common'); // Simulate `cat readfile.js | node readfile.js` -if (common.isWindows || common.isAIX) +if (common.isWindows || common.isAIX || common.isIBMi) common.skip(`No /dev/stdin on ${process.platform}.`); const assert = require('assert'); diff --git a/test/parallel/test-fs-readfilesync-pipe-large.js b/test/parallel/test-fs-readfilesync-pipe-large.js index 2cb857a329fbee8..5450337c4f680a7 100644 --- a/test/parallel/test-fs-readfilesync-pipe-large.js +++ b/test/parallel/test-fs-readfilesync-pipe-large.js @@ -3,7 +3,7 @@ const common = require('../common'); // Simulate `cat readfile.js | node readfile.js` -if (common.isWindows || common.isAIX) +if (common.isWindows || common.isAIX || common.isIBMi) common.skip(`No /dev/stdin on ${process.platform}.`); const assert = require('assert'); diff --git a/test/parallel/test-fs-realpath-pipe.js b/test/parallel/test-fs-realpath-pipe.js index 29fe1d3b7d28e1c..f637642ca21d679 100644 --- a/test/parallel/test-fs-realpath-pipe.js +++ b/test/parallel/test-fs-realpath-pipe.js @@ -2,7 +2,7 @@ const common = require('../common'); -if (common.isWindows || common.isAIX) +if (common.isWindows || common.isAIX || common.isIBMi) common.skip(`No /dev/stdin on ${process.platform}.`); const assert = require('assert'); diff --git a/test/parallel/test-fs-utimes.js b/test/parallel/test-fs-utimes.js index 8f8286f8a3343f4..e6ae75d4e33a5f4 100644 --- a/test/parallel/test-fs-utimes.js +++ b/test/parallel/test-fs-utimes.js @@ -39,7 +39,7 @@ function stat_resource(resource, statSync = fs.statSync) { const stats = fs.fstatSync(resource); // Ensure mtime has been written to disk // except for directories on AIX where it cannot be synced - if (common.isAIX && stats.isDirectory()) + if ((common.isAIX || common.isIBMi) && stats.isDirectory()) return stats; fs.fsyncSync(resource); return fs.fstatSync(resource); diff --git a/test/parallel/test-os.js b/test/parallel/test-os.js index d82f2ece3159a66..f7059260ce507eb 100644 --- a/test/parallel/test-os.js +++ b/test/parallel/test-os.js @@ -81,11 +81,14 @@ const hostname = os.hostname(); is.string(hostname); assert.ok(hostname.length > 0); -const DUMMY_PRIORITY = 10; -os.setPriority(DUMMY_PRIORITY); -const priority = os.getPriority(); -is.number(priority); -assert.strictEqual(priority, DUMMY_PRIORITY); +// IBMi process priority is different. +if (!common.isIBMi) { + const DUMMY_PRIORITY = 10; + os.setPriority(DUMMY_PRIORITY); + const priority = os.getPriority(); + is.number(priority); + assert.strictEqual(priority, DUMMY_PRIORITY); +} // On IBMi, os.uptime() returns 'undefined' if (!common.isIBMi) { diff --git a/test/parallel/test-process-dlopen-error-message-crash.js b/test/parallel/test-process-dlopen-error-message-crash.js index d678021764e8f42..de8b3033195a466 100644 --- a/test/parallel/test-process-dlopen-error-message-crash.js +++ b/test/parallel/test-process-dlopen-error-message-crash.js @@ -17,7 +17,7 @@ assert.throws(() => { }, ({ name, code, message }) => { assert.strictEqual(name, 'Error'); assert.strictEqual(code, 'ERR_DLOPEN_FAILED'); - if (!common.isAIX) { + if (!common.isAIX && !common.isIBMi) { assert.match(message, /foo-%s\.node/); } return true; From 6322d4f5871036d9dba70e9897e55e26cd79c163 Mon Sep 17 00:00:00 2001 From: Richard Lau Date: Thu, 18 May 2023 12:51:19 +0100 Subject: [PATCH 26/31] build: fix IBM i build with Python 3.9 Python 3.9 on IBM i returns "os400" for `sys.platform`. PR-URL: https://github.com/nodejs/node/pull/48056 Refs: https://github.com/nodejs/node/pull/46739 Refs: https://github.com/nodejs/build/pull/3358 Reviewed-By: Moshe Atlow Reviewed-By: Michael Dawson Reviewed-By: Luigi Pinca --- deps/openssl/openssl-cl_asm.gypi | 2 +- deps/openssl/openssl-cl_asm_avx2.gypi | 2 +- deps/openssl/openssl-cl_no_asm.gypi | 2 +- deps/openssl/openssl-fips_asm.gypi | 2 +- deps/openssl/openssl-fips_asm_avx2.gypi | 2 +- deps/openssl/openssl-fips_no_asm.gypi | 2 +- deps/openssl/openssl_asm.gypi | 2 +- deps/openssl/openssl_asm_avx2.gypi | 2 +- deps/openssl/openssl_common.gypi | 2 +- deps/openssl/openssl_no_asm.gypi | 2 +- 10 files changed, 10 insertions(+), 10 deletions(-) diff --git a/deps/openssl/openssl-cl_asm.gypi b/deps/openssl/openssl-cl_asm.gypi index 8791e005bf0f41a..cd10355c1712282 100644 --- a/deps/openssl/openssl-cl_asm.gypi +++ b/deps/openssl/openssl-cl_asm.gypi @@ -1,6 +1,6 @@ { 'conditions': [ - ['target_arch=="ppc64" and OS=="aix"', { + ['target_arch=="ppc64" and OS in ("aix", "os400")', { 'includes': ['config/archs/aix64-gcc-as/asm/openssl-cl.gypi'], }, 'target_arch=="ppc64" and OS=="linux" and node_byteorder =="little"', { 'includes': ['config/archs/linux-ppc64le/asm/openssl-cl.gypi'], diff --git a/deps/openssl/openssl-cl_asm_avx2.gypi b/deps/openssl/openssl-cl_asm_avx2.gypi index 815598676770c68..50b5a9c375bd8d1 100644 --- a/deps/openssl/openssl-cl_asm_avx2.gypi +++ b/deps/openssl/openssl-cl_asm_avx2.gypi @@ -1,6 +1,6 @@ { 'conditions': [ - ['target_arch=="ppc64" and OS=="aix"', { + ['target_arch=="ppc64" and OS in ("aix", "os400")', { 'includes': ['config/archs/aix64-gcc-as/asm_avx2/openssl-cl.gypi'], }, 'target_arch=="ppc64" and OS=="linux" and node_byteorder =="little"', { 'includes': ['config/archs/linux-ppc64le/asm_avx2/openssl-cl.gypi'], diff --git a/deps/openssl/openssl-cl_no_asm.gypi b/deps/openssl/openssl-cl_no_asm.gypi index 22bcf1c3c77d842..0964fb36739b3ac 100644 --- a/deps/openssl/openssl-cl_no_asm.gypi +++ b/deps/openssl/openssl-cl_no_asm.gypi @@ -1,6 +1,6 @@ { 'conditions': [ - ['target_arch=="ppc64" and OS=="aix"', { + ['target_arch=="ppc64" and OS in ("aix", "os400")', { 'includes': ['config/archs/aix64-gcc-as/no-asm/openssl-cl.gypi'], }, 'target_arch=="ppc64" and OS=="linux" and node_byteorder =="little"', { 'includes': ['config/archs/linux-ppc64le/no-asm/openssl-cl.gypi'], diff --git a/deps/openssl/openssl-fips_asm.gypi b/deps/openssl/openssl-fips_asm.gypi index d0717df2a5094fa..631df9eb893288b 100644 --- a/deps/openssl/openssl-fips_asm.gypi +++ b/deps/openssl/openssl-fips_asm.gypi @@ -1,6 +1,6 @@ { 'conditions': [ - ['target_arch=="ppc64" and OS=="aix"', { + ['target_arch=="ppc64" and OS in ("aix", "os400")', { 'includes': ['config/archs/aix64-gcc-as/asm/openssl-fips.gypi'], }, 'target_arch=="ppc64" and OS=="linux" and node_byteorder =="little"', { 'includes': ['config/archs/linux-ppc64le/asm/openssl-fips.gypi'], diff --git a/deps/openssl/openssl-fips_asm_avx2.gypi b/deps/openssl/openssl-fips_asm_avx2.gypi index d2a2a4bc11413fa..4d63cacf29d0407 100644 --- a/deps/openssl/openssl-fips_asm_avx2.gypi +++ b/deps/openssl/openssl-fips_asm_avx2.gypi @@ -1,6 +1,6 @@ { 'conditions': [ - ['target_arch=="ppc64" and OS=="aix"', { + ['target_arch=="ppc64" and OS in ("aix", "os400")', { 'includes': ['config/archs/aix64-gcc-as/asm_avx2/openssl-fips.gypi'], }, 'target_arch=="ppc64" and OS=="linux" and node_byteorder =="little"', { 'includes': ['config/archs/linux-ppc64le/asm_avx2/openssl-fips.gypi'], diff --git a/deps/openssl/openssl-fips_no_asm.gypi b/deps/openssl/openssl-fips_no_asm.gypi index d598bde68c783d9..7fdfd772abbce8f 100644 --- a/deps/openssl/openssl-fips_no_asm.gypi +++ b/deps/openssl/openssl-fips_no_asm.gypi @@ -1,7 +1,7 @@ { 'defines': ['OPENSSL_NO_ASM'], 'conditions': [ - ['target_arch=="ppc64" and OS=="aix"', { + ['target_arch=="ppc64" and OS in ("aix", "os400")', { 'includes': ['config/archs/aix64-gcc-as/no-asm/openssl-fips.gypi'], }, 'target_arch=="ppc64" and OS=="linux" and node_byteorder =="little"', { 'includes': ['config/archs/linux-ppc64le/no-asm/openssl-fips.gypi'], diff --git a/deps/openssl/openssl_asm.gypi b/deps/openssl/openssl_asm.gypi index dbd5a5f69ff8d94..dd7e636eb088931 100644 --- a/deps/openssl/openssl_asm.gypi +++ b/deps/openssl/openssl_asm.gypi @@ -1,6 +1,6 @@ { 'conditions': [ - ['target_arch=="ppc64" and OS=="aix"', { + ['target_arch=="ppc64" and OS in ("aix", "os400")', { 'includes': ['config/archs/aix64-gcc-as/asm/openssl.gypi'], }, 'target_arch=="ppc64" and OS=="linux" and node_byteorder =="little"', { 'includes': ['config/archs/linux-ppc64le/asm/openssl.gypi'], diff --git a/deps/openssl/openssl_asm_avx2.gypi b/deps/openssl/openssl_asm_avx2.gypi index 2883f83d6943948..6a9c56d76a211ac 100644 --- a/deps/openssl/openssl_asm_avx2.gypi +++ b/deps/openssl/openssl_asm_avx2.gypi @@ -1,6 +1,6 @@ { 'conditions': [ - ['target_arch=="ppc64" and OS=="aix"', { + ['target_arch=="ppc64" and OS in ("aix", "os400")', { 'includes': ['config/archs/aix64-gcc-as/asm_avx2/openssl.gypi'], }, 'target_arch=="ppc64" and OS=="linux" and node_byteorder =="little"', { 'includes': ['config/archs/linux-ppc64le/asm_avx2/openssl.gypi'], diff --git a/deps/openssl/openssl_common.gypi b/deps/openssl/openssl_common.gypi index 256eb7d1808db30..1290fff251fc5bf 100644 --- a/deps/openssl/openssl_common.gypi +++ b/deps/openssl/openssl_common.gypi @@ -13,7 +13,7 @@ ], # build options specific to OS 'conditions': [ - [ 'OS=="aix"', { + [ 'OS in ("aix", "os400")', { # AIX is missing /usr/include/endian.h 'defines': [ '__LITTLE_ENDIAN=1234', diff --git a/deps/openssl/openssl_no_asm.gypi b/deps/openssl/openssl_no_asm.gypi index de0e486f67e90eb..20663decabba233 100644 --- a/deps/openssl/openssl_no_asm.gypi +++ b/deps/openssl/openssl_no_asm.gypi @@ -1,7 +1,7 @@ { 'defines': ['OPENSSL_NO_ASM'], 'conditions': [ - ['target_arch=="ppc64" and OS=="aix"', { + ['target_arch=="ppc64" and OS in ("aix", "os400")', { 'includes': ['config/archs/aix64-gcc-as/no-asm/openssl.gypi'], }, 'target_arch=="ppc64" and OS=="linux" and node_byteorder =="little"', { 'includes': ['config/archs/linux-ppc64le/no-asm/openssl.gypi'], From 3227d7327c02f53662e1d49adaf7e2abb3505cfe Mon Sep 17 00:00:00 2001 From: "Node.js GitHub Bot" Date: Wed, 27 Sep 2023 18:51:03 +0000 Subject: [PATCH 27/31] deps: update uvwasi to 0.0.19 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR-URL: https://github.com/nodejs/node/pull/49908 Reviewed-By: Tobias Nießen Reviewed-By: Colin Ihrig Reviewed-By: Luigi Pinca --- deps/uvwasi/include/uvwasi.h | 11 +- deps/uvwasi/src/fd_table.c | 69 +++- deps/uvwasi/src/fd_table.h | 5 + deps/uvwasi/src/sync_helpers.c | 95 ++++++ deps/uvwasi/src/sync_helpers.h | 27 ++ deps/uvwasi/src/uvwasi.c | 320 ++++++++++++++++-- deps/uvwasi/src/wasi_rights.h | 5 +- .../maintaining/maintaining-dependencies.md | 6 +- 8 files changed, 491 insertions(+), 47 deletions(-) create mode 100644 deps/uvwasi/src/sync_helpers.c create mode 100644 deps/uvwasi/src/sync_helpers.h diff --git a/deps/uvwasi/include/uvwasi.h b/deps/uvwasi/include/uvwasi.h index d475d3e67512bf3..a458cfe17daaa34 100644 --- a/deps/uvwasi/include/uvwasi.h +++ b/deps/uvwasi/include/uvwasi.h @@ -5,12 +5,13 @@ extern "C" { #endif +#include "uv.h" #include "wasi_serdes.h" #include "wasi_types.h" #define UVWASI_VERSION_MAJOR 0 #define UVWASI_VERSION_MINOR 0 -#define UVWASI_VERSION_PATCH 18 +#define UVWASI_VERSION_PATCH 19 #define UVWASI_VERSION_HEX ((UVWASI_VERSION_MAJOR << 16) | \ (UVWASI_VERSION_MINOR << 8) | \ (UVWASI_VERSION_PATCH)) @@ -47,6 +48,7 @@ typedef struct uvwasi_s { char* env_buf; uvwasi_size_t env_buf_size; const uvwasi_mem_t* allocator; + uv_loop_t* loop; } uvwasi_t; typedef struct uvwasi_preopen_s { @@ -54,10 +56,17 @@ typedef struct uvwasi_preopen_s { const char* real_path; } uvwasi_preopen_t; +typedef struct uvwasi_preopen_socket_s { + const char* address; + int port; +} uvwasi_preopen_socket_t; + typedef struct uvwasi_options_s { uvwasi_size_t fd_table_size; uvwasi_size_t preopenc; uvwasi_preopen_t* preopens; + uvwasi_size_t preopen_socketc; + uvwasi_preopen_socket_t* preopen_sockets; uvwasi_size_t argc; const char** argv; const char** envp; diff --git a/deps/uvwasi/src/fd_table.c b/deps/uvwasi/src/fd_table.c index 7782f1ee43b4209..881d192ff3a3401 100644 --- a/deps/uvwasi/src/fd_table.c +++ b/deps/uvwasi/src/fd_table.c @@ -37,6 +37,7 @@ static uvwasi_errno_t uvwasi__insert_stdio(uvwasi_t* uvwasi, err = uvwasi_fd_table_insert(uvwasi, table, fd, + NULL, name, name, type, @@ -58,6 +59,7 @@ static uvwasi_errno_t uvwasi__insert_stdio(uvwasi_t* uvwasi, uvwasi_errno_t uvwasi_fd_table_insert(uvwasi_t* uvwasi, struct uvwasi_fd_table_t* table, uv_file fd, + uv_tcp_t* sock, const char* mapped_path, const char* real_path, uvwasi_filetype_t type, @@ -78,29 +80,40 @@ uvwasi_errno_t uvwasi_fd_table_insert(uvwasi_t* uvwasi, char* rp_copy; char* np_copy; - mp_len = strlen(mapped_path); - rp_len = strlen(real_path); + if (type != UVWASI_FILETYPE_SOCKET_STREAM) { + mp_len = strlen(mapped_path); + rp_len = strlen(real_path); + } else { + mp_len = 0; + rp_len = 0; + rp_copy = NULL; + mp_copy = NULL; + np_copy = NULL; + } + /* Reserve room for the mapped path, real path, and normalized mapped path. */ entry = (struct uvwasi_fd_wrap_t*) uvwasi__malloc(uvwasi, sizeof(*entry) + mp_len + mp_len + rp_len + 3); if (entry == NULL) return UVWASI_ENOMEM; - mp_copy = (char*)(entry + 1); - rp_copy = mp_copy + mp_len + 1; - np_copy = rp_copy + rp_len + 1; - memcpy(mp_copy, mapped_path, mp_len); - mp_copy[mp_len] = '\0'; - memcpy(rp_copy, real_path, rp_len); - rp_copy[rp_len] = '\0'; - - /* Calculate the normalized version of the mapped path, as it will be used for - any path calculations on this fd. Use the length of the mapped path as an - upper bound for the normalized path length. */ - err = uvwasi__normalize_path(mp_copy, mp_len, np_copy, mp_len); - if (err) { - uvwasi__free(uvwasi, entry); - goto exit; + if (type != UVWASI_FILETYPE_SOCKET_STREAM) { + mp_copy = (char*)(entry + 1); + rp_copy = mp_copy + mp_len + 1; + np_copy = rp_copy + rp_len + 1; + memcpy(mp_copy, mapped_path, mp_len); + mp_copy[mp_len] = '\0'; + memcpy(rp_copy, real_path, rp_len); + rp_copy[rp_len] = '\0'; + + /* Calculate the normalized version of the mapped path, as it will be used for + any path calculations on this fd. Use the length of the mapped path as an + upper bound for the normalized path length. */ + err = uvwasi__normalize_path(mp_copy, mp_len, np_copy, mp_len); + if (err) { + uvwasi__free(uvwasi, entry); + goto exit; + } } uv_rwlock_wrlock(&table->rwlock); @@ -150,6 +163,7 @@ uvwasi_errno_t uvwasi_fd_table_insert(uvwasi_t* uvwasi, entry->id = index; entry->fd = fd; + entry->sock = sock; entry->path = mp_copy; entry->real_path = rp_copy; entry->normalized_path = np_copy; @@ -280,6 +294,7 @@ uvwasi_errno_t uvwasi_fd_table_insert_preopen(uvwasi_t* uvwasi, return uvwasi_fd_table_insert(uvwasi, table, fd, + NULL, path, real_path, UVWASI_FILETYPE_DIRECTORY, @@ -290,6 +305,26 @@ uvwasi_errno_t uvwasi_fd_table_insert_preopen(uvwasi_t* uvwasi, } +uvwasi_errno_t uvwasi_fd_table_insert_preopen_socket(uvwasi_t* uvwasi, + struct uvwasi_fd_table_t* table, + uv_tcp_t* sock) { + if (table == NULL || sock == NULL) + return UVWASI_EINVAL; + + return uvwasi_fd_table_insert(uvwasi, + table, + -1, + sock, + NULL, + NULL, + UVWASI_FILETYPE_SOCKET_STREAM, + UVWASI__RIGHTS_SOCKET_BASE, + UVWASI__RIGHTS_SOCKET_INHERITING, + 1, + NULL); +} + + uvwasi_errno_t uvwasi_fd_table_get(struct uvwasi_fd_table_t* table, const uvwasi_fd_t id, struct uvwasi_fd_wrap_t** wrap, diff --git a/deps/uvwasi/src/fd_table.h b/deps/uvwasi/src/fd_table.h index 0755c2d17fdec25..9a7a825bc85d4c8 100644 --- a/deps/uvwasi/src/fd_table.h +++ b/deps/uvwasi/src/fd_table.h @@ -11,6 +11,7 @@ struct uvwasi_options_s; struct uvwasi_fd_wrap_t { uvwasi_fd_t id; uv_file fd; + uv_tcp_t* sock; char* path; char* real_path; char* normalized_path; @@ -35,6 +36,7 @@ void uvwasi_fd_table_free(struct uvwasi_s* uvwasi, uvwasi_errno_t uvwasi_fd_table_insert(struct uvwasi_s* uvwasi, struct uvwasi_fd_table_t* table, uv_file fd, + uv_tcp_t* sock, const char* mapped_path, const char* real_path, uvwasi_filetype_t type, @@ -47,6 +49,9 @@ uvwasi_errno_t uvwasi_fd_table_insert_preopen(struct uvwasi_s* uvwasi, const uv_file fd, const char* path, const char* real_path); +uvwasi_errno_t uvwasi_fd_table_insert_preopen_socket(struct uvwasi_s* uvwasi, + struct uvwasi_fd_table_t* table, + uv_tcp_t* sock); uvwasi_errno_t uvwasi_fd_table_get(struct uvwasi_fd_table_t* table, const uvwasi_fd_t id, struct uvwasi_fd_wrap_t** wrap, diff --git a/deps/uvwasi/src/sync_helpers.c b/deps/uvwasi/src/sync_helpers.c new file mode 100644 index 000000000000000..c63a62b83d75ed3 --- /dev/null +++ b/deps/uvwasi/src/sync_helpers.c @@ -0,0 +1,95 @@ +#include "uv.h" +#include "sync_helpers.h" +#include "uv_mapping.h" +#include "uvwasi_alloc.h" + +typedef struct free_handle_data_s { + uvwasi_t* uvwasi; + int done; +} free_handle_data_t; + +static void free_handle_cb(uv_handle_t* handle) { + free_handle_data_t* free_handle_data = uv_handle_get_data((uv_handle_t*) handle); + uvwasi__free(free_handle_data->uvwasi, handle); + free_handle_data->done = 1; +} + +int free_handle_sync(struct uvwasi_s* uvwasi, uv_handle_t* handle) { + free_handle_data_t free_handle_data = { uvwasi, 0 }; + uv_handle_set_data(handle, (void*) &free_handle_data); + uv_close(handle, free_handle_cb); + uv_loop_t* handle_loop = uv_handle_get_loop(handle); + while(!free_handle_data.done) { + if (uv_run(handle_loop, UV_RUN_ONCE) == 0) { + break; + } + } + return UVWASI_ESUCCESS; +} + +static void do_stream_shutdown(uv_shutdown_t* req, int status) { + shutdown_data_t* shutdown_data; + shutdown_data = uv_handle_get_data((uv_handle_t*) req->handle); + shutdown_data->status = status; + shutdown_data->done = 1; + } + +int shutdown_stream_sync(struct uvwasi_s* uvwasi, + uv_stream_t* stream, + shutdown_data_t* shutdown_data) { + uv_shutdown_t req; + uv_loop_t* stream_loop; + + shutdown_data->done = 0; + shutdown_data->status = 0; + stream_loop = uv_handle_get_loop((uv_handle_t*) stream); + + uv_handle_set_data((uv_handle_t*) stream, (void*) shutdown_data); + uv_shutdown(&req, stream, do_stream_shutdown); + while (!shutdown_data->done) { + if (uv_run(stream_loop, UV_RUN_ONCE) == 0) { + return UVWASI_ECANCELED; + } + } + return UVWASI_ESUCCESS; +} + +static void recv_alloc_cb(uv_handle_t* handle, size_t suggested_size, uv_buf_t* buf) { + recv_data_t* recv_data; + recv_data = uv_handle_get_data(handle); + buf->base = recv_data->base; + buf->len = recv_data->len; +} + +void do_stream_recv(uv_stream_t* stream, ssize_t nread, const uv_buf_t* buf) { + recv_data_t* recv_data; + recv_data = uv_handle_get_data((uv_handle_t*) stream); + uv_read_stop(stream); + recv_data->nread = nread; + recv_data->done = 1; +} + +int read_stream_sync(struct uvwasi_s* uvwasi, + uv_stream_t* stream, + recv_data_t* recv_data) { + uv_loop_t* recv_loop; + int r; + + recv_data->nread = 0; + recv_data->done = 0; + recv_loop = uv_handle_get_loop((uv_handle_t*) stream); + + uv_handle_set_data((uv_handle_t*) stream, (void*) recv_data); + r = uv_read_start(stream, recv_alloc_cb, do_stream_recv); + if (r != 0) { + return uvwasi__translate_uv_error(r); + } + + while (!recv_data->done) { + if (uv_run(recv_loop, UV_RUN_ONCE) == 0) { + return UVWASI_ECANCELED; + } + } + + return UVWASI_ESUCCESS; +} diff --git a/deps/uvwasi/src/sync_helpers.h b/deps/uvwasi/src/sync_helpers.h new file mode 100644 index 000000000000000..6ffe2cac133d6c3 --- /dev/null +++ b/deps/uvwasi/src/sync_helpers.h @@ -0,0 +1,27 @@ +#ifndef __UVWASI_SYNC_HELPERS_H__ +#define __UVWASI_SYNC_HELPERS_H__ + +struct uvwasi_s; + +typedef struct shutdown_data_s { + int status; + int done; +} shutdown_data_t; + +typedef struct recv_data_s { + char* base; + size_t len; + ssize_t nread; + int done; +} recv_data_t; + +int free_handle_sync(struct uvwasi_s* uvwasi, uv_handle_t* handle); + +int shutdown_stream_sync(struct uvwasi_s* uvwasi, + uv_stream_t* stream, + shutdown_data_t* shutdown_data); + +int read_stream_sync(struct uvwasi_s* uvwasi, + uv_stream_t* stream, + recv_data_t* recv_data); +#endif /* __UVWASI_SYNC_HELPERS_H__ */ diff --git a/deps/uvwasi/src/uvwasi.c b/deps/uvwasi/src/uvwasi.c index 9e7fc7681664b84..e904b9f9293864d 100644 --- a/deps/uvwasi/src/uvwasi.c +++ b/deps/uvwasi/src/uvwasi.c @@ -25,6 +25,7 @@ #include "clocks.h" #include "path_resolver.h" #include "poll_oneoff.h" +#include "sync_helpers.h" #include "wasi_rights.h" #include "wasi_serdes.h" #include "debug.h" @@ -231,6 +232,13 @@ static uvwasi_errno_t uvwasi__setup_ciovs(const uvwasi_t* uvwasi, return UVWASI_ESUCCESS; } +typedef struct new_connection_data_s { + int done; +} new_connection_data_t; + +void on_new_connection(uv_stream_t *server, int status) { + // just do nothing +} uvwasi_errno_t uvwasi_init(uvwasi_t* uvwasi, const uvwasi_options_t* options) { uv_fs_t realpath_req; @@ -243,11 +251,16 @@ uvwasi_errno_t uvwasi_init(uvwasi_t* uvwasi, const uvwasi_options_t* options) { uvwasi_size_t env_buf_size; uvwasi_size_t i; int r; + struct sockaddr_in addr; if (uvwasi == NULL || options == NULL || options->fd_table_size == 0) return UVWASI_EINVAL; + // loop is only needed if there were pre-open sockets + uvwasi->loop = NULL; + uvwasi->allocator = options->allocator; + if (uvwasi->allocator == NULL) uvwasi->allocator = &default_allocator; @@ -328,6 +341,14 @@ uvwasi_errno_t uvwasi_init(uvwasi_t* uvwasi, const uvwasi_options_t* options) { } } + for (i = 0; i < options->preopen_socketc; ++i) { + if (options->preopen_sockets[i].address == NULL || + options->preopen_sockets[i].port > 65535) { + err = UVWASI_EINVAL; + goto exit; + } + } + err = uvwasi_fd_table_init(uvwasi, options); if (err != UVWASI_ESUCCESS) goto exit; @@ -363,6 +384,36 @@ uvwasi_errno_t uvwasi_init(uvwasi_t* uvwasi, const uvwasi_options_t* options) { goto exit; } + if (options->preopen_socketc > 0) { + uvwasi->loop = uvwasi__malloc(uvwasi, sizeof(uv_loop_t)); + r = uv_loop_init(uvwasi->loop); + if (r != 0) { + err = uvwasi__translate_uv_error(r); + goto exit; + } + } + + for (i = 0; i < options->preopen_socketc; ++i) { + uv_tcp_t* socket = (uv_tcp_t*) malloc(sizeof(uv_tcp_t)); + uv_tcp_init(uvwasi->loop, socket); + + uv_ip4_addr(options->preopen_sockets[i].address, options->preopen_sockets[i].port, &addr); + + uv_tcp_bind(socket, (const struct sockaddr*)&addr, 0); + r = uv_listen((uv_stream_t*) socket, 128, on_new_connection); + if (r != 0) { + err = uvwasi__translate_uv_error(r); + goto exit; + } + + err = uvwasi_fd_table_insert_preopen_socket(uvwasi, + uvwasi->fds, + socket); + + if (err != UVWASI_ESUCCESS) + goto exit; + } + return UVWASI_ESUCCESS; exit: @@ -380,6 +431,12 @@ void uvwasi_destroy(uvwasi_t* uvwasi) { uvwasi__free(uvwasi, uvwasi->argv); uvwasi__free(uvwasi, uvwasi->env_buf); uvwasi__free(uvwasi, uvwasi->env); + if (uvwasi->loop != NULL) { + uv_stop(uvwasi->loop); + uv_loop_close(uvwasi->loop); + uvwasi__free(uvwasi, uvwasi->loop); + uvwasi->loop = NULL; + } uvwasi->fds = NULL; uvwasi->argv_buf = NULL; uvwasi->argv = NULL; @@ -401,6 +458,8 @@ void uvwasi_options_init(uvwasi_options_t* options) { options->envp = NULL; options->preopenc = 0; options->preopens = NULL; + options->preopen_socketc = 0; + options->preopen_sockets = NULL; options->allocator = NULL; } @@ -694,10 +753,9 @@ uvwasi_errno_t uvwasi_fd_allocate(uvwasi_t* uvwasi, return err; } - uvwasi_errno_t uvwasi_fd_close(uvwasi_t* uvwasi, uvwasi_fd_t fd) { struct uvwasi_fd_wrap_t* wrap; - uvwasi_errno_t err; + uvwasi_errno_t err = 0; uv_fs_t req; int r; @@ -712,9 +770,18 @@ uvwasi_errno_t uvwasi_fd_close(uvwasi_t* uvwasi, uvwasi_fd_t fd) { if (err != UVWASI_ESUCCESS) goto exit; - r = uv_fs_close(NULL, &req, wrap->fd, NULL); - uv_mutex_unlock(&wrap->mutex); - uv_fs_req_cleanup(&req); + if (wrap->sock == NULL) { + r = uv_fs_close(NULL, &req, wrap->fd, NULL); + uv_mutex_unlock(&wrap->mutex); + uv_fs_req_cleanup(&req); + } else { + r = 0; + err = free_handle_sync(uvwasi, (uv_handle_t*) wrap->sock); + uv_mutex_unlock(&wrap->mutex); + if (err != UVWASI_ESUCCESS) { + goto exit; + } + } if (r != 0) { err = uvwasi__translate_uv_error(r); @@ -1997,6 +2064,7 @@ uvwasi_errno_t uvwasi_path_open(uvwasi_t* uvwasi, err = uvwasi_fd_table_insert(uvwasi, uvwasi->fds, r, + NULL, resolved_path, resolved_path, filetype, @@ -2520,7 +2588,6 @@ uvwasi_errno_t uvwasi_sched_yield(uvwasi_t* uvwasi) { return UVWASI_ESUCCESS; } - uvwasi_errno_t uvwasi_sock_recv(uvwasi_t* uvwasi, uvwasi_fd_t sock, const uvwasi_iovec_t* ri_data, @@ -2528,10 +2595,50 @@ uvwasi_errno_t uvwasi_sock_recv(uvwasi_t* uvwasi, uvwasi_riflags_t ri_flags, uvwasi_size_t* ro_datalen, uvwasi_roflags_t* ro_flags) { - /* TODO(cjihrig): Waiting to implement, pending - https://github.com/WebAssembly/WASI/issues/4 */ - UVWASI_DEBUG("uvwasi_sock_recv(uvwasi=%p, unimplemented)\n", uvwasi); - return UVWASI_ENOTSUP; + struct uvwasi_fd_wrap_t* wrap; + uvwasi_errno_t err = 0; + recv_data_t recv_data; + + UVWASI_DEBUG("uvwasi_sock_recv(uvwasi=%p, sock=%d, ri_data=%p, " + "ri_data_len=%d, ri_flags=%d, ro_datalen=%p, ro_flags=%p)\n", + uvwasi, + sock, + ri_data, + ri_data_len, + ri_flags, + ro_datalen, + ro_flags); + + if (uvwasi == NULL || ri_data == NULL || ro_datalen == NULL || ro_flags == NULL) + return UVWASI_EINVAL; + + if (ri_flags != 0) + return UVWASI_ENOTSUP; + + err = uvwasi_fd_table_get(uvwasi->fds, + sock, + &wrap, + UVWASI__RIGHTS_SOCKET_BASE, + 0); + if (err != UVWASI_ESUCCESS) + return err; + + recv_data.base = ri_data->buf; + recv_data.len = ri_data->buf_len; + err = read_stream_sync(uvwasi, (uv_stream_t*) wrap->sock, &recv_data); + uv_mutex_unlock(&wrap->mutex); + if (err != 0) { + return err; + } + + if (recv_data.nread == 0) { + return UVWASI_EAGAIN; + } else if (recv_data.nread < 0) { + return uvwasi__translate_uv_error(recv_data.nread); + } + + *ro_datalen = recv_data.nread; + return UVWASI_ESUCCESS; } @@ -2541,30 +2648,195 @@ uvwasi_errno_t uvwasi_sock_send(uvwasi_t* uvwasi, uvwasi_size_t si_data_len, uvwasi_siflags_t si_flags, uvwasi_size_t* so_datalen) { - /* TODO(cjihrig): Waiting to implement, pending - https://github.com/WebAssembly/WASI/issues/4 */ - UVWASI_DEBUG("uvwasi_sock_send(uvwasi=%p, unimplemented)\n", uvwasi); - return UVWASI_ENOTSUP; -} + struct uvwasi_fd_wrap_t* wrap; + uvwasi_errno_t err = 0; + uv_buf_t* bufs; + int r = 0; + + UVWASI_DEBUG("uvwasi_sock_send(uvwasi=%p, sock=%d, si_data=%p, " + "si_data_len=%d, si_flags=%d, so_datalen=%p)\n", + uvwasi, + sock, + si_data, + si_data_len, + si_flags, + so_datalen); + + if (uvwasi == NULL || si_data == NULL || so_datalen == NULL || + si_flags != 0) + return UVWASI_EINVAL; + + err = uvwasi_fd_table_get(uvwasi->fds, + sock, + &wrap, + UVWASI__RIGHTS_SOCKET_BASE, + 0); + if (err != UVWASI_ESUCCESS) + return err; + + err = uvwasi__setup_ciovs(uvwasi, &bufs, si_data, si_data_len); + if (err != UVWASI_ESUCCESS) { + uv_mutex_unlock(&wrap->mutex); + return err; + } + + r = uv_try_write((uv_stream_t*) wrap->sock, bufs, si_data_len); + uvwasi__free(uvwasi, bufs); + uv_mutex_unlock(&wrap->mutex); + if (r < 0) + return uvwasi__translate_uv_error(r); + + *so_datalen = (uvwasi_size_t) r; + return UVWASI_ESUCCESS; +} uvwasi_errno_t uvwasi_sock_shutdown(uvwasi_t* uvwasi, uvwasi_fd_t sock, uvwasi_sdflags_t how) { - /* TODO(cjihrig): Waiting to implement, pending - https://github.com/WebAssembly/WASI/issues/4 */ - UVWASI_DEBUG("uvwasi_sock_shutdown(uvwasi=%p, unimplemented)\n", uvwasi); - return UVWASI_ENOTSUP; + struct uvwasi_fd_wrap_t* wrap; + uvwasi_errno_t err = 0; + shutdown_data_t shutdown_data; + + if (how & ~UVWASI_SHUT_WR) + return UVWASI_ENOTSUP; + + UVWASI_DEBUG("uvwasi_sock_shutdown(uvwasi=%p, sock=%d, how=%d)\n", + uvwasi, + sock, + how); + + if (uvwasi == NULL) + return UVWASI_EINVAL; + + err = uvwasi_fd_table_get(uvwasi->fds, + sock, + &wrap, + UVWASI__RIGHTS_SOCKET_BASE, + 0); + if (err != UVWASI_ESUCCESS) + return err; + + if (how & UVWASI_SHUT_WR) { + err = shutdown_stream_sync(uvwasi, (uv_stream_t*) wrap->sock, &shutdown_data); + if (err != UVWASI_ESUCCESS) { + uv_mutex_unlock(&wrap->mutex); + return err; + } + } + + uv_mutex_unlock(&wrap->mutex); + + if (shutdown_data.status != 0) + return uvwasi__translate_uv_error(shutdown_data.status); + + return UVWASI_ESUCCESS; } uvwasi_errno_t uvwasi_sock_accept(uvwasi_t* uvwasi, uvwasi_fd_t sock, uvwasi_fdflags_t flags, - uvwasi_fd_t* fd) { - /* TODO(mhdawson): Needs implementation */ - UVWASI_DEBUG("uvwasi_sock_accept(uvwasi=%p, unimplemented)\n", uvwasi); - return UVWASI_ENOTSUP; -}; + uvwasi_fd_t* connect_sock) { + struct uvwasi_fd_wrap_t* wrap; + struct uvwasi_fd_wrap_t* connected_wrap; + uvwasi_errno_t err = 0; + uv_loop_t* sock_loop = NULL; + int r = 0; + + UVWASI_DEBUG("uvwasi_sock_accept(uvwasi=%p, sock=%d, flags=%d, " + "connect_sock=%p)\n", + uvwasi, + sock, + flags, + connect_sock); + + if (uvwasi == NULL || connect_sock == NULL) + return UVWASI_EINVAL; + + if (flags & ~UVWASI_FDFLAG_NONBLOCK) + return UVWASI_ENOTSUP; + + err = uvwasi_fd_table_get(uvwasi->fds, + sock, + &wrap, + UVWASI__RIGHTS_SOCKET_BASE, + 0); + if (err != UVWASI_ESUCCESS) + return err; + + sock_loop = uv_handle_get_loop((uv_handle_t*) wrap->sock); + uv_tcp_t* uv_connect_sock = (uv_tcp_t*) uvwasi__malloc(uvwasi, sizeof(uv_tcp_t)); + uv_tcp_init(sock_loop, uv_connect_sock); + + r = uv_accept((uv_stream_t*) wrap->sock, (uv_stream_t*) uv_connect_sock); + if (r != 0) { + if (r == UV_EAGAIN) { + // if not blocking then just return as we have to wait for a connection + if (flags & UVWASI_FDFLAG_NONBLOCK) { + err = free_handle_sync(uvwasi, (uv_handle_t*) uv_connect_sock); + uv_mutex_unlock(&wrap->mutex); + if (err != UVWASI_ESUCCESS) { + return err; + } + return UVWASI_EAGAIN; + } + } else { + err = uvwasi__translate_uv_error(r); + goto close_sock_and_error_exit; + } + + // request was blocking and we have no connection yet. run + // the loop until a connection comes in + while (1) { + err = 0; + if (uv_run(sock_loop, UV_RUN_ONCE) == 0) { + err = UVWASI_ECONNABORTED; + goto close_sock_and_error_exit; + } + + int r = uv_accept((uv_stream_t*) wrap->sock, (uv_stream_t*) uv_connect_sock); + if (r == UV_EAGAIN) { + // still no connection or error so run the loop again + continue; + } + + if (r != 0) { + // An error occurred accepting the connection. Break out of the loop and + // report an error. + err = uvwasi__translate_uv_error(r); + goto close_sock_and_error_exit; + } + + // if we get here a new connection was successfully accepted + break; + } + } + + err = uvwasi_fd_table_insert(uvwasi, + uvwasi->fds, + -1, + uv_connect_sock, + NULL, + NULL, + UVWASI_FILETYPE_SOCKET_STREAM, + UVWASI__RIGHTS_SOCKET_BASE, + UVWASI__RIGHTS_SOCKET_INHERITING, + 1, + &connected_wrap); + + if (err != UVWASI_ESUCCESS) + goto close_sock_and_error_exit; + + *connect_sock = connected_wrap->id; + uv_mutex_unlock(&wrap->mutex); + uv_mutex_unlock(&connected_wrap->mutex); + return UVWASI_ESUCCESS; + +close_sock_and_error_exit: + uvwasi__free(uvwasi, uv_connect_sock); + uv_mutex_unlock(&wrap->mutex); + return err; +} const char* uvwasi_embedder_err_code_to_string(uvwasi_errno_t code) { diff --git a/deps/uvwasi/src/wasi_rights.h b/deps/uvwasi/src/wasi_rights.h index 09009b39889cc05..54ac7d7072b7fe7 100644 --- a/deps/uvwasi/src/wasi_rights.h +++ b/deps/uvwasi/src/wasi_rights.h @@ -84,8 +84,9 @@ UVWASI_RIGHT_FD_WRITE | \ UVWASI_RIGHT_FD_FILESTAT_GET | \ UVWASI_RIGHT_POLL_FD_READWRITE | \ - UVWASI_RIGHT_SOCK_SHUTDOWN) -#define UVWASI__RIGHTS_SOCKET_INHERITING UVWASI__RIGHTS_ALL; + UVWASI_RIGHT_SOCK_SHUTDOWN | \ + UVWASI_RIGHT_SOCK_ACCEPT) +#define UVWASI__RIGHTS_SOCKET_INHERITING UVWASI__RIGHTS_ALL #define UVWASI__RIGHTS_TTY_BASE (UVWASI_RIGHT_FD_READ | \ UVWASI_RIGHT_FD_FDSTAT_SET_FLAGS | \ diff --git a/doc/contributing/maintaining/maintaining-dependencies.md b/doc/contributing/maintaining/maintaining-dependencies.md index 6271f05ce363266..80827fe9eaadbde 100644 --- a/doc/contributing/maintaining/maintaining-dependencies.md +++ b/doc/contributing/maintaining/maintaining-dependencies.md @@ -29,7 +29,7 @@ This a list of all the dependencies: * [postject 1.0.0-alpha.6][] * [simdutf 3.2.17][] * [undici 5.25.2][] -* [uvwasi 0.0.16][] +* [uvwasi 0.0.19][] * [V8 11.3.244.8][] * [zlib 1.2.13.1-motley-f5fd0ad][] @@ -297,7 +297,7 @@ The [undici](https://github.com/nodejs/undici) dependency is an HTTP/1.1 client, written from scratch for Node.js.. See [maintaining-http][] for more informations. -### uvwasi 0.0.16 +### uvwasi 0.0.19 The [uvwasi](https://github.com/nodejs/uvwasi) dependency implements the WASI system call API, so that WebAssembly runtimes can easily @@ -347,6 +347,6 @@ performance improvements not currently available in standard zlib. [simdutf 3.2.17]: #simdutf-3217 [undici 5.25.2]: #undici-5252 [update-openssl-action]: ../../../.github/workflows/update-openssl.yml -[uvwasi 0.0.16]: #uvwasi-0016 +[uvwasi 0.0.19]: #uvwasi-0019 [v8 11.3.244.8]: #v8-1132448 [zlib 1.2.13.1-motley-f5fd0ad]: #zlib-12131-motley-f5fd0ad From d37b0d267f96e4564cad1426a3284bde8f1d1faa Mon Sep 17 00:00:00 2001 From: Michael Dawson Date: Wed, 27 Sep 2023 20:06:12 +0000 Subject: [PATCH 28/31] wasi: updates required for latest uvwasi version MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Michael Dawson PR-URL: https://github.com/nodejs/node/pull/49908 Reviewed-By: Tobias Nießen Reviewed-By: Colin Ihrig Reviewed-By: Luigi Pinca --- deps/uvwasi/uvwasi.gyp | 1 + test/wasi/c/sock.c | 4 ++-- test/wasi/wasm/sock.wasm | Bin 18999 -> 19026 bytes 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/deps/uvwasi/uvwasi.gyp b/deps/uvwasi/uvwasi.gyp index 229dc9f41f8de8b..4dfe3c46d51818f 100644 --- a/deps/uvwasi/uvwasi.gyp +++ b/deps/uvwasi/uvwasi.gyp @@ -5,6 +5,7 @@ 'src/fd_table.c', 'src/path_resolver.c', 'src/poll_oneoff.c', + 'src/sync_helpers.c', 'src/uv_mapping.c', 'src/uvwasi.c', 'src/wasi_rights.c', diff --git a/test/wasi/c/sock.c b/test/wasi/c/sock.c index de4a3ccc5f95a61..d11e86c805784da 100644 --- a/test/wasi/c/sock.c +++ b/test/wasi/c/sock.c @@ -9,9 +9,9 @@ int main(void) { int fd = 0 ; socklen_t addrlen = 0; int flags = 0; - int ret = accept(0, NULL, &addrlen); + int ret = accept(10, NULL, &addrlen); assert(ret == -1); - assert(errno == ENOTSUP); + assert(errno == EBADF); return 0; } diff --git a/test/wasi/wasm/sock.wasm b/test/wasi/wasm/sock.wasm index 78e7b8e430f91141c4d2305da27a6dbc367f5437..9fdba111a23ca2298b62a449a5f8bce92fcc1f80 100755 GIT binary patch delta 2181 zcmZ`(Yit`u5Z*n%b9Q`b9M|z9*>!Bk)JdAARhrO7>XuSSg;EI#0fOpqoJ&aJ_?){7 zX{ELfj~~1W4O_&cDYWzfloX_zf>a3*FBKtv@Rr~Q1tA{dU#kkK{6NfJ9TcQs$+NRF zv){~oJ9}>*!e1W32XCU%-U{A(kE2>T1^M;$k^ZSnzuiK3z2_cL< zZV5SkJ_-5id`R{O+z5fr8D8%Qg(G|xZ-~#&&CMZ#0yromBom}OCJacvAmMg?0S{K{ zsD~lyWg+S#67fyYI#Nev;vrrlD+fd{^@GFTE5VYL{4~HSw4MZLkknI!1c|bpHZV+A zkp{AAnl?&XNMiu8uOAjCL+rG@Hn9K=vm>^?nuJIb36s^zlYm}vePYxr%15K5Ay&Bhn-h^82! zY3L$7K{F(ig1kUVDkujLuXCKnukmw^HT;reAos^3!sAq)5)o1539%qqLUKJ)4vLNP zypZxnjQM$IG>Ql&l5$ems6-r0nW%wF{Lh}8lY@hAb$*7Q=0Vp!$E4{PA^nU6cHVo;RbvW9$MibPePyNe8r!s1tG?<`22g;?sPi zX8<4OGoBAY@AvlfIpMZtKL~=A7wk;8VGIZiWu=cq!XyTaMVoQVYFe-K5uf+A1P(k3 z0TTpgrI7L>cWvsyi`BkOb(mkezR`6|pmDZ&CqB;SH*al80WJ*GqA*j5RTu(>5d%UX z^$~v2yAPk@yL_|cu!sm?m_A_3!9KwgAU}sZ4t5kzD5t7de7|74Q2kzRk?_m>YJh-@ z)(g4GmUr87pDkao<$LuMygvz9bp&t5{s>KwFvAl`OvC(k<$17QR*pI1_GrOaDM#%x zLw+d3m6iIEjqFGMMu_6q`4^!%{1U&lX%gn(-KI2N;M18_u0&j3D6t7TT;NJL4I#bZ z3^;d%xBY+MmGHN=Gr`MQF9cs&odcPO#6b2(64`ge*pgO8rjv~^c1rBn`?jqy1~Z!+ zjxmRwaet(L1)#d-6h2;E+uZ2^=PvR9$or|N81ls6N?~SY7!}ls5Tpnok|u37+%0s& z@>n?W75xq@XOeNyKTW3PAM9GN9sO7GB>0ZDQSg1x*0<^vf$VqI9x99XO5C5CgOCHM z73sQ|YKD_nQzBh;h~V)^<~&&LXm103Z+qrHHJ@qE zg7w?>?xW48%s%p4PIR%{ zSL#SW#N!>Jfp?ph-Smn#kRZ)APO5Wx>RhUJTO>}R;=dyX6N!cbNM2%jpjWDX)_I-e zepRrK_?E5^4|Wei(mT78f`IP!o+S_$o0Do^9yyNKZ}wdtg?ERP6ZTXreaU{Up55>e z#)H+q-Y$%HR>wB3$JO@rk7C|&eHCBkZ}t7m7y1uIqxRc|7mbxO0w4)5dlFu+6yslR z?#)`7X|0{jo8xN>YfZf{*;PP8YzPewZAY46lyovMKyKX1wpB9&pS#a)n|4<6!@6Mw z1FD)gP0g^>(foK(agEGOP3>uDP*uR1QVaQFQ5~^#!(8PVEgRz{YqZf-DC(vbf`g_t zGOnxkw#hwfj9c1$;eXBceziwci>9VcM*K_RV%J=!sug{-s1~8p=)W#)_qY}Fsj8*h zhvT4*+S=}f`J-n-l0cDIjxc4oWVZo8#cC{L+ym51=7YLuuk=Ca-1V%u%+_U@K~ zfLk9iKCw6ike9p)h=|esVIU+LF@nesqlg+};v+_kG4jWR@XIr^r6A#M_S`e)oH^(F z&dfdEY{egM#S_b^t;io-Ty23DT3ueE)zZcy-YA{Gwpv!Tymrg82Z`P8LOqi35Iay2pfVvG zC#)CnTWo`E5+t>MVBHE<_K7ou53tM5kMLo(+0_O51=ktSC)_ieR$*L}7ON;Jo#sSi z7-$?4l(9Oc&5=&C>+WdB3$H@J0KrMgue`^r$#K{WSl{IfDyc;k0Ue}bkXYHyTD_a_ zE_T#A1{Fy~6z?i6s`wq_ZKZ?~mGEx1q?&-#g?vfKBSM}N@=qb%J_<2yT$Z|h4`5HN ziU(qt*1};#*a81euy@t$w}gR#I4Wg_K;!pdnb?xjyqHhD#IDyP%UA>!dgs%IOo5tQNBp)7(DWWj|kAo`iiFo_~8;D84EZ z$0th9L}oa^c{Kh!$h~cx37`VGDG>mI zw)@#r){5qx+)gE8QI&zI2_?x+q<+drg#3a9TVQ%9``8ux5dNH1I9E9&F-(Nc9L`g0 z+}dhA2w6(D`LARAM~TdS5lbUWOOtcjF&-&hoI4ALgJO=r^uWqdUN8nzWRSoZ3CaJm zrT6@1fzOB~7f!G(3rba17>}|C1VkU>(Jbxp{($z(lc0c&&BdZ=l-D)>l(Ov53`r2+q@RY9KKV~iVjt&j2udLFv zv2=b&%cQeet+!y9dBv9QG0Z}hSJTq@ylxh>zI1=K+CFCX7xcA0Fy{-s{e~vieut)I z^SVA*#xV7; zBWL8aw7Di?$)$UvfvhpSM(Z_3d$PJVY!tNoXf6l%^xg)?)OO<5G@~!8Wr07P#-Xq4 zFT?;x!4N Date: Sat, 30 Sep 2023 03:17:00 +0200 Subject: [PATCH 29/31] fs: throw errors from sync branches instead of separate implementations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously to throw errors from C++ land, sync versions of the fs were created by copying C++ code from the original implementation and moving JS code to a separate file. This can lead to several problems: 1. By moving code to a new file for the sake of moving, it would be harder to use git blame to trace changes and harder to backport changes to older branches. 2. Scattering the async and sync versions of fs methods in different files makes it harder to keep them in sync and share code in the prologues and epilogues. 3. Having two copies of code doing almost the same thing results in duplication and can be prone to out-of-sync problems when the prologue and epilogue get updated. 4. There is a minor cost to startup in adding an additional file. This can add up even with the help of snapshots. This patch moves the JS code back to lib/fs.js to stop 1, 2 & 4 and introduces C++ helpers SyncCallAndThrowIf() and SyncCallAndThrowOnError() so that the original implementations can be easily tweaked to allow throwing from C++ and stop 3. PR-URL: https://github.com/nodejs/node/pull/49913 Reviewed-By: Stephen Belanger Reviewed-By: Colin Ihrig Reviewed-By: Darshan Sen Reviewed-By: Benjamin Gruenbaum Reviewed-By: Tobias Nießen Reviewed-By: Yagiz Nizipli --- lib/fs.js | 66 ++++- lib/internal/fs/sync.js | 106 -------- src/node_file-inl.h | 32 +++ src/node_file.cc | 323 +++++------------------- src/node_file.h | 29 ++- test/parallel/test-bootstrap-modules.js | 1 - 6 files changed, 173 insertions(+), 384 deletions(-) delete mode 100644 lib/internal/fs/sync.js diff --git a/lib/fs.js b/lib/fs.js index 3931216e9522f11..d682759d8b0a141 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -141,7 +141,6 @@ const { validateObject, validateString, } = require('internal/validators'); -const syncFs = require('internal/fs/sync'); let truncateWarn = true; let fs; @@ -243,7 +242,10 @@ function access(path, mode, callback) { * @returns {void} */ function accessSync(path, mode) { - syncFs.access(path, mode); + path = getValidatedPath(path); + mode = getValidMode(mode, 'access'); + + binding.access(pathModule.toNamespacedPath(path), mode); } /** @@ -285,7 +287,13 @@ ObjectDefineProperty(exists, kCustomPromisifiedSymbol, { * @returns {boolean} */ function existsSync(path) { - return syncFs.exists(path); + try { + path = getValidatedPath(path); + } catch { + return false; + } + + return binding.existsSync(pathModule.toNamespacedPath(path)); } function readFileAfterOpen(err, fd) { @@ -438,7 +446,10 @@ function readFileSync(path, options) { options = getOptions(options, { flag: 'r' }); if (options.encoding === 'utf8' || options.encoding === 'utf-8') { - return syncFs.readFileUtf8(path, options.flag); + if (!isInt32(path)) { + path = pathModule.toNamespacedPath(getValidatedPath(path)); + } + return binding.readFileUtf8(path, stringToFlags(options.flag)); } const isUserFd = isFd(path); // File descriptor ownership @@ -516,7 +527,9 @@ function close(fd, callback = defaultCloseCallback) { * @returns {void} */ function closeSync(fd) { - return syncFs.close(fd); + fd = getValidatedFd(fd); + + return binding.close(fd); } /** @@ -562,7 +575,13 @@ function open(path, flags, mode, callback) { * @returns {number} */ function openSync(path, flags, mode) { - return syncFs.open(path, flags, mode); + path = getValidatedPath(path); + + return binding.open( + pathModule.toNamespacedPath(path), + stringToFlags(flags), + parseFileMode(mode, 'mode', 0o666), + ); } /** @@ -1667,12 +1686,24 @@ function lstatSync(path, options = { bigint: false, throwIfNoEntry: true }) { * }} [options] * @returns {Stats} */ -function statSync(path, options) { - return syncFs.stat(path, options); +function statSync(path, options = { bigint: false, throwIfNoEntry: true }) { + path = getValidatedPath(path); + const stats = binding.stat( + pathModule.toNamespacedPath(path), + options.bigint, + undefined, + options.throwIfNoEntry, + ); + if (stats === undefined) { + return undefined; + } + return getStatsFromBinding(stats); } -function statfsSync(path, options) { - return syncFs.statfs(path, options); +function statfsSync(path, options = { bigint: false }) { + path = getValidatedPath(path); + const stats = binding.statfs(pathModule.toNamespacedPath(path), options.bigint); + return getStatFsFromBinding(stats); } /** @@ -1852,7 +1883,8 @@ function unlink(path, callback) { * @returns {void} */ function unlinkSync(path) { - return syncFs.unlink(path); + path = pathModule.toNamespacedPath(getValidatedPath(path)); + return binding.unlink(path); } /** @@ -2652,8 +2684,7 @@ function realpathSync(p, options) { } if (linkTarget === null) { const ctx = { path: base }; - binding.stat(baseLong, false, undefined, ctx); - handleErrorFromBinding(ctx); + binding.stat(baseLong, false, undefined, true); linkTarget = binding.readlink(baseLong, undefined, undefined, ctx); handleErrorFromBinding(ctx); } @@ -2948,7 +2979,14 @@ function copyFile(src, dest, mode, callback) { * @returns {void} */ function copyFileSync(src, dest, mode) { - syncFs.copyFile(src, dest, mode); + src = getValidatedPath(src, 'src'); + dest = getValidatedPath(dest, 'dest'); + + binding.copyFile( + pathModule.toNamespacedPath(src), + pathModule.toNamespacedPath(dest), + getValidMode(mode, 'copyFile'), + ); } /** diff --git a/lib/internal/fs/sync.js b/lib/internal/fs/sync.js deleted file mode 100644 index fbcc2ad2e25b2ac..000000000000000 --- a/lib/internal/fs/sync.js +++ /dev/null @@ -1,106 +0,0 @@ -'use strict'; - -const pathModule = require('path'); -const { - getValidatedPath, - stringToFlags, - getValidMode, - getStatsFromBinding, - getStatFsFromBinding, - getValidatedFd, -} = require('internal/fs/utils'); -const { parseFileMode, isInt32 } = require('internal/validators'); - -const binding = internalBinding('fs'); - -/** - * @param {string} path - * @param {number} flag - * @return {string} - */ -function readFileUtf8(path, flag) { - if (!isInt32(path)) { - path = pathModule.toNamespacedPath(getValidatedPath(path)); - } - return binding.readFileUtf8(path, stringToFlags(flag)); -} - -function exists(path) { - try { - path = getValidatedPath(path); - } catch { - return false; - } - - return binding.existsSync(pathModule.toNamespacedPath(path)); -} - -function access(path, mode) { - path = getValidatedPath(path); - mode = getValidMode(mode, 'access'); - - binding.accessSync(pathModule.toNamespacedPath(path), mode); -} - -function copyFile(src, dest, mode) { - src = getValidatedPath(src, 'src'); - dest = getValidatedPath(dest, 'dest'); - - binding.copyFileSync( - pathModule.toNamespacedPath(src), - pathModule.toNamespacedPath(dest), - getValidMode(mode, 'copyFile'), - ); -} - -function stat(path, options = { bigint: false, throwIfNoEntry: true }) { - path = getValidatedPath(path); - const stats = binding.statSync( - pathModule.toNamespacedPath(path), - options.bigint, - options.throwIfNoEntry, - ); - if (stats === undefined) { - return undefined; - } - return getStatsFromBinding(stats); -} - -function statfs(path, options = { bigint: false }) { - path = getValidatedPath(path); - const stats = binding.statfsSync(pathModule.toNamespacedPath(path), options.bigint); - return getStatFsFromBinding(stats); -} - -function open(path, flags, mode) { - path = getValidatedPath(path); - - return binding.openSync( - pathModule.toNamespacedPath(path), - stringToFlags(flags), - parseFileMode(mode, 'mode', 0o666), - ); -} - -function close(fd) { - fd = getValidatedFd(fd); - - return binding.closeSync(fd); -} - -function unlink(path) { - path = pathModule.toNamespacedPath(getValidatedPath(path)); - return binding.unlinkSync(path); -} - -module.exports = { - readFileUtf8, - exists, - access, - copyFile, - stat, - statfs, - open, - close, - unlink, -}; diff --git a/src/node_file-inl.h b/src/node_file-inl.h index cdf21a4b3a6c228..6c059add3bfc024 100644 --- a/src/node_file-inl.h +++ b/src/node_file-inl.h @@ -349,6 +349,38 @@ int SyncCall(Environment* env, v8::Local ctx, return err; } +// Similar to SyncCall but throws immediately if there is an error. +template +int SyncCallAndThrowIf(Predicate should_throw, + Environment* env, + FSReqWrapSync* req_wrap, + Func fn, + Args... args) { + env->PrintSyncTrace(); + int result = fn(nullptr, &(req_wrap->req), args..., nullptr); + if (should_throw(result)) { + env->ThrowUVException(result, + req_wrap->syscall_p, + nullptr, + req_wrap->path_p, + req_wrap->dest_p); + } + return result; +} + +constexpr bool is_uv_error(int result) { + return result < 0; +} + +// Similar to SyncCall but throws immediately if there is an error. +template +int SyncCallAndThrowOnError(Environment* env, + FSReqWrapSync* req_wrap, + Func fn, + Args... args) { + return SyncCallAndThrowIf(is_uv_error, env, req_wrap, fn, args...); +} + } // namespace fs } // namespace node diff --git a/src/node_file.cc b/src/node_file.cc index 0e03f4b1f4bd2de..9430aff5c29a37d 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -981,90 +981,45 @@ void Access(const FunctionCallbackInfo& args) { THROW_IF_INSUFFICIENT_PERMISSIONS( env, permission::PermissionScope::kFileSystemRead, path.ToStringView()); - FSReqBase* req_wrap_async = GetReqWrap(args, 2); - if (req_wrap_async != nullptr) { // access(path, mode, req) + if (argc > 2) { // access(path, mode, req) + FSReqBase* req_wrap_async = GetReqWrap(args, 2); + CHECK_NOT_NULL(req_wrap_async); FS_ASYNC_TRACE_BEGIN1( UV_FS_ACCESS, req_wrap_async, "path", TRACE_STR_COPY(*path)) AsyncCall(env, req_wrap_async, args, "access", UTF8, AfterNoArgs, uv_fs_access, *path, mode); - } else { // access(path, mode, undefined, ctx) - CHECK_EQ(argc, 4); - FSReqWrapSync req_wrap_sync; + } else { // access(path, mode) + FSReqWrapSync req_wrap_sync("access", *path); FS_SYNC_TRACE_BEGIN(access); - SyncCall(env, args[3], &req_wrap_sync, "access", uv_fs_access, *path, mode); + SyncCallAndThrowOnError(env, &req_wrap_sync, uv_fs_access, *path, mode); FS_SYNC_TRACE_END(access); } } -static void AccessSync(const FunctionCallbackInfo& args) { - Environment* env = Environment::GetCurrent(args); - Isolate* isolate = env->isolate(); - - const int argc = args.Length(); - CHECK_GE(argc, 2); - - CHECK(args[1]->IsInt32()); - int mode = args[1].As()->Value(); - - BufferValue path(isolate, args[0]); - CHECK_NOT_NULL(*path); - THROW_IF_INSUFFICIENT_PERMISSIONS( - env, permission::PermissionScope::kFileSystemRead, path.ToStringView()); - - uv_fs_t req; - FS_SYNC_TRACE_BEGIN(access); - int err = uv_fs_access(nullptr, &req, *path, mode, nullptr); - uv_fs_req_cleanup(&req); - FS_SYNC_TRACE_END(access); - - if (err) { - return env->ThrowUVException(err, "access", nullptr, path.out()); - } -} - void Close(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); const int argc = args.Length(); - CHECK_GE(argc, 2); + CHECK_GE(argc, 1); CHECK(args[0]->IsInt32()); int fd = args[0].As()->Value(); env->RemoveUnmanagedFd(fd); - FSReqBase* req_wrap_async = GetReqWrap(args, 1); - if (req_wrap_async != nullptr) { // close(fd, req) + if (argc > 1) { // close(fd, req) + FSReqBase* req_wrap_async = GetReqWrap(args, 1); + CHECK_NOT_NULL(req_wrap_async); FS_ASYNC_TRACE_BEGIN0(UV_FS_CLOSE, req_wrap_async) AsyncCall(env, req_wrap_async, args, "close", UTF8, AfterNoArgs, uv_fs_close, fd); - } else { // close(fd, undefined, ctx) - CHECK_EQ(argc, 3); - FSReqWrapSync req_wrap_sync; + } else { // close(fd) + FSReqWrapSync req_wrap_sync("close"); FS_SYNC_TRACE_BEGIN(close); - SyncCall(env, args[2], &req_wrap_sync, "close", uv_fs_close, fd); + SyncCallAndThrowOnError(env, &req_wrap_sync, uv_fs_close, fd); FS_SYNC_TRACE_END(close); } } -static void CloseSync(const FunctionCallbackInfo& args) { - Environment* env = Environment::GetCurrent(args); - CHECK_GE(args.Length(), 1); - CHECK(args[0]->IsInt32()); - - int fd = args[0].As()->Value(); - env->RemoveUnmanagedFd(fd); - - uv_fs_t req; - FS_SYNC_TRACE_BEGIN(close); - int err = uv_fs_close(nullptr, &req, fd, nullptr); - FS_SYNC_TRACE_END(close); - uv_fs_req_cleanup(&req); - - if (err < 0) { - return env->ThrowUVException(err, "close"); - } -} - static void ExistsSync(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); Isolate* isolate = env->isolate(); @@ -1179,13 +1134,17 @@ static void InternalModuleStat(const FunctionCallbackInfo& args) { args.GetReturnValue().Set(rc); } +constexpr bool is_uv_error_except_no_entry(int result) { + return result < 0 && result != UV_ENOENT; +} + static void Stat(const FunctionCallbackInfo& args) { Realm* realm = Realm::GetCurrent(args); BindingData* binding_data = realm->GetBindingData(); Environment* env = realm->env(); const int argc = args.Length(); - CHECK_GE(argc, 2); + CHECK_GE(argc, 3); BufferValue path(realm->isolate(), args[0]); CHECK_NOT_NULL(*path); @@ -1193,63 +1152,34 @@ static void Stat(const FunctionCallbackInfo& args) { env, permission::PermissionScope::kFileSystemRead, path.ToStringView()); bool use_bigint = args[1]->IsTrue(); - FSReqBase* req_wrap_async = GetReqWrap(args, 2, use_bigint); - if (req_wrap_async != nullptr) { // stat(path, use_bigint, req) + if (!args[2]->IsUndefined()) { // stat(path, use_bigint, req) + FSReqBase* req_wrap_async = GetReqWrap(args, 2, use_bigint); + CHECK_NOT_NULL(req_wrap_async); FS_ASYNC_TRACE_BEGIN1( UV_FS_STAT, req_wrap_async, "path", TRACE_STR_COPY(*path)) AsyncCall(env, req_wrap_async, args, "stat", UTF8, AfterStat, uv_fs_stat, *path); - } else { // stat(path, use_bigint, undefined, ctx) - CHECK_EQ(argc, 4); - FSReqWrapSync req_wrap_sync; + } else { // stat(path, use_bigint, undefined, do_not_throw_if_no_entry) + bool do_not_throw_if_no_entry = args[3]->IsFalse(); + FSReqWrapSync req_wrap_sync("stat", *path); FS_SYNC_TRACE_BEGIN(stat); - int err = SyncCall(env, args[3], &req_wrap_sync, "stat", uv_fs_stat, *path); + int result; + if (do_not_throw_if_no_entry) { + result = SyncCallAndThrowIf( + is_uv_error_except_no_entry, env, &req_wrap_sync, uv_fs_stat, *path); + } else { + result = SyncCallAndThrowOnError(env, &req_wrap_sync, uv_fs_stat, *path); + } FS_SYNC_TRACE_END(stat); - if (err != 0) { - return; // error info is in ctx + if (is_uv_error(result)) { + return; } - Local arr = FillGlobalStatsArray(binding_data, use_bigint, static_cast(req_wrap_sync.req.ptr)); args.GetReturnValue().Set(arr); } } -static void StatSync(const FunctionCallbackInfo& args) { - Realm* realm = Realm::GetCurrent(args); - BindingData* binding_data = realm->GetBindingData(); - Environment* env = realm->env(); - - CHECK_GE(args.Length(), 3); - - BufferValue path(realm->isolate(), args[0]); - bool use_bigint = args[1]->IsTrue(); - bool do_not_throw_if_no_entry = args[2]->IsFalse(); - CHECK_NOT_NULL(*path); - THROW_IF_INSUFFICIENT_PERMISSIONS( - env, permission::PermissionScope::kFileSystemRead, path.ToStringView()); - - env->PrintSyncTrace(); - - uv_fs_t req; - auto make = OnScopeLeave([&req]() { uv_fs_req_cleanup(&req); }); - - FS_SYNC_TRACE_BEGIN(stat); - int err = uv_fs_stat(nullptr, &req, *path, nullptr); - FS_SYNC_TRACE_END(stat); - - if (err < 0) { - if (err == UV_ENOENT && do_not_throw_if_no_entry) { - return; - } - return env->ThrowUVException(err, "stat", nullptr, path.out()); - } - - Local arr = FillGlobalStatsArray( - binding_data, use_bigint, static_cast(req.ptr)); - args.GetReturnValue().Set(arr); -} - static void LStat(const FunctionCallbackInfo& args) { Realm* realm = Realm::GetCurrent(args); BindingData* binding_data = realm->GetBindingData(); @@ -1332,8 +1262,9 @@ static void StatFs(const FunctionCallbackInfo& args) { env, permission::PermissionScope::kFileSystemRead, path.ToStringView()); bool use_bigint = args[1]->IsTrue(); - FSReqBase* req_wrap_async = GetReqWrap(args, 2, use_bigint); - if (req_wrap_async != nullptr) { // statfs(path, use_bigint, req) + if (argc > 2) { // statfs(path, use_bigint, req) + FSReqBase* req_wrap_async = GetReqWrap(args, 2, use_bigint); + CHECK_NOT_NULL(req_wrap_async); FS_ASYNC_TRACE_BEGIN1( UV_FS_STATFS, req_wrap_async, "path", TRACE_STR_COPY(*path)) AsyncCall(env, @@ -1344,15 +1275,14 @@ static void StatFs(const FunctionCallbackInfo& args) { AfterStatFs, uv_fs_statfs, *path); - } else { // statfs(path, use_bigint, undefined, ctx) - CHECK_EQ(argc, 4); - FSReqWrapSync req_wrap_sync; + } else { // statfs(path, use_bigint) + FSReqWrapSync req_wrap_sync("statfs", *path); FS_SYNC_TRACE_BEGIN(statfs); - int err = - SyncCall(env, args[3], &req_wrap_sync, "statfs", uv_fs_statfs, *path); + int result = + SyncCallAndThrowOnError(env, &req_wrap_sync, uv_fs_statfs, *path); FS_SYNC_TRACE_END(statfs); - if (err != 0) { - return; // error info is in ctx + if (is_uv_error(result)) { + return; } Local arr = FillGlobalStatFsArray( @@ -1363,34 +1293,6 @@ static void StatFs(const FunctionCallbackInfo& args) { } } -static void StatFsSync(const FunctionCallbackInfo& args) { - Realm* realm = Realm::GetCurrent(args); - BindingData* binding_data = realm->GetBindingData(); - Environment* env = realm->env(); - - CHECK_GE(args.Length(), 2); - - BufferValue path(realm->isolate(), args[0]); - bool use_bigint = args[1]->IsTrue(); - - CHECK_NOT_NULL(*path); - THROW_IF_INSUFFICIENT_PERMISSIONS( - env, permission::PermissionScope::kFileSystemRead, path.ToStringView()); - - uv_fs_t req; - auto make = OnScopeLeave([&req]() { uv_fs_req_cleanup(&req); }); - FS_SYNC_TRACE_BEGIN(statfs); - int err = uv_fs_statfs(nullptr, &req, *path, nullptr); - FS_SYNC_TRACE_END(statfs); - if (err < 0) { - return env->ThrowUVException(err, "statfs", *path, nullptr); - } - - Local arr = FillGlobalStatFsArray( - binding_data, use_bigint, static_cast(req.ptr)); - args.GetReturnValue().Set(arr); -} - static void Symlink(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); Isolate* isolate = env->isolate(); @@ -1645,49 +1547,28 @@ static void Unlink(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); const int argc = args.Length(); - CHECK_GE(argc, 2); + CHECK_GE(argc, 1); BufferValue path(env->isolate(), args[0]); CHECK_NOT_NULL(*path); THROW_IF_INSUFFICIENT_PERMISSIONS( env, permission::PermissionScope::kFileSystemWrite, path.ToStringView()); - FSReqBase* req_wrap_async = GetReqWrap(args, 1); - if (req_wrap_async != nullptr) { + if (argc > 1) { // unlink(path, req) + FSReqBase* req_wrap_async = GetReqWrap(args, 1); + CHECK_NOT_NULL(req_wrap_async); FS_ASYNC_TRACE_BEGIN1( UV_FS_UNLINK, req_wrap_async, "path", TRACE_STR_COPY(*path)) AsyncCall(env, req_wrap_async, args, "unlink", UTF8, AfterNoArgs, uv_fs_unlink, *path); - } else { - CHECK_EQ(argc, 3); - FSReqWrapSync req_wrap_sync; + } else { // unlink(path) + FSReqWrapSync req_wrap_sync("unlink", *path); FS_SYNC_TRACE_BEGIN(unlink); - SyncCall(env, args[2], &req_wrap_sync, "unlink", uv_fs_unlink, *path); + SyncCallAndThrowOnError(env, &req_wrap_sync, uv_fs_unlink, *path); FS_SYNC_TRACE_END(unlink); } } -static void UnlinkSync(const FunctionCallbackInfo& args) { - Environment* env = Environment::GetCurrent(args); - - const int argc = args.Length(); - CHECK_GE(argc, 1); - - BufferValue path(env->isolate(), args[0]); - CHECK_NOT_NULL(*path); - THROW_IF_INSUFFICIENT_PERMISSIONS( - env, permission::PermissionScope::kFileSystemWrite, path.ToStringView()); - - uv_fs_t req; - auto make = OnScopeLeave([&req]() { uv_fs_req_cleanup(&req); }); - FS_SYNC_TRACE_BEGIN(unlink); - int err = uv_fs_unlink(nullptr, &req, *path, nullptr); - FS_SYNC_TRACE_END(unlink); - if (err < 0) { - return env->ThrowUVException(err, "unlink", nullptr, *path); - } -} - static void RMDir(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); @@ -2137,54 +2018,26 @@ static void Open(const FunctionCallbackInfo& args) { if (CheckOpenPermissions(env, path, flags).IsNothing()) return; - FSReqBase* req_wrap_async = GetReqWrap(args, 3); - if (req_wrap_async != nullptr) { // open(path, flags, mode, req) + if (argc > 3) { // open(path, flags, mode, req) + FSReqBase* req_wrap_async = GetReqWrap(args, 3); + CHECK_NOT_NULL(req_wrap_async); req_wrap_async->set_is_plain_open(true); FS_ASYNC_TRACE_BEGIN1( UV_FS_OPEN, req_wrap_async, "path", TRACE_STR_COPY(*path)) AsyncCall(env, req_wrap_async, args, "open", UTF8, AfterInteger, uv_fs_open, *path, flags, mode); - } else { // open(path, flags, mode, undefined, ctx) - CHECK_EQ(argc, 5); - FSReqWrapSync req_wrap_sync; + } else { // open(path, flags, mode) + FSReqWrapSync req_wrap_sync("open", *path); FS_SYNC_TRACE_BEGIN(open); - int result = SyncCall(env, args[4], &req_wrap_sync, "open", - uv_fs_open, *path, flags, mode); + int result = SyncCallAndThrowOnError( + env, &req_wrap_sync, uv_fs_open, *path, flags, mode); FS_SYNC_TRACE_END(open); - if (result >= 0) env->AddUnmanagedFd(result); + if (is_uv_error(result)) return; + env->AddUnmanagedFd(result); args.GetReturnValue().Set(result); } } -static void OpenSync(const FunctionCallbackInfo& args) { - Environment* env = Environment::GetCurrent(args); - - const int argc = args.Length(); - CHECK_GE(argc, 3); - - BufferValue path(env->isolate(), args[0]); - CHECK_NOT_NULL(*path); - - CHECK(args[1]->IsInt32()); - const int flags = args[1].As()->Value(); - - CHECK(args[2]->IsInt32()); - const int mode = args[2].As()->Value(); - - if (CheckOpenPermissions(env, path, flags).IsNothing()) return; - - uv_fs_t req; - auto make = OnScopeLeave([&req]() { uv_fs_req_cleanup(&req); }); - FS_SYNC_TRACE_BEGIN(open); - auto err = uv_fs_open(nullptr, &req, *path, flags, mode, nullptr); - FS_SYNC_TRACE_END(open); - if (err < 0) { - return env->ThrowUVException(err, "open", nullptr, path.out()); - } - env->AddUnmanagedFd(err); - args.GetReturnValue().Set(err); -} - static void OpenFileHandle(const FunctionCallbackInfo& args) { Realm* realm = Realm::GetCurrent(args); BindingData* binding_data = realm->GetBindingData(); @@ -2246,8 +2099,8 @@ static void CopyFile(const FunctionCallbackInfo& args) { CHECK(args[2]->IsInt32()); const int flags = args[2].As()->Value(); - FSReqBase* req_wrap_async = GetReqWrap(args, 3); - if (req_wrap_async != nullptr) { // copyFile(src, dest, flags, req) + if (argc > 3) { // copyFile(src, dest, flags, req) + FSReqBase* req_wrap_async = GetReqWrap(args, 3); FS_ASYNC_TRACE_BEGIN2(UV_FS_COPYFILE, req_wrap_async, "src", @@ -2257,49 +2110,15 @@ static void CopyFile(const FunctionCallbackInfo& args) { AsyncDestCall(env, req_wrap_async, args, "copyfile", *dest, dest.length(), UTF8, AfterNoArgs, uv_fs_copyfile, *src, *dest, flags); - } else { // copyFile(src, dest, flags, undefined, ctx) - CHECK_EQ(argc, 5); - FSReqWrapSync req_wrap_sync; + } else { // copyFile(src, dest, flags) + FSReqWrapSync req_wrap_sync("copyfile", *src, *dest); FS_SYNC_TRACE_BEGIN(copyfile); - SyncCall(env, args[4], &req_wrap_sync, "copyfile", - uv_fs_copyfile, *src, *dest, flags); + SyncCallAndThrowOnError( + env, &req_wrap_sync, uv_fs_copyfile, *src, *dest, flags); FS_SYNC_TRACE_END(copyfile); } } -static void CopyFileSync(const FunctionCallbackInfo& args) { - Environment* env = Environment::GetCurrent(args); - Isolate* isolate = env->isolate(); - - const int argc = args.Length(); - CHECK_GE(argc, 3); - - BufferValue src(isolate, args[0]); - CHECK_NOT_NULL(*src); - THROW_IF_INSUFFICIENT_PERMISSIONS( - env, permission::PermissionScope::kFileSystemRead, src.ToStringView()); - - BufferValue dest(isolate, args[1]); - CHECK_NOT_NULL(*dest); - THROW_IF_INSUFFICIENT_PERMISSIONS( - env, permission::PermissionScope::kFileSystemWrite, dest.ToStringView()); - - CHECK(args[2]->IsInt32()); - const int flags = args[2].As()->Value(); - - uv_fs_t req; - FS_SYNC_TRACE_BEGIN(copyfile); - int err = - uv_fs_copyfile(nullptr, &req, src.out(), dest.out(), flags, nullptr); - uv_fs_req_cleanup(&req); - FS_SYNC_TRACE_END(copyfile); - - if (err) { - return env->ThrowUVException( - err, "copyfile", nullptr, src.out(), dest.out()); - } -} - // Wrapper for write(2). // // bytesWritten = write(fd, buffer, offset, length, position, callback) @@ -3391,12 +3210,9 @@ static void CreatePerIsolateProperties(IsolateData* isolate_data, Isolate* isolate = isolate_data->isolate(); SetMethod(isolate, target, "access", Access); - SetMethod(isolate, target, "accessSync", AccessSync); SetMethod(isolate, target, "close", Close); - SetMethod(isolate, target, "closeSync", CloseSync); SetMethod(isolate, target, "existsSync", ExistsSync); SetMethod(isolate, target, "open", Open); - SetMethod(isolate, target, "openSync", OpenSync); SetMethod(isolate, target, "openFileHandle", OpenFileHandle); SetMethod(isolate, target, "read", Read); SetMethod(isolate, target, "readFileUtf8", ReadFileUtf8); @@ -3411,22 +3227,18 @@ static void CreatePerIsolateProperties(IsolateData* isolate_data, SetMethod(isolate, target, "internalModuleReadJSON", InternalModuleReadJSON); SetMethod(isolate, target, "internalModuleStat", InternalModuleStat); SetMethod(isolate, target, "stat", Stat); - SetMethod(isolate, target, "statSync", StatSync); SetMethod(isolate, target, "lstat", LStat); SetMethod(isolate, target, "fstat", FStat); SetMethod(isolate, target, "statfs", StatFs); - SetMethod(isolate, target, "statfsSync", StatFsSync); SetMethod(isolate, target, "link", Link); SetMethod(isolate, target, "symlink", Symlink); SetMethod(isolate, target, "readlink", ReadLink); SetMethod(isolate, target, "unlink", Unlink); - SetMethod(isolate, target, "unlinkSync", UnlinkSync); SetMethod(isolate, target, "writeBuffer", WriteBuffer); SetMethod(isolate, target, "writeBuffers", WriteBuffers); SetMethod(isolate, target, "writeString", WriteString); SetMethod(isolate, target, "realpath", RealPath); SetMethod(isolate, target, "copyFile", CopyFile); - SetMethod(isolate, target, "copyFileSync", CopyFileSync); SetMethod(isolate, target, "chmod", Chmod); SetMethod(isolate, target, "fchmod", FChmod); @@ -3514,15 +3326,12 @@ BindingData* FSReqBase::binding_data() { void RegisterExternalReferences(ExternalReferenceRegistry* registry) { registry->Register(Access); - registry->Register(AccessSync); StatWatcher::RegisterExternalReferences(registry); BindingData::RegisterExternalReferences(registry); registry->Register(Close); - registry->Register(CloseSync); registry->Register(ExistsSync); registry->Register(Open); - registry->Register(OpenSync); registry->Register(OpenFileHandle); registry->Register(Read); registry->Register(ReadFileUtf8); @@ -3537,22 +3346,18 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) { registry->Register(InternalModuleReadJSON); registry->Register(InternalModuleStat); registry->Register(Stat); - registry->Register(StatSync); registry->Register(LStat); registry->Register(FStat); registry->Register(StatFs); - registry->Register(StatFsSync); registry->Register(Link); registry->Register(Symlink); registry->Register(ReadLink); registry->Register(Unlink); - registry->Register(UnlinkSync); registry->Register(WriteBuffer); registry->Register(WriteBuffers); registry->Register(WriteString); registry->Register(RealPath); registry->Register(CopyFile); - registry->Register(CopyFileSync); registry->Register(Chmod); registry->Register(FChmod); diff --git a/src/node_file.h b/src/node_file.h index f608da5b7a17e21..bdad1ae25f4892c 100644 --- a/src/node_file.h +++ b/src/node_file.h @@ -457,10 +457,22 @@ int MKDirpSync(uv_loop_t* loop, class FSReqWrapSync { public: - FSReqWrapSync() = default; + FSReqWrapSync(const char* syscall = nullptr, + const char* path = nullptr, + const char* dest = nullptr) + : syscall_p(syscall), path_p(path), dest_p(dest) {} ~FSReqWrapSync() { uv_fs_req_cleanup(&req); } + uv_fs_t req; + const char* syscall_p; + const char* path_p; + const char* dest_p; + + FSReqWrapSync(const FSReqWrapSync&) = delete; + FSReqWrapSync& operator=(const FSReqWrapSync&) = delete; + // TODO(joyeecheung): move these out of FSReqWrapSync and into a special + // class for mkdirp FSContinuationData* continuation_data() const { return continuation_data_.get(); } @@ -468,9 +480,6 @@ class FSReqWrapSync { continuation_data_ = std::move(data); } - FSReqWrapSync(const FSReqWrapSync&) = delete; - FSReqWrapSync& operator=(const FSReqWrapSync&) = delete; - private: std::unique_ptr continuation_data_; }; @@ -507,6 +516,18 @@ inline int SyncCall(Environment* env, v8::Local ctx, FSReqWrapSync* req_wrap, const char* syscall, Func fn, Args... args); +// Similar to SyncCall but throws immediately if there is an error. +template +int SyncCallAndThrowIf(Predicate should_throw, + Environment* env, + FSReqWrapSync* req_wrap, + Func fn, + Args... args); +template +int SyncCallAndThrowOnError(Environment* env, + FSReqWrapSync* req_wrap, + Func fn, + Args... args); } // namespace fs } // namespace node diff --git a/test/parallel/test-bootstrap-modules.js b/test/parallel/test-bootstrap-modules.js index 8e9f6fa0f1535df..78db466a95b38e5 100644 --- a/test/parallel/test-bootstrap-modules.js +++ b/test/parallel/test-bootstrap-modules.js @@ -74,7 +74,6 @@ const expectedModules = new Set([ 'NativeModule internal/webstreams/queuingstrategies', 'NativeModule internal/blob', 'NativeModule internal/fs/utils', - 'NativeModule internal/fs/sync', 'NativeModule fs', 'Internal Binding options', 'NativeModule internal/options', From 2990390359d3bb61c7f5301a28e4e91b8d054054 Mon Sep 17 00:00:00 2001 From: Daniel Lemire Date: Fri, 29 Sep 2023 21:58:09 -0400 Subject: [PATCH 30/31] inspector: simplify dispatchProtocolMessage PR-URL: https://github.com/nodejs/node/pull/49780 Reviewed-By: Yagiz Nizipli --- src/inspector/node_string.cc | 10 ---------- src/inspector/node_string.h | 2 -- src/inspector_agent.cc | 4 ++-- .../lib/base_string_adapter_cc.template | 12 ------------ .../lib/base_string_adapter_h.template | 1 - 5 files changed, 2 insertions(+), 27 deletions(-) diff --git a/src/inspector/node_string.cc b/src/inspector/node_string.cc index 0f780f46c8ebdd3..7960971a094fd41 100644 --- a/src/inspector/node_string.cc +++ b/src/inspector/node_string.cc @@ -97,16 +97,6 @@ double toDouble(const char* buffer, size_t length, bool* ok) { return d; } -std::unique_ptr parseMessage(const std::string_view message, - bool binary) { - if (binary) { - return Value::parseBinary( - reinterpret_cast(message.data()), - message.length()); - } - return parseJSON(message); -} - ProtocolMessage jsonToMessage(String message) { return message; } diff --git a/src/inspector/node_string.h b/src/inspector/node_string.h index e36da446248ef16..369041841ef96de 100644 --- a/src/inspector/node_string.h +++ b/src/inspector/node_string.h @@ -68,8 +68,6 @@ void builderAppendQuotedString(StringBuilder& builder, const std::string_view); std::unique_ptr parseJSON(const std::string_view); std::unique_ptr parseJSON(v8_inspector::StringView view); -std::unique_ptr parseMessage(const std::string_view message, - bool binary); ProtocolMessage jsonToMessage(String message); ProtocolMessage binaryToMessage(std::vector message); String fromUTF8(const uint8_t* data, size_t length); diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index f0b4cc43c864aee..de372400fd9cedb 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -253,8 +253,8 @@ class ChannelImpl final : public v8_inspector::V8Inspector::Channel, "[inspector received] %s\n", raw_message); std::unique_ptr value = - protocol::DictionaryValue::cast(protocol::StringUtil::parseMessage( - raw_message, false)); + protocol::DictionaryValue::cast( + protocol::StringUtil::parseJSON(message)); int call_id; std::string method; node_dispatcher_->parseCommand(value.get(), &call_id, &method); diff --git a/tools/inspector_protocol/lib/base_string_adapter_cc.template b/tools/inspector_protocol/lib/base_string_adapter_cc.template index 639b39bb520d85d..af078b8e8ae7294 100644 --- a/tools/inspector_protocol/lib/base_string_adapter_cc.template +++ b/tools/inspector_protocol/lib/base_string_adapter_cc.template @@ -128,18 +128,6 @@ std::unique_ptr toBaseValue(Value* value, int depth) { return nullptr; } -// static -std::unique_ptr StringUtil::parseMessage( - const std::string& message, bool binary) { - if (binary) { - return Value::parseBinary( - reinterpret_cast(message.data()), - message.length()); - } - std::unique_ptr value = base::JSONReader::ReadDeprecated(message); - return toProtocolValue(value.get(), 1000); -} - // static ProtocolMessage StringUtil::jsonToMessage(String message) { return message; diff --git a/tools/inspector_protocol/lib/base_string_adapter_h.template b/tools/inspector_protocol/lib/base_string_adapter_h.template index 8bf3c355c0e5840..46f9e1778b12bb1 100644 --- a/tools/inspector_protocol/lib/base_string_adapter_h.template +++ b/tools/inspector_protocol/lib/base_string_adapter_h.template @@ -92,7 +92,6 @@ class {{config.lib.export_macro}} StringUtil { return builder.toString(); } - static std::unique_ptr parseMessage(const std::string& message, bool binary); static ProtocolMessage jsonToMessage(String message); static ProtocolMessage binaryToMessage(std::vector message); From 51f4ff245018153abbb918b0d4a3cce65510d762 Mon Sep 17 00:00:00 2001 From: Geoffrey Booth Date: Fri, 29 Sep 2023 19:24:14 -0700 Subject: [PATCH 31/31] module: move helpers out of cjs loader PR-URL: https://github.com/nodejs/node/pull/49912 Reviewed-By: Antoine du Hamel Reviewed-By: Luigi Pinca Reviewed-By: Yagiz Nizipli --- lib/internal/modules/cjs/loader.js | 72 ++------------------- lib/internal/modules/helpers.js | 20 ++++++ lib/internal/modules/package_json_reader.js | 52 ++++++++++++++- lib/internal/modules/run_main.js | 5 +- 4 files changed, 79 insertions(+), 70 deletions(-) diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index db31feb93ee7246..44c3f4c31fb352e 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -55,7 +55,6 @@ const { StringPrototypeCharAt, StringPrototypeCharCodeAt, StringPrototypeEndsWith, - StringPrototypeLastIndexOf, StringPrototypeIndexOf, StringPrototypeRepeat, StringPrototypeSlice, @@ -68,7 +67,7 @@ const cjsParseCache = new SafeWeakMap(); // Set first due to cycle with ESM loader functions. module.exports = { - wrapSafe, Module, toRealPath, readPackageScope, cjsParseCache, + wrapSafe, Module, cjsParseCache, get hasLoadedAnyUserCJSModule() { return hasLoadedAnyUserCJSModule; }, initializeCJS, }; @@ -88,9 +87,7 @@ const { const { internalCompileFunction } = require('internal/vm'); const assert = require('internal/assert'); const fs = require('fs'); -const internalFS = require('internal/fs/utils'); const path = require('path'); -const { sep } = path; const { internalModuleStat } = internalBinding('fs'); const { safeGetenv } = internalBinding('credentials'); const { @@ -106,6 +103,7 @@ const { makeRequireFunction, normalizeReferrerURL, stripBOM, + toRealPath, } = require('internal/modules/helpers'); const packageJsonReader = require('internal/modules/package_json_reader'); const { getOptionValue, getEmbedderOptions } = require('internal/options'); @@ -403,15 +401,7 @@ function initializeCJS() { // -> a. // -> a/index. -/** - * @param {string} requestPath - * @return {PackageConfig} - */ -function readPackage(requestPath) { - return packageJsonReader.read(path.resolve(requestPath, 'package.json')); -} - -let _readPackage = readPackage; +let _readPackage = packageJsonReader.readPackage; ObjectDefineProperty(Module, '_readPackage', { __proto__: null, get() { return _readPackage; }, @@ -423,37 +413,6 @@ ObjectDefineProperty(Module, '_readPackage', { configurable: true, }); -/** - * Get the nearest parent package.json file from a given path. - * Return the package.json data and the path to the package.json file, or false. - * @param {string} checkPath The path to start searching from. - */ -function readPackageScope(checkPath) { - const rootSeparatorIndex = StringPrototypeIndexOf(checkPath, sep); - let separatorIndex; - const enabledPermission = permission.isEnabled(); - do { - separatorIndex = StringPrototypeLastIndexOf(checkPath, sep); - checkPath = StringPrototypeSlice(checkPath, 0, separatorIndex); - // Stop the search when the process doesn't have permissions - // to walk upwards - if (enabledPermission && !permission.has('fs.read', checkPath + sep)) { - return false; - } - if (StringPrototypeEndsWith(checkPath, sep + 'node_modules')) { - return false; - } - const pjson = _readPackage(checkPath + sep); - if (pjson.exists) { - return { - data: pjson, - path: checkPath, - }; - } - } while (separatorIndex > rootSeparatorIndex); - return false; -} - /** * Try to load a specifier as a package. * @param {string} requestPath The path to what we are trying to load @@ -498,14 +457,6 @@ function tryPackage(requestPath, exts, isMain, originalPath) { return actual; } -/** - * Cache for storing resolved real paths of modules. - * In order to minimize unnecessary lstat() calls, this cache is a list of known-real paths. - * Set to an empty Map to reset. - * @type {Map} - */ -const realpathCache = new SafeMap(); - /** * Check if the file exists and is not a directory if using `--preserve-symlinks` and `isMain` is false, keep symlinks * intact, otherwise resolve to the absolute realpath. @@ -521,17 +472,6 @@ function tryFile(requestPath, isMain) { return toRealPath(requestPath); } - -/** - * Resolves the path of a given `require` specifier, following symlinks. - * @param {string} requestPath The `require` specifier - */ -function toRealPath(requestPath) { - return fs.realpathSync(requestPath, { - [internalFS.realpathCacheKey]: realpathCache, - }); -} - /** * Given a path, check if the file exists with any of the set extensions. * @param {string} basePath The path and filename without extension @@ -593,7 +533,7 @@ function trySelfParentPath(parent) { function trySelf(parentPath, request) { if (!parentPath) { return false; } - const { data: pkg, path: pkgPath } = readPackageScope(parentPath); + const { data: pkg, path: pkgPath } = packageJsonReader.readPackageScope(parentPath); if (!pkg || pkg.exports == null || pkg.name === undefined) { return false; } @@ -1153,7 +1093,7 @@ Module._resolveFilename = function(request, parent, isMain, options) { if (request[0] === '#' && (parent?.filename || parent?.id === '')) { const parentPath = parent?.filename ?? process.cwd() + path.sep; - const pkg = readPackageScope(parentPath) || { __proto__: null }; + const pkg = packageJsonReader.readPackageScope(parentPath) || { __proto__: null }; if (pkg.data?.imports != null) { try { const { packageImportsResolve } = require('internal/modules/esm/resolve'); @@ -1450,7 +1390,7 @@ Module._extensions['.js'] = function(module, filename) { content = fs.readFileSync(filename, 'utf8'); } if (StringPrototypeEndsWith(filename, '.js')) { - const pkg = readPackageScope(filename) || { __proto__: null }; + const pkg = packageJsonReader.readPackageScope(filename) || { __proto__: null }; // Function require shouldn't be used in ES modules. if (pkg.data?.type === 'module') { const parent = moduleParentCache.get(module); diff --git a/lib/internal/modules/helpers.js b/lib/internal/modules/helpers.js index cc32e95c4eb413e..7f2959cc469dc15 100644 --- a/lib/internal/modules/helpers.js +++ b/lib/internal/modules/helpers.js @@ -21,6 +21,8 @@ const { const { BuiltinModule } = require('internal/bootstrap/realm'); const { validateString } = require('internal/validators'); +const fs = require('fs'); // Import all of `fs` so that it can be monkey-patched. +const internalFS = require('internal/fs/utils'); const path = require('path'); const { pathToFileURL, fileURLToPath, URL } = require('internal/url'); @@ -39,6 +41,23 @@ let debug = require('internal/util/debuglog').debuglog('module', (fn) => { /** @typedef {import('internal/modules/cjs/loader.js').Module} Module */ +/** + * Cache for storing resolved real paths of modules. + * In order to minimize unnecessary lstat() calls, this cache is a list of known-real paths. + * Set to an empty Map to reset. + * @type {Map} + */ +const realpathCache = new SafeMap(); +/** + * Resolves the path of a given `require` specifier, following symlinks. + * @param {string} requestPath The `require` specifier + */ +function toRealPath(requestPath) { + return fs.realpathSync(requestPath, { + [internalFS.realpathCacheKey]: realpathCache, + }); +} + /** @type {Set} */ let cjsConditions; /** @@ -310,4 +329,5 @@ module.exports = { makeRequireFunction, normalizeReferrerURL, stripBOM, + toRealPath, }; diff --git a/lib/internal/modules/package_json_reader.js b/lib/internal/modules/package_json_reader.js index c4afd159ebdb639..65f5ce3551bbd0c 100644 --- a/lib/internal/modules/package_json_reader.js +++ b/lib/internal/modules/package_json_reader.js @@ -4,12 +4,17 @@ const { JSONParse, ObjectPrototypeHasOwnProperty, SafeMap, + StringPrototypeEndsWith, + StringPrototypeIndexOf, + StringPrototypeLastIndexOf, + StringPrototypeSlice, } = primordials; const { ERR_INVALID_PACKAGE_CONFIG, } = require('internal/errors').codes; const { internalModuleReadJSON } = internalBinding('fs'); -const { toNamespacedPath } = require('path'); +const { resolve, sep, toNamespacedPath } = require('path'); +const permission = require('internal/process/permission'); const { kEmptyObject, setOwnProperty } = require('internal/util'); const { fileURLToPath, pathToFileURL } = require('internal/url'); @@ -111,4 +116,47 @@ function read(jsonPath, { base, specifier, isESM } = kEmptyObject) { return result; } -module.exports = { read }; +/** + * @param {string} requestPath + * @return {PackageConfig} + */ +function readPackage(requestPath) { + return read(resolve(requestPath, 'package.json')); +} + +/** + * Get the nearest parent package.json file from a given path. + * Return the package.json data and the path to the package.json file, or false. + * @param {string} checkPath The path to start searching from. + */ +function readPackageScope(checkPath) { + const rootSeparatorIndex = StringPrototypeIndexOf(checkPath, sep); + let separatorIndex; + const enabledPermission = permission.isEnabled(); + do { + separatorIndex = StringPrototypeLastIndexOf(checkPath, sep); + checkPath = StringPrototypeSlice(checkPath, 0, separatorIndex); + // Stop the search when the process doesn't have permissions + // to walk upwards + if (enabledPermission && !permission.has('fs.read', checkPath + sep)) { + return false; + } + if (StringPrototypeEndsWith(checkPath, sep + 'node_modules')) { + return false; + } + const pjson = readPackage(checkPath + sep); + if (pjson.exists) { + return { + data: pjson, + path: checkPath, + }; + } + } while (separatorIndex > rootSeparatorIndex); + return false; +} + +module.exports = { + read, + readPackage, + readPackageScope, +}; diff --git a/lib/internal/modules/run_main.js b/lib/internal/modules/run_main.js index 2e4dabd503a883f..e5969bf7b75ea68 100644 --- a/lib/internal/modules/run_main.js +++ b/lib/internal/modules/run_main.js @@ -15,12 +15,13 @@ function resolveMainPath(main) { // Note extension resolution for the main entry point can be deprecated in a // future major. // Module._findPath is monkey-patchable here. - const { Module, toRealPath } = require('internal/modules/cjs/loader'); + const { Module } = require('internal/modules/cjs/loader'); let mainPath = Module._findPath(path.resolve(main), null, true); if (!mainPath) { return; } const preserveSymlinksMain = getOptionValue('--preserve-symlinks-main'); if (!preserveSymlinksMain) { + const { toRealPath } = require('internal/modules/helpers'); mainPath = toRealPath(mainPath); } @@ -48,7 +49,7 @@ function shouldUseESMLoader(mainPath) { if (mainPath && StringPrototypeEndsWith(mainPath, '.mjs')) { return true; } if (!mainPath || StringPrototypeEndsWith(mainPath, '.cjs')) { return false; } - const { readPackageScope } = require('internal/modules/cjs/loader'); + const { readPackageScope } = require('internal/modules/package_json_reader'); const pkg = readPackageScope(mainPath); // No need to guard `pkg` as it can only be an object or `false`. return pkg.data?.type === 'module' || getOptionValue('--experimental-default-type') === 'module';