fs: fix write methods param validation and docs

LiviaMedeiros · web-flow · commit 0c9273d1266b · 2022-04-04T12:57:59.000+02:00
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: #34993 PR-URL: #41677 Refs: #41666 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
diff --git 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.
 <!-- YAML
 added: v10.0.0
 changes:
-  - version: v14.12.0
-    pr-url: https://github.com/nodejs/node/pull/34993
-    description: The `buffer` 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 `buffer` parameter won't coerce unsupported input to
                  buffers anymore.
 -->
 
-* `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,31 +624,27 @@ the end of the file.
 <!-- YAML
 added: v10.0.0
 changes:
-  - version: v14.12.0
-    pr-url: https://github.com/nodejs/node/pull/34993
-    description: The `string` 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 `string` parameter won't coerce unsupported input to
                  strings anymore.
 -->
 
-* `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)
   documentation for more detail.
 * `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,19 +1514,14 @@ 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
                  strings anymore.
 -->
 
 * `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.
 
@@ -4361,10 +4333,6 @@ changes:
     description: Passing an invalid callback to the `callback` argument
                  now throws `ERR_INVALID_ARG_TYPE` instead of
                  `ERR_INVALID_CALLBACK`.
-  - version: v14.12.0
-    pr-url: https://github.com/nodejs/node/pull/34993
-    description: The `buffer` 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 `buffer` parameter won't coerce unsupported input to
@@ -4390,7 +4358,7 @@ changes:
 -->
 
 * `fd` {integer}
-* `buffer` {Buffer|TypedArray|DataView|string|Object}
+* `buffer` {Buffer|TypedArray|DataView}
 * `offset` {integer}
 * `length` {integer}
 * `position` {integer}
@@ -4399,8 +4367,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.
@@ -5770,10 +5737,6 @@ this API: [`fs.writeFile()`][].
 <!-- YAML
 added: v0.1.21
 changes:
-  - version: v14.12.0
-    pr-url: https://github.com/nodejs/node/pull/34993
-    description: The `buffer` 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 `buffer` parameter won't coerce unsupported input to
@@ -5791,15 +5754,12 @@ changes:
 -->
 
 * `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...)`][].
 
@@ -5808,10 +5768,6 @@ this API: [`fs.write(fd, buffer...)`][].
 <!-- YAML
 added: v0.11.5
 changes:
-  - version: v14.12.0
-    pr-url: https://github.com/nodejs/node/pull/34993
-    description: The `string` 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 `string` parameter won't coerce unsupported input to
@@ -5822,14 +5778,11 @@ changes:
 -->
 
 * `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
@@ -116,6 +116,7 @@ const {
   validateRmOptionsSync,
   validateRmdirOptions,
   validateStringAfterArrayBufferView,
+  validatePrimitiveStringAfterArrayBufferView,
   warnOnNonPortableTemplate
 } = require('internal/fs/utils');
 const {
@@ -852,7 +853,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]
@@ -876,7 +877,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
@@ -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
@@ -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
@@ -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
@@ -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()),
     {
