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
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 aduh95 committed Apr 4, 2022
1 parent 808e8b1 commit 18ac4af
Show file tree
Hide file tree
Showing 6 changed files with 46 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 @@ -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.
Expand Down Expand Up @@ -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)
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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`.
Expand Down Expand Up @@ -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`
Expand All @@ -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.
Expand Down Expand Up @@ -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
Expand All @@ -3792,7 +3760,7 @@ changes:
-->
* `fd` {integer}
* `buffer` {Buffer|TypedArray|DataView|string|Object}
* `buffer` {Buffer|TypedArray|DataView}
* `offset` {integer}
* `length` {integer}
* `position` {integer}
Expand All @@ -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.
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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...)`][].
Expand Down
3 changes: 2 additions & 1 deletion lib/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ const {
validateRmOptionsSync,
validateRmdirOptions,
validateStringAfterArrayBufferView,
validatePrimitiveStringAfterArrayBufferView,
warnOnNonPortableTemplate
} = require('internal/fs/utils');
const {
Expand Down Expand Up @@ -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;
Expand Down
8 changes: 4 additions & 4 deletions lib/internal/fs/promises.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,8 @@ const {
validateOffsetLengthWrite,
validateRmOptions,
validateRmdirOptions,
validateStringAfterArrayBufferView,
warnOnNonPortableTemplate
validatePrimitiveStringAfterArrayBufferView,
warnOnNonPortableTemplate,
} = require('internal/fs/utils');
const { opendir } = require('internal/fs/dir');
const {
Expand Down Expand Up @@ -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 };
Expand Down Expand Up @@ -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');
}

Expand Down
12 changes: 12 additions & 0 deletions lib/internal/fs/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -866,5 +877,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()];
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 @@ -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()),
{
Expand Down

0 comments on commit 18ac4af

Please sign in to comment.