fs: validate the callback passed to fs.rm() - #65588
Open
bitpshr wants to merge 1 commit into
Open
Conversation
Every other callback based fs API runs its callback through makeCallback(), so a missing or non-function callback is reported synchronously as ERR_INVALID_ARG_TYPE. fs.rm() skipped that step, so the bad callback was not noticed until rimraf tried to invoke it, by which point the removal had already happened and the resulting "callback is not a function" TypeError was thrown from an async completion handler where it could not be caught. Refs: nodejs#65578 Signed-off-by: Paul Bouchon <mail@bitpshr.net>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #65588 +/- ##
==========================================
- Coverage 90.07% 90.07% -0.01%
==========================================
Files 751 751
Lines 254875 254876 +1
Branches 48108 48116 +8
==========================================
- Hits 229579 229568 -11
- Misses 16466 16477 +11
- Partials 8830 8831 +1
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs: #65578
Every other callback based
fsAPI runs its callback throughmakeCallback(), so a missing or non-function callback is reported synchronously asERR_INVALID_ARG_TYPE.fs.rm()skipped that step:The second one is thrown from an async completion handler, so it cannot be caught, and it only happens after the removal has already run. On my machine the directory was gone by the time the process died. Adding the
makeCallback()call bringsrm()in line withrmdir()andunlink()and means nothing is deleted when the call was malformed.I found this while looking at #65578. That issue is mostly about
fs.rmsilently failing on non-ASCII paths on Windows, which I have no way to reproduce and which this does not address. But one row of the reporter's table is the crash above, and that reproduces anywhere with plain ASCII paths, so it seemed worth separating out.I put the
makeCallback()call after the vfs handler check because that is whereunlink()andrmdir()put theirs. That leaves the vfs branch passing an unvalidated callback tovfsVoid(), but that is equally true of the other two today, so fixing it only here felt like the wrong shape. Happy to do it across all of them in a follow-up if that is preferred.