From 18ac4afb2ff6527c85ccff1ede4722f4f6a62a54 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 | 81 ++++--------------- lib/fs.js | 3 +- lib/internal/fs/promises.js | 8 +- lib/internal/fs/utils.js | 12 +++ .../test-fs-promises-file-handle-write.js | 7 +- test/parallel/test-fs-write.js | 6 +- 6 files changed, 46 insertions(+), 71 deletions(-) diff --git a/doc/api/fs.md b/doc/api/fs.md index bf605818c8132c..e5006e34995085 100644 --- a/doc/api/fs.md +++ b/doc/api/fs.md @@ -171,18 +171,13 @@ changes: - version: 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. @@ -425,21 +420,17 @@ promise with an error using code `UV_ENOSYS`. -* `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) @@ -448,13 +439,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 @@ -469,17 +457,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) @@ -487,13 +471,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 @@ -510,27 +494,21 @@ changes: - version: 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`. @@ -1274,10 +1252,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 @@ -1285,8 +1259,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` @@ -1295,8 +1268,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. @@ -3763,10 +3735,6 @@ This happens when: * `fd` {integer} -* `buffer` {Buffer|TypedArray|DataView|string|Object} +* `buffer` {Buffer|TypedArray|DataView} * `offset` {integer} * `length` {integer} * `position` {integer} @@ -3801,8 +3769,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. @@ -5046,10 +5013,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...)`][]. @@ -5083,10 +5043,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 acd8e10f390869..95354aad273abf 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -103,6 +103,7 @@ const { validateRmOptionsSync, validateRmdirOptions, validateStringAfterArrayBufferView, + validatePrimitiveStringAfterArrayBufferView, warnOnNonPortableTemplate } = require('internal/fs/utils'); const { @@ -726,7 +727,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'); if (offset === undefined) offset = null; diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index db2e54bad68342..90448699022b18 100644 --- a/lib/internal/fs/promises.js +++ b/lib/internal/fs/promises.js @@ -56,8 +56,8 @@ const { validateOffsetLengthWrite, validateRmOptions, validateRmdirOptions, - validateStringAfterArrayBufferView, - warnOnNonPortableTemplate + validatePrimitiveStringAfterArrayBufferView, + warnOnNonPortableTemplate, } = require('internal/fs/utils'); const { opendir } = require('internal/fs/dir'); const { @@ -461,7 +461,7 @@ async function write(handle, buffer, offset, length, position) { return { bytesWritten, buffer }; } - validateStringAfterArrayBufferView(buffer, 'buffer'); + validatePrimitiveStringAfterArrayBufferView(buffer, 'buffer'); const bytesWritten = (await binding.writeString(handle.fd, buffer, offset, length, kUsePromises)) || 0; return { bytesWritten, buffer }; @@ -683,7 +683,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 6af79033f452b6..772d10ca5eec55 100644 --- a/lib/internal/fs/utils.js +++ b/lib/internal/fs/utils.js @@ -815,6 +815,17 @@ 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'); @@ -866,5 +877,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 3c25842d8bf9cc..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()]; + 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 63f0245663c435..8a5ec9d22daccf 100644 --- a/test/parallel/test-fs-write.js +++ b/test/parallel/test-fs-write.js @@ -154,7 +154,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()), {