fs: fix write methods param validation and docs

LiviaMedeiros · aduh95 · commit 18ac4afb2ff6 · 2022-04-04T14:12:18.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: nodejs#34993 PR-URL: nodejs#41677 Refs: nodejs#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
@@ -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`.
 <!-- 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)
@@ -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,31 +457,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
@@ -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,19 +1252,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`
@@ -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:
 <!-- YAML
 added: v0.0.2
 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
@@ -3792,7 +3760,7 @@ changes:
 -->
 
 * `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()`][].
 <!-- 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
@@ -5067,26 +5030,19 @@ 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...)`][].
 
 ### `fs.writeSync(fd, string[, position[, encoding]])`
 <!-- 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
@@ -5097,14 +5053,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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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()),
     {
