Repository navigation
fs: validate opendir() bufferSize before opening the directory - #67183
yohannesjoya wants to merge 1 commit into
Conversation
fs.opendir() only checked options.bufferSize in the Dir constructor, which runs from the completion callback of the native opendir call. An invalid value was thrown from there, so it could not be caught and the process exited. fs.promises.opendir() is built on fs.opendir() and had the same problem. Check bufferSize before starting the native call, so fs.opendir() throws to the caller and fs.promises.opendir() rejects. Fixes: nodejs#67182 Signed-off-by: yohannesjoya <yohannesdestagebru10@gmail.com>
nodejs-github-bot
added
fs
|
Welcome to Node.js, and thank you for your first contribution! Before review, please take a moment to read:
Please make sure every commit is signed off. For a first pull request, GitHub Actions require collaborator approval and Jenkins CI must be started by a collaborator or triager, so an initial wait is normal. |
christianaurichzm
left a comment
christianaurichzm
left a comment
There was a problem hiding this comment.
Thanks! The fix makes sense, and I confirmed the new test fails on main. Left a comment about bufferSize being read twice.
| // validates it too, but an error thrown from opendirCallback can't be caught. | ||
| if (ObjectHasOwn(options, 'bufferSize')) { | ||
| validateUint32(options.bufferSize, 'options.bufferSize', true); | ||
| } |
There was a problem hiding this comment.
bufferSize is read here and again in the Dir constructor, so a getter that returns 1 and then 0 still crashes the process (it works on main). ObjectHasOwn also doesn't match the spread there. A non-enumerable bufferSize: 0 now throws here, but opendirSync() ignores it. Copying once fixes both:
| } | |
| options = { bufferSize: 32, ...options }; | |
| validateUint32(options.bufferSize, 'options.bufferSize', true); |
| ArrayPrototypeShift, | ||
| FunctionPrototypeBind, | ||
| ObjectDefineProperties, | ||
| ObjectHasOwn, |
There was a problem hiding this comment.
No longer needed.
| ObjectHasOwn, |
| // A valid bufferSize still opens the directory. | ||
| fs.opendir(tmpdir.path, { bufferSize: 1 }, common.mustSucceed((dir) => { | ||
| dir.closeSync(); | ||
| })); |
There was a problem hiding this comment.
Maybe add a test with a getter that returns 1 and then 0, checking that it's only read once?
fs.opendir()only validatedoptions.bufferSizein theDirconstructor, which runs from the completion callback of the nativeopendircall. An invalid value was thrown from there, so notry/catchcould catch it, the callback was never called, and the process exited.fs.promises.opendir()ispromisify(opendir)and had the same problem.This checks
bufferSizebefore the native call, sofs.opendir()throws to the caller andfs.promises.opendir()rejects.fs.opendirSync()is unchanged, and the check in theDirconstructor stays for it. An explicitbufferSize: undefinedis still rejected, as before.Fixes: #67182
The new test fails on main and passes with this change.
tools/test.pyforparallel/test-fs-opendir*,parallel/test-fs-*dir*andparallel/test-fs-promises*(91 tests) andmake lint-jspass locally.AI disclosure: I used a coding agent to help find the bug and draft the test. I reproduced the crash on v20.20.2, v22.20.0 and main, reviewed the change line by line, and ran the tests above locally.