From 99f9880a6fd0f26afd7411e677efb79c3a625c05 Mon Sep 17 00:00:00 2001 From: Livia Medeiros <74449973+LiviaMedeiros@users.noreply.github.com> Date: Mon, 4 Apr 2022 18:57:59 +0800 Subject: [PATCH] fs: fix write methods param validation and docs The FS docs wrongfully indicated support for passing object with an own `toString` function property to `FileHandle.prototype.appendFile`, `FileHandle.prototype.writeFile`, `FileHandle.prototype.write`, `fsPromises.writeFile`, and `fs.writeSync`. This commit fixes that, and adds some test to ensure the actual behavior is aligned with the docs. It also fixes a bug that makes the process crash if a non-buffer object was passed to `FileHandle.prototype.write`. Refs: https://github.com/nodejs/node/pull/34993 PR-URL: https://github.com/nodejs/node/pull/41677 Refs: https://github.com/nodejs/node/issues/41666 Reviewed-By: Antoine du Hamel Reviewed-By: Matteo Collina Reviewed-By: James M Snell --- doc/api/fs.md | 82 +++++-------------- lib/fs.js | 5 +- lib/internal/fs/promises.js | 6 +- lib/internal/fs/utils.js | 11 +++ .../test-fs-promises-file-handle-write.js | 7 +- test/parallel/test-fs-write.js | 6 +- 6 files changed, 50 insertions(+), 67 deletions(-) diff --git a/doc/api/fs.md b/doc/api/fs.md index 8bc833c41d4088..337baafc1307f8 100644 --- a/doc/api/fs.md +++ b/doc/api/fs.md @@ -185,18 +185,13 @@ changes: - v14.18.0 pr-url: https://github.com/nodejs/node/pull/37490 description: The `data` argument supports `AsyncIterable`, `Iterable` and `Stream`. - - version: v14.12.0 - pr-url: https://github.com/nodejs/node/pull/34993 - description: The `data` parameter will stringify an object with an - explicit `toString` function. - version: v14.0.0 pr-url: https://github.com/nodejs/node/pull/31030 description: The `data` parameter won't coerce unsupported input to strings anymore. --> -* `data` {string|Buffer|TypedArray|DataView|Object|AsyncIterable|Iterable - |Stream} +* `data` {string|Buffer|TypedArray|DataView|AsyncIterable|Iterable|Stream} * `options` {Object|string} * `encoding` {string|null} **Default:** `'utf8'` * Returns: {Promise} Fulfills with `undefined` upon success. @@ -591,21 +586,17 @@ then resolves the promise with no arguments upon success. -* `buffer` {Buffer|TypedArray|DataView|string|Object} +* `buffer` {Buffer|TypedArray|DataView} * `offset` {integer} The start position from within `buffer` where the data to write begins. **Default:** `0` * `length` {integer} The number of bytes from `buffer` to write. **Default:** - `buffer.byteLength` + `buffer.byteLength - offset` * `position` {integer} The offset from the beginning of the file where the data from `buffer` should be written. If `position` is not a `number`, the data will be written at the current position. See the POSIX pwrite(2) @@ -614,13 +605,10 @@ changes: Write `buffer` to the file. -If `buffer` is a plain object, it must have an own (not inherited) `toString` -function property. - The promise is resolved with an object containing two properties: * `bytesWritten` {integer} the number of bytes written -* `buffer` {Buffer|TypedArray|DataView|string|Object} a reference to the +* `buffer` {Buffer|TypedArray|DataView} a reference to the `buffer` written. It is unsafe to use `filehandle.write()` multiple times on the same file @@ -636,17 +624,13 @@ the end of the file. -* `string` {string|Object} +* `string` {string} * `position` {integer} The offset from the beginning of the file where the data from `string` should be written. If `position` is not a `number` the data will be written at the current position. See the POSIX pwrite(2) @@ -654,13 +638,13 @@ changes: * `encoding` {string} The expected string encoding. **Default:** `'utf8'` * Returns: {Promise} -Write `string` to the file. If `string` is not a string, or an object with an -own `toString` function property, the promise is rejected with an error. +Write `string` to the file. If `string` is not a string, the promise is +rejected with an error. The promise is resolved with an object containing two properties: * `bytesWritten` {integer} the number of bytes written -* `buffer` {string|Object} a reference to the `string` written. +* `buffer` {string} a reference to the `string` written. It is unsafe to use `filehandle.write()` multiple times on the same file without waiting for the promise to be resolved (or rejected). For this @@ -680,27 +664,21 @@ changes: - v14.18.0 pr-url: https://github.com/nodejs/node/pull/37490 description: The `data` argument supports `AsyncIterable`, `Iterable` and `Stream`. - - version: v14.12.0 - pr-url: https://github.com/nodejs/node/pull/34993 - description: The `data` parameter will stringify an object with an - explicit `toString` function. - version: v14.0.0 pr-url: https://github.com/nodejs/node/pull/31030 description: The `data` parameter won't coerce unsupported input to strings anymore. --> -* `data` {string|Buffer|TypedArray|DataView|Object|AsyncIterable|Iterable - |Stream} +* `data` {string|Buffer|TypedArray|DataView|AsyncIterable|Iterable|Stream} * `options` {Object|string} * `encoding` {string|null} The expected character encoding when `data` is a string. **Default:** `'utf8'` * Returns: {Promise} Asynchronously writes data to a file, replacing the file if it already exists. -`data` can be a string, a buffer, an {AsyncIterable} or {Iterable} object, or an -object with an own `toString` function -property. The promise is resolved with no arguments upon success. +`data` can be a string, a buffer, an {AsyncIterable} or {Iterable} object. +The promise is resolved with no arguments upon success. If `options` is a string, then it specifies the `encoding`. @@ -1536,10 +1514,6 @@ changes: pr-url: https://github.com/nodejs/node/pull/35993 description: The options argument may include an AbortSignal to abort an ongoing writeFile request. - - version: v14.12.0 - pr-url: https://github.com/nodejs/node/pull/34993 - description: The `data` parameter will stringify an object with an - explicit `toString` function. - version: v14.0.0 pr-url: https://github.com/nodejs/node/pull/31030 description: The `data` parameter won't coerce unsupported input to @@ -1547,8 +1521,7 @@ changes: --> * `file` {string|Buffer|URL|FileHandle} filename or `FileHandle` -* `data` {string|Buffer|TypedArray|DataView|Object|AsyncIterable|Iterable - |Stream} +* `data` {string|Buffer|TypedArray|DataView|AsyncIterable|Iterable|Stream} * `options` {Object|string} * `encoding` {string|null} **Default:** `'utf8'` * `mode` {integer} **Default:** `0o666` @@ -1557,8 +1530,7 @@ changes: * Returns: {Promise} Fulfills with `undefined` upon success. Asynchronously writes data to a file, replacing the file if it already exists. -`data` can be a string, a {Buffer}, or, an object with an own (not inherited) -`toString` function property. +`data` can be a string, a buffer, an {AsyncIterable} or {Iterable} object. The `encoding` option is ignored if `data` is a buffer. @@ -4161,6 +4133,11 @@ This happens when: * `fd` {integer} -* `buffer` {Buffer|TypedArray|DataView|string|Object} +* `buffer` {Buffer|TypedArray|DataView} * `offset` {integer} * `length` {integer} * `position` {integer} @@ -4199,8 +4176,7 @@ changes: * `bytesWritten` {integer} * `buffer` {Buffer|TypedArray|DataView} -Write `buffer` to the file specified by `fd`. If `buffer` is a normal object, it -must have an own `toString` function property. +Write `buffer` to the file specified by `fd`. `offset` determines the part of the buffer to be written, and `length` is an integer specifying the number of bytes to write. @@ -5557,10 +5533,6 @@ this API: [`fs.writeFile()`][]. * `fd` {integer} -* `buffer` {Buffer|TypedArray|DataView|string|Object} +* `buffer` {Buffer|TypedArray|DataView} * `offset` {integer} * `length` {integer} * `position` {integer} * Returns: {number} The number of bytes written. -If `buffer` is a plain object, it must have an own (not inherited) `toString` -function property. - For detailed information, see the documentation of the asynchronous version of this API: [`fs.write(fd, buffer...)`][]. @@ -5595,10 +5564,6 @@ this API: [`fs.write(fd, buffer...)`][]. * `fd` {integer} -* `string` {string|Object} +* `string` {string} * `position` {integer} * `encoding` {string} * Returns: {number} The number of bytes written. -If `string` is a plain object, it must have an own (not inherited) `toString` -function property. - For detailed information, see the documentation of the asynchronous version of this API: [`fs.write(fd, string...)`][]. diff --git a/lib/fs.js b/lib/fs.js index 3299a62495e2ec..907cc39e94118d 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -116,6 +116,7 @@ const { validateRmOptionsSync, validateRmdirOptions, validateStringAfterArrayBufferView, + validatePrimitiveStringAfterArrayBufferView, warnOnNonPortableTemplate } = require('internal/fs/utils'); const { @@ -853,7 +854,7 @@ ObjectDefineProperty(write, internalUtil.customPromisifyArgs, * Synchronously writes `buffer` to the * specified `fd` (file descriptor). * @param {number} fd - * @param {Buffer | TypedArray | DataView | string | object} buffer + * @param {Buffer | TypedArray | DataView | string} buffer * @param {number} [offset] * @param {number} [length] * @param {number} [position] @@ -877,7 +878,7 @@ function writeSync(fd, buffer, offset, length, position) { result = binding.writeBuffer(fd, buffer, offset, length, position, undefined, ctx); } else { - validateStringAfterArrayBufferView(buffer, 'buffer'); + validatePrimitiveStringAfterArrayBufferView(buffer, 'buffer'); validateEncoding(buffer, length); if (offset === undefined) diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index 868c7df2f1ed79..73c159550740b0 100644 --- a/lib/internal/fs/promises.js +++ b/lib/internal/fs/promises.js @@ -65,7 +65,7 @@ const { validateOffsetLengthWrite, validateRmOptions, validateRmdirOptions, - validateStringAfterArrayBufferView, + validatePrimitiveStringAfterArrayBufferView, warnOnNonPortableTemplate, } = require('internal/fs/utils'); const { opendir } = require('internal/fs/dir'); @@ -581,7 +581,7 @@ async function write(handle, buffer, offset, length, position) { return { bytesWritten, buffer }; } - validateStringAfterArrayBufferView(buffer, 'buffer'); + validatePrimitiveStringAfterArrayBufferView(buffer, 'buffer'); validateEncoding(buffer, length); const bytesWritten = (await binding.writeString(handle.fd, buffer, offset, length, kUsePromises)) || 0; @@ -811,7 +811,7 @@ async function writeFile(path, data, options) { const flag = options.flag || 'w'; if (!isArrayBufferView(data) && !isCustomIterable(data)) { - validateStringAfterArrayBufferView(data, 'data'); + validatePrimitiveStringAfterArrayBufferView(data, 'data'); data = Buffer.from(data, options.encoding || 'utf8'); } diff --git a/lib/internal/fs/utils.js b/lib/internal/fs/utils.js index 481b5292b1d726..61670011372bb7 100644 --- a/lib/internal/fs/utils.js +++ b/lib/internal/fs/utils.js @@ -889,6 +889,16 @@ const validateStringAfterArrayBufferView = hideStackFrames((buffer, name) => { ); }); +const validatePrimitiveStringAfterArrayBufferView = hideStackFrames((buffer, name) => { + if (typeof buffer !== 'string') { + throw new ERR_INVALID_ARG_TYPE( + name, + ['string', 'Buffer', 'TypedArray', 'DataView'], + buffer + ); + } +}); + const validatePosition = hideStackFrames((position, name) => { if (typeof position === 'number') { validateInteger(position, 'position'); @@ -943,5 +953,6 @@ module.exports = { validateRmOptionsSync, validateRmdirOptions, validateStringAfterArrayBufferView, + validatePrimitiveStringAfterArrayBufferView, warnOnNonPortableTemplate }; diff --git a/test/parallel/test-fs-promises-file-handle-write.js b/test/parallel/test-fs-promises-file-handle-write.js index 8d020522767853..7f3d12d4817042 100644 --- a/test/parallel/test-fs-promises-file-handle-write.js +++ b/test/parallel/test-fs-promises-file-handle-write.js @@ -53,7 +53,12 @@ async function validateNonUint8ArrayWrite() { async function validateNonStringValuesWrite() { const filePathForHandle = path.resolve(tmpDir, 'tmp-non-string-write.txt'); const fileHandle = await open(filePathForHandle, 'w+'); - const nonStringValues = [123, {}, new Map(), null, undefined, 0n, () => {}, Symbol(), true]; + const nonStringValues = [ + 123, {}, new Map(), null, undefined, 0n, () => {}, Symbol(), true, + new String('notPrimitive'), + { toString() { return 'amObject'; } }, + { [Symbol.toPrimitive]: (hint) => 'amObject' }, + ]; for (const nonStringValue of nonStringValues) { await assert.rejects( fileHandle.write(nonStringValue), diff --git a/test/parallel/test-fs-write.js b/test/parallel/test-fs-write.js index 30e25b15808dd2..c30eba28bd95a0 100644 --- a/test/parallel/test-fs-write.js +++ b/test/parallel/test-fs-write.js @@ -155,7 +155,11 @@ fs.open(fn4, 'w', 0o644, common.mustSucceed((fd) => { ); }); -[false, 5, {}, [], null, undefined].forEach((data) => { +[ + false, 5, {}, [], null, undefined, + new String('notPrimitive'), + { [Symbol.toPrimitive]: (hint) => 'amObject' }, +].forEach((data) => { assert.throws( () => fs.write(1, data, common.mustNotCall()), {