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

[LiviaMedeiros]

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';constfd=openSync('/mnt/flashdrive/last_digits_of_pi');constbuffer=Buffer.alloc(9);constresult=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]

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.

[LiviaMedeiros]

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';constfd=openSync(fileURLToPath(import.meta.url));constbuffer=Buffer.alloc(9);readSync(fd,buffer,{position: 2n**63n-15n,length: 9});console.log(buffer);// should workreadSync(fd,buffer,{position: 2n**63n-5n,length: 9});console.log(buffer);// might fail with EINVALreadSync(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.

[LiviaMedeiros]

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

>node.exe c:\chk.mjs <Buffer 000000000000000000> 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?

[aduh95]

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)

[aduh95]

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.

[LiviaMedeiros]

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.

[LiviaMedeiros]

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]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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)

[nodejs-github-bot]

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

[nodejs-github-bot]

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]

Landed in 6be94c9