Skip to content

Commit

Permalink
fs: fix write methods param validation and docs
Browse files Browse the repository at this point in the history
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
Backport-PR-URL: #42631
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>
  • Loading branch information
LiviaMedeiros authored and juanarbol committed Apr 7, 2022
1 parent 6e56924 commit 8009cb0
Show file tree
Hide file tree
Showing 6 changed files with 45 additions and 71 deletions.
81 changes: 17 additions & 64 deletions doc/api/fs.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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)
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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`.
Expand Down Expand Up @@ -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`
Expand All @@ -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.
Expand Down Expand Up @@ -4161,10 +4133,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
Expand All @@ -4190,7 +4158,7 @@ changes:
-->
* `fd` {integer}
* `buffer` {Buffer|TypedArray|DataView|string|Object}
* `buffer` {Buffer|TypedArray|DataView}
* `offset` {integer}
* `length` {integer}
* `position` {integer}
Expand All @@ -4199,8 +4167,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.
Expand Down Expand Up @@ -5557,10 +5524,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
Expand All @@ -5578,15 +5541,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...)`][].
Expand All @@ -5595,10 +5555,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
Expand All @@ -5609,14 +5565,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...)`][].
Expand Down
5 changes: 3 additions & 2 deletions lib/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ const {
validateRmOptionsSync,
validateRmdirOptions,
validateStringAfterArrayBufferView,
validatePrimitiveStringAfterArrayBufferView,
warnOnNonPortableTemplate
} = require('internal/fs/utils');
const {
Expand Down Expand Up @@ -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]
Expand All @@ -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)
Expand Down
6 changes: 3 additions & 3 deletions lib/internal/fs/promises.js
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ const {
validateOffsetLengthWrite,
validateRmOptions,
validateRmdirOptions,
validateStringAfterArrayBufferView,
validatePrimitiveStringAfterArrayBufferView,
warnOnNonPortableTemplate,
} = require('internal/fs/utils');
const { opendir } = require('internal/fs/dir');
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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');
}

Expand Down
11 changes: 11 additions & 0 deletions lib/internal/fs/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -943,5 +953,6 @@ module.exports = {
validateRmOptionsSync,
validateRmdirOptions,
validateStringAfterArrayBufferView,
validatePrimitiveStringAfterArrayBufferView,
warnOnNonPortableTemplate
};
7 changes: 6 additions & 1 deletion test/parallel/test-fs-promises-file-handle-write.js
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
6 changes: 5 additions & 1 deletion test/parallel/test-fs-write.js
Original file line number Diff line number Diff line change
Expand Up @@ -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()),
{
Expand Down

0 comments on commit 8009cb0

Please sign in to comment.