GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||
GeezerVison is showing github.com — test: add test for `position` validation in `fs.read()` and `fs.readSync()` by LiviaMedeiros · Pull Request #42837 · nodejs/node Conversation
|
|
|
||
| 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)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
position + length exceeds int64 limitposition validation in fs.read() and fs.readSync()
aduh95
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
-
👍 1 reaction
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Antoine du Hamel <[email protected]>
18700ee to
c9054a5
Compare
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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()https://github.com/nodejs/node/actions/runs/2259461302 |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Landed in 6be94c9 |
All reactions
-
🎉 1 reaction
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
PR-URL: #42837 Co-authored-by: Antoine du Hamel <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
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]> 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: #42837 Co-authored-by: Antoine du Hamel <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
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]>
Reviewers
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assignedLabels
Projects
Milestone
No milestoneDevelopment
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
3 participants
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.