Skip to content

fs: harden options typecheck in reading methods#42772

Closed
LiviaMedeiros wants to merge 3 commits into
nodejs:mainfrom
LiviaMedeiros:harden-read-typecheck
Closed

fs: harden options typecheck in reading methods#42772
LiviaMedeiros wants to merge 3 commits into
nodejs:mainfrom
LiviaMedeiros:harden-read-typecheck

Conversation

@LiviaMedeiros

@LiviaMedeiros LiviaMedeiros commented Apr 18, 2022

Copy link
Copy Markdown
Member

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';
const fh = await open(process.argv[2], 'r');
const res = await fh.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

Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. semver-major PRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants