fs: harden options typecheck in reading methods

[LiviaMedeiros]

Can we have this on semver-patch level?

This PR adds validation in fs.read() and filehandle.read() to make sure that first argument is a buffer or a params object (that is, not arbitrary-type value). In case of null or undefined, default values are used.

It also adds the same validation in fs.readSync(), which has a different signature: buffer is not a part of options object, but a mandatory first argument.

Currently, said methods are destructuring any non-ArrayBufferView value as {buffer, offset, position, length } object.

For example, this happens without any warning if we accidentaly pass string or array (both have .length property):

import{open}from'fs/promises';constfh=awaitopen(process.argv[2],'r');constres=awaitfh.read('gwak');// or [1, 2, 3, 4]console.log('result:',res);console.log('string:',String(res.buffer));console.log('string length:',String(res.buffer).length);

$ cat /tmp/tst ABCDEFGHI $ node test.mjs /tmp/tst result:{bytesRead: 4, buffer: <Buffer 41 42 43 44 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ... 16334 more bytes> } string: ABCD string length: 16384

With the patch:

$ node test.mjs /tmp/tst node:internal/errors:475 ErrorCaptureStackTrace(err); ^ TypeError [ERR_INVALID_ARG_TYPE]: The "options" argument must be of type object. Received type string ('gwak') at read (node:internal/fs/promises:516:7) at fsCall (node:internal/fs/promises:367:18) at FileHandle.read (node:internal/fs/promises:168:12) at file://test.mjs:3:22{code: 'ERR_INVALID_ARG_TYPE' } Node.js v18.0.0-pre

[aduh95]

Can we have this on semver-patch level?

Adding an error to a stable API is considered semver-majorPRs that contain breaking changes and should be released in the next major version. as there could be code that currently works and would start to throw (e.g. the test you've modified).

node/doc/contributing/collaborator-guide.md

Lines 342 to 349 in 8dbdca8

	Examples of breaking changes include:
	* Removal or redefinition of existing API arguments.
	* Changing return values.
	* Removing or modifying existing properties on an options argument.
	* Adding or removing errors.
	* Altering expected timing of an event.
	* Changing the side effects of using a particular API.

The semver cutoff for v18.x is already closed, so this would need to wait for v19.0.0 (or you can ask the TSC for an exception and land this on v18.x anyway if you feel strongly it should sooner than that, but that most likely can't happen in 18.0.0).

[aduh95]

//cc @nodejs/fs

[LiviaMedeiros]

Thanks for detailed explanation.

I'd prefer it to be in master branch sooner, but only if it can be considered to be non-breaking.
The documentation says that accepted types are only Buffer | TypedArray | DataView or Object, so maybe there are chances to see it as a mere bugfix.

As for the test, I assumed it to be outdated. handle.read(0, 0, 0, 0) initially worked as buffer = offset = position = length = 0, but now the implementation does {buffer, offset, position, length } = 0 (and ignores further arguments), so it still passes only thanks to primitive numbers not having these properties.

However, technically it's indeed adding an error and potentially breaking a code that uses this "undocumented feature". In this case, it can be worked around until 19.x.

[aduh95]

As a Node.js collaborator, I'm bound to the collaborator guide and cannot decide on my own to treat this PR as something other than semver-majorPRs that contain breaking changes and should be released in the next major version. (because it's technically a breaking change). But I can add the tsc-agendaIssues and PRs to discuss during the meetings of the TSC. label and ask the TSC for a one-off exception.

As a TSC member, I'd be personally -1 on landing it on any LTS release, and -0 to land it on v18.x as a semver-minorPRs that contain new features and should be released in the next minor version. . I could be convinced otherwise if there was a security risk with the current behavior, to me it looks more like a DX nice-to-have change (and semver compliance is arguably more important for DX). However that's just me, I can't speak for the other TSC members.

[LiviaMedeiros]

Affirmative. Let's keep it as semver-majorPRs that contain breaking changes and should be released in the next major version. with aim on 19.x.

I'll add patches for fs.readSync() as well.

[LiviaMedeiros]

Notes on fs.readSync() changes:

All lines in test/parallel/test-fs-readSync-optional-params.js that got changed/removed were designed to be changed/removed ASAP anyway.

{offset: null } seems to be unwanted, see #32479

new String('4444') still works but is still marked as malicious code.
Sorry if these tests were looking as something that should actually keep passing. 😄

[aduh95]

Given that this PR is semver-majorPRs that contain breaking changes and should be released in the next major version. , I would recommend not landing it just yet to minimize the risks of conflict between Current release line and master. For info, Node.js v19.0.0 semver-cutoff is in October 2022. Of course, if that PR is blocking other work for you, it's OK to land it now.

[LiviaMedeiros]

Thanks for warning! No, it's not blocking for now and I'm not planning to land it too early; just want to keep it in up-to-date (ideally in ready-to-land) state to minimize complexity of potential merge conflicts later.

[aduh95]

Hey @LiviaMedeiros can you please rebase to fix the conflicts?

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/46509/

[nodejs-github-bot]

Landed in 53633c0...41a6d82