test: add test for `position` validation in `fs.read()` and `fs.readSync()` #42837

LiviaMedeiros · 2022-04-23T11:49:14Z

This PR adds a test for position validation in fs.read() and fs.readSync() methods.

To be expanded in: #42835 (filehandle.read() and cases where method tries to read bytes beyond int64 limit)

Outdated stuff

This PR replaced `EINVAL` from `read(2)` syscall with `RangeError` exception, when `fs.read()` or `fs.readSync()` is called with huge bigint `position`, and adding `length` makes it bigger than `2⁶³ - 1`.

import { openSync, readSync } from 'fs';
const fd = openSync('/mnt/flashdrive/last_digits_of_pi');
const buffer = Buffer.alloc(9);
const result = readSync(fd, buffer, { position: 2n ** 63n - 5n, length: 9 });

node:fs:723
  handleErrorFromBinding(ctx);
  ^

Error: EINVAL: invalid argument, read
    at readSync (node:fs:723:3)
    at file:///tmp/test.mjs:4:16
    at ModuleJob.run (node:internal/modules/esm/module_job:197:25)
    at async Promise.all (index 0)
    at async ESMLoader.import (node:internal/modules/esm/loader:337:24)
    at async loadESM (node:internal/process/esm_loader:88:5)
    at async handleMainPromise (node:internal/modules/run_main:61:12) {
  errno: -22,
  syscall: 'read',
  code: 'EINVAL'
}

aduh95

Can we add a test?

aduh95 · 2022-04-23T18:50:39Z

test/parallel/test-fs-read-position-validation.mjs

+
+  await testValid(2n ** 63n - 1n - BigInt(length));
+  await testInvalid('ERR_OUT_OF_RANGE', 2n ** 63n);
+  await testInvalid('ERR_OUT_OF_RANGE', 2n ** 63n - BigInt(length));


It looks like passing 2n ** 63n - BigInt(length) works on master (on both the sync and the async version), I'm not seeing any EINVAL error 🤔 maybe it's OS specific or something? If that's the case, it's probably not worth doing it in a separate PR, it's probably easier to land it alongside the other changes as semver-major.

Huh. Well, it definitely depends on read syscall implementation.
I retried this on three Node.js versions:

import { fileURLToPath } from 'url'; import { openSync, readSync } from 'fs'; const fd = openSync(fileURLToPath(import.meta.url)); const buffer = Buffer.alloc(9); readSync(fd, buffer, { position: 2n ** 63n - 15n, length: 9 }); console.log(buffer); // should work readSync(fd, buffer, { position: 2n ** 63n - 5n, length: 9 }); console.log(buffer); // might fail with EINVAL readSync(fd, buffer, { position: 2n ** 63n + 5n, length: 9 }); console.log(buffer); // must fail with exception

v14.17.6 most likely doesn't support bigints and assumes current position so it reads from the start:

<Buffer 69 6d 70 6f 72 74 20 7b 20> <Buffer 66 69 6c 65 55 52 4c 54 6f> <Buffer 50 61 74 68 20 7d 20 66 72>

v16.14.1 and master (v19.0.0-pre) both show this:

<Buffer 00 00 00 00 00 00 00 00 00> node:fs:732 handleErrorFromBinding(ctx); ^ Error: EINVAL: invalid argument, read at readSync (node:fs:732:3) at file:///tmp/chk.mjs:6:1 at ModuleJob.run (node:internal/modules/esm/module_job:198:25) at async Promise.all (index 0) at async ESMLoader.import (node:internal/modules/esm/loader:409:24) at async loadESM (node:internal/process/esm_loader:85:5) at async handleMainPromise (node:internal/modules/run_main:61:12) { errno: -22, syscall: 'read', code: 'EINVAL' }

This is on 64bit linux 5.17.0.

If the - 5n line works on any other platform, it is definitely semver-major indeed.

Tried v18.0.0 release build on 64bit windows 10.0.18363, same result

>node.exe c:\chk.mjs <Buffer 00 00 00 00 00 00 00 00 00> node:fs:732 handleErrorFromBinding(ctx); ^ Error: EINVAL: invalid argument, read at readSync (node:fs:732:3) at file:///c:/chk.mjs:6:1 at ModuleJob.run (node:internal/modules/esm/module_job:198:25) at async Promise.all (index 0) at async ESMLoader.import (node:internal/modules/esm/loader:409:24) at async loadESM (node:internal/process/esm_loader:85:5) at async handleMainPromise (node:internal/modules/run_main:61:12) { errno: -4071, syscall: 'read', code: 'EINVAL' } Node.js v18.0.0

Out of curiosity, on which platform it works successfully?

I’ve tested it on macOS. Could we try to revert the changes in lib, change the test case to catch EINVAL, and run the CI to see which platforms behave like this? (in case that’s just an oddity of my machine or if I’ve tested it wrong somehow)

So it looks like the behavior is indeed platform specific. I suggest we keep in this PR all the tests that pass on master and make the behavior consistent in the other PR that's semver-major.

Yep! Can we have one more CI run please? I wanna make sure that testing this part for success and continuing on won't show anything unexpected for these platforms.

Also, results on AIX are bothering: https://ci.nodejs.org/job/node-test-commit-aix/40804/nodes=aix72-ppc64/testReport/junit/(root)/test/parallel_test_fs_read_position_validation/
This one reported Error: EFBIG: file too large, read on the test with Number.MAX_SAFE_INTEGER
I guess there's no choice but to suppress this particular error in testValid.

Thanks a lot.
If I'm reading CI reports correctly, the results were mutually exclusive, except for AIX.
Helper functions got slightly overgeneralized but it should be fine.

nodejs-github-bot · 2022-04-23T22:15:31Z

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

nodejs-github-bot · 2022-04-24T08:40:07Z

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

nodejs-github-bot · 2022-04-24T12:57:31Z

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

aduh95

Assuming the CI results are green, that'd be great If you could rebase and update the commit message (otherwise it can be done by whoever lands this PR)

test/parallel/test-fs-read-position-validation.mjs

Co-authored-by: Antoine du Hamel <[email protected]>

nodejs-github-bot · 2022-04-24T13:53:02Z

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

nodejs-github-bot · 2022-05-02T17:49:22Z

Commit Queue failed

- Loading data for nodejs/node/pull/42837 ✔ Done loading data for nodejs/node/pull/42837 ----------------------------------- PR info ------------------------------------ Title test: add test for `position` validation in `fs.read()` and `fs.readSync()` (#42837) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch LiviaMedeiros:fs-reads-position-einvalfix -> nodejs:master Labels fs, author ready, needs-ci Commits 1 - test: add test for position validation in fs.read() and fs.readSync() Committers 1 - LiviaMedeiros PR-URL: https://github.com/nodejs/node/pull/42837 Reviewed-By: Antoine du Hamel ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/42837 Reviewed-By: Antoine du Hamel -------------------------------------------------------------------------------- ℹ This PR was created on Sat, 23 Apr 2022 11:49:14 GMT ✔ Approvals: 1 ✔ - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/42837#pullrequestreview-959354525 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2022-04-24T13:53:02Z: https://ci.nodejs.org/job/node-test-pull-request/43666/ - Querying data for job/node-test-pull-request/43666/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/master up to date... From https://github.com/nodejs/node * branch master -> FETCH_HEAD acffd3d9e6..916a13a8a3 master -> origin/master ✔ origin/master is now up-to-date master is out of sync with origin/master. Mismatched commits: - 668380b721 http2: compat support for array headers - 916a13a8a3 http2: compat support for array headers -------------------------------------------------------------------------------- HEAD is now at 916a13a8a3 http2: compat support for array headers ✔ Reset to origin/master - Downloading patch for 42837 From https://github.com/nodejs/node * branch refs/pull/42837/merge -> FETCH_HEAD ✔ Fetched commits as 916a13a8a36d..c9054a554940 -------------------------------------------------------------------------------- [master 548016233f] test: add test for position validation in fs.read() and fs.readSync() Author: LiviaMedeiros Date: Sat Apr 23 19:06:45 2022 +0800 1 file changed, 133 insertions(+) create mode 100644 test/parallel/test-fs-read-position-validation.mjs ✔ Patches applied -------------------------------------------------------------------------------- --------------------------------- New Message ---------------------------------- test: add test for position validation in fs.read() and fs.readSync()

Co-authored-by: Antoine du Hamel [email protected]

PR-URL: #42837
Reviewed-By: Antoine du Hamel [email protected]

[master a5d9fd2e47] test: add test for position validation in fs.read() and fs.readSync()
Author: LiviaMedeiros [email protected]
Date: Sat Apr 23 19:06:45 2022 +0800
1 file changed, 133 insertions(+)
create mode 100644 test/parallel/test-fs-read-position-validation.mjs
✖ a5d9fd2e475437aa0d7ffc5248a2d74f49a86cde
✖ 1:0 Co-authored-by must be a trailer co-authored-by-is-trailer
✔ 0:0 skipping fixes-url fixes-url
✔ 0:0 blank line after title line-after-title
✔ 0:0 line-lengths are valid line-length
✔ 0:0 metadata is at end of message metadata-end
✔ 3:8 PR-URL is valid. pr-url
✔ 0:0 reviewers are valid reviewers
✔ 0:0 valid subsystems subsystem
✔ 0:0 Title is formatted correctly. title-format
⚠ 0:50 Title should be <= 50 columns. title-length

ℹ Please fix the commit message and try again.

https://github.com/nodejs/node/actions/runs/2259461302

aduh95 · 2022-05-02T17:52:53Z

Landed in 6be94c9

PR-URL: #42837 Co-authored-by: Antoine du Hamel <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

PR-URL: nodejs#42837 Co-authored-by: Antoine du Hamel <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

PR-URL: nodejs#42837 Co-authored-by: Antoine du Hamel <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Backport-PR-URL: nodejs#43588

PR-URL: #42837 Co-authored-by: Antoine du Hamel <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

PR-URL: nodejs/node#42837 Co-authored-by: Antoine du Hamel <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Apr 23, 2022

LiviaMedeiros mentioned this pull request Apr 23, 2022

fs: adjust position validation in reading methods #42835

Merged

aduh95 reviewed Apr 23, 2022

View reviewed changes

aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 23, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 23, 2022

github-actions bot mentioned this pull request Apr 24, 2022

CI Reliability 2022-04-24 nodejs/reliability#261

Open

21 tasks

aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 24, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 24, 2022

LiviaMedeiros changed the title ~~fs: replace EINVAL with exception when position + length exceeds int64 limit~~ test: add test for position validation in fs.read() and fs.readSync() Apr 24, 2022

aduh95 approved these changes Apr 24, 2022

View reviewed changes

test/parallel/test-fs-read-position-validation.mjs Outdated Show resolved Hide resolved

test: add test for position validation in fs.read() and fs.readSync()

c9054a5

Co-authored-by: Antoine du Hamel <[email protected]>

LiviaMedeiros force-pushed the fs-reads-position-einvalfix branch from 18700ee to c9054a5 Compare April 24, 2022 13:32

aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Apr 24, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 24, 2022

This was referenced Apr 25, 2022

CI Reliability 2022-04-25 nodejs/reliability#262

Open

CI Reliability 2022-04-26 nodejs/reliability#263

Open

CI Reliability 2022-04-27 nodejs/reliability#264

Open

aduh95 mentioned this pull request Apr 27, 2022

fs: make params in writing methods optional #42601

Merged

This was referenced Apr 28, 2022

CI Reliability 2022-04-28 nodejs/reliability#265

Open

CI Reliability 2022-04-29 nodejs/reliability#266

Open

aduh95 approved these changes May 2, 2022

View reviewed changes

aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label May 2, 2022

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 2, 2022

nodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label May 2, 2022

aduh95 merged commit 6be94c9 into nodejs:master May 2, 2022

RafaelGSS pushed a commit that referenced this pull request May 10, 2022

test: add test for position validation in fs.read() and fs.readSync()

68ed3c8

PR-URL: #42837 Co-authored-by: Antoine du Hamel <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

RafaelGSS mentioned this pull request May 10, 2022

v18.2.0 proposal #43036

Merged

juanarbol pushed a commit that referenced this pull request May 31, 2022

test: add test for position validation in fs.read() and fs.readSync()

51a6afa

PR-URL: #42837 Co-authored-by: Antoine du Hamel <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

juanarbol mentioned this pull request May 31, 2022

2022-05-XX, Version 16.16.0 'Gallium' (LTS) #43268

Closed

LiviaMedeiros mentioned this pull request Jun 27, 2022

[v16.x backport] test: add test for position validation in fs.read() and fs.readSync() #43588

Closed

danielleadams pushed a commit that referenced this pull request Jun 27, 2022

test: add test for position validation in fs.read() and fs.readSync()

0ce9a6c

PR-URL: #42837 Co-authored-by: Antoine du Hamel <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

targos pushed a commit that referenced this pull request Jul 12, 2022

test: add test for position validation in fs.read() and fs.readSync()

e55a3ca

PR-URL: #42837 Co-authored-by: Antoine du Hamel <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

targos pushed a commit that referenced this pull request Jul 31, 2022

test: add test for position validation in fs.read() and fs.readSync()

eceb318

PR-URL: #42837 Co-authored-by: Antoine du Hamel <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

targos mentioned this pull request Aug 3, 2022

v16.17.0 release proposal #44098

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: add test for `position` validation in `fs.read()` and `fs.readSync()` #42837

test: add test for `position` validation in `fs.read()` and `fs.readSync()` #42837

LiviaMedeiros commented Apr 23, 2022 •

edited

Loading

aduh95 left a comment

aduh95 Apr 23, 2022

LiviaMedeiros Apr 23, 2022

LiviaMedeiros Apr 23, 2022

aduh95 Apr 23, 2022

aduh95 Apr 23, 2022

LiviaMedeiros Apr 24, 2022

LiviaMedeiros Apr 24, 2022

nodejs-github-bot commented Apr 23, 2022

nodejs-github-bot commented Apr 24, 2022

nodejs-github-bot commented Apr 24, 2022

aduh95 left a comment

nodejs-github-bot commented Apr 24, 2022

nodejs-github-bot commented May 2, 2022

PR-URL: #42837
Reviewed-By: Antoine du Hamel [email protected]

aduh95 commented May 2, 2022

test: add test for position validation in fs.read() and fs.readSync() #42837

test: add test for position validation in fs.read() and fs.readSync() #42837

Conversation

LiviaMedeiros commented Apr 23, 2022 • edited Loading

aduh95 left a comment

Choose a reason for hiding this comment

aduh95 Apr 23, 2022

Choose a reason for hiding this comment

LiviaMedeiros Apr 23, 2022

Choose a reason for hiding this comment

LiviaMedeiros Apr 23, 2022

Choose a reason for hiding this comment

aduh95 Apr 23, 2022

Choose a reason for hiding this comment

aduh95 Apr 23, 2022

Choose a reason for hiding this comment

LiviaMedeiros Apr 24, 2022

Choose a reason for hiding this comment

LiviaMedeiros Apr 24, 2022

Choose a reason for hiding this comment

nodejs-github-bot commented Apr 23, 2022

nodejs-github-bot commented Apr 24, 2022

nodejs-github-bot commented Apr 24, 2022

aduh95 left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Apr 24, 2022

nodejs-github-bot commented May 2, 2022

PR-URL: #42837 Reviewed-By: Antoine du Hamel [email protected]

aduh95 commented May 2, 2022

test: add test for `position` validation in `fs.read()` and `fs.readSync()` #42837

test: add test for `position` validation in `fs.read()` and `fs.readSync()` #42837

LiviaMedeiros commented Apr 23, 2022 •

edited

Loading

PR-URL: #42837
Reviewed-By: Antoine du Hamel [email protected]