I added a new custom ESLint rule to fix these problems.
We have a lot of replaceable codes with primordials.
Accessing built-in objects is restricted by existing rule
(no-restricted-globals), but accessing property in the built-in objects
is not restricted right now. We manually review codes that can be
replaced by primordials, but there's a lot of code that actually needs
to be fixed. We have often made pull requests to replace the primordials
with.
Restrict accessing global built-in objects such as `Promise`.
Restrict calling static methods such as `Array.from` or `Symbol.for`.
Don't restrict prototype methods to prevent false-positive.
PR-URL: https://github.com/nodejs/node/pull/35448
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Ben Coe <bencoe@gmail.com>
This is to unsure that code using those methods won't crash if the
methods are deleted in userland.
PR-URL: https://github.com/nodejs/node/pull/35837
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
This is a follow up to #35494 to add a deprecation warning when
using recursive rmdir. This only warns if you are attempting
to remove a file or a nonexistent path.
PR-URL: https://github.com/nodejs/node/pull/35562
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
This commit updates validateRmOptions() to throw on input
validation failures. This is consistent with how Node handles
validation in most places across the codebase.
PR-URL: https://github.com/nodejs/node/pull/35602
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
The validation code for the rm/rmdir functions treated the
options as distinct parameters instead of the options properties
that they are. This commit updates the validation to treat them
like properties.
PR-URL: https://github.com/nodejs/node/pull/35565
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Masashi Hirano <shisama07@gmail.com>
This commit updates rm() to use the EISDIR constant with
ERR_FS_EISDIR instead of hard coding -21.
PR-URL: https://github.com/nodejs/node/pull/35563
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
This PR introduces a new method fs.rm that provides the behaviour of
rimraf when used with the recursive: true and force: true options.
PR-URL: https://github.com/nodejs/node/pull/35494
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruy Adorno <ruyadorno@github.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Once an `FSReqCallback` instance is created, it is a GC root until
the underlying fs operation has completed, meaning that it cannot
be garbage collected.
This is a problem when the underlying operation never starts
because an exception is thrown before that happens, for example
as part of parameter validation.
Instead, move all potentially throwing code before the `FSReqCallback`
creation.
PR-URL: https://github.com/nodejs/node/pull/35487
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Before this change, only the first part of typed arrays which have more
than 1 byte per element (e.g. Uint16Array) would be written.
This also removes the use of the `slice` method to avoid unnecessary
copying the data.
Fixes: https://github.com/nodejs/node/issues/35343
PR-URL: https://github.com/nodejs/node/pull/35376
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
This will be a start to generalize all argument validation
errors. As currently we throw ARG/OPT, OUT_OF_RANGE, and other more
specific errors.
The OPT errors didn't bring much to the errors as it's just another
variant of ARG error which is sometimes more confusing (some of our code
used OPT errors to denote just argument validation errors presumably
because of similarity of OPT to 'option' and not 'options-object')
and they don't specify the name of the options object where the invalid
value is located. Much better approach would be to just specify path
to the invalid value in the name of the value as it is done in this PR
(i.e. 'options.format', 'options.publicKey.type' etc)
Also since this decreases a variety of errors we have it'd be easier to
reuse validation code across the codebase.
Refs: https://github.com/nodejs/node/pull/31251
Refs: https://github.com/nodejs/node/pull/34070#discussion_r467251009
Signed-off-by: Denys Otrishko <shishugi@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/34682
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Calling close on a file description which is currently in use is
undefined behavior due to implementation details in libuv. Add
a guard against this when using FileHandle.
PR-URL: https://github.com/nodejs/node/pull/34746
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/33399
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Allow passing `FileHandle` instances in the transfer list
of a `.postMessage()` call.
PR-URL: https://github.com/nodejs/node/pull/33772
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Removes the else after return to match rest of the style of
the function.
PR-URL: https://github.com/nodejs/node/pull/33496
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Use the namespaced (with the \\?\ prefix) paths for symlink targets when
the path is absolute. This allows creation of symlinks to files with
long filenames.
Fixes: https://github.com/nodejs/node/issues/27795
PR-URL: https://github.com/nodejs/node/pull/33351
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
libuv does not expect concurrent operations on `uv_dir_t` instances,
and will gladly create memory leaks, corrupt data, or crash the
process.
This patch forbids that, and:
- Makes sure that concurrent async operations are run sequentially
- Throws an exception if sync operations are attempted during an
async operation
The assumption here is that a thrown exception is preferable to
a potential hard crash.
This fully fixes flakiness from `parallel/test-fs-opendir` when
run under ASAN.
PR-URL: https://github.com/nodejs/node/pull/33274
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/32640
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/32662
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
destroy w/ is undocumented API which also will cause
a race if the stream is already destroying and potentially
invoking the callback too early and withou error.
PR-URL: https://github.com/nodejs/node/pull/32809
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/32694
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Use "options.recursive" instead of just "recursive"
Signed-off-by: James M Snell <jasnell@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/32472
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Previously destroy could be called multiple times causing inconsistent
and hard to predict behavior. Furthermore, since the stream _destroy
implementation can only be called once, the behavior of applying destroy
multiple times becomes unclear.
This changes so that only the first destroy() call is executed and any
subsequent calls are noops.
PR-URL: https://github.com/nodejs/node/pull/29197
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
WriteStream autoClose was implemented by manually
calling .destroy() instead of using autoDestroy
and callback. This caused some invariants related
to order of events to be broken.
Fixes: https://github.com/nodejs/node/issues/31776
PR-URL: https://github.com/nodejs/node/pull/31790
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
The input was not validated so far and that caused unwanted side
effects. E.g., `undefined` became the string `'undefined'`. It was
expected to fail or to end up as empty string.
Now all input is validated to be either some type of array buffer
view or a string. That way it's always clear what the user intents.
PR-URL: https://github.com/nodejs/node/pull/31030
Fixes: https://github.com/nodejs/node/issues/31025
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Change the type of `Buffer::kMaxLength` to size_t because upcoming
changes in V8 will allow typed arrays > 2 GB on 64 bits platforms.
Not all platforms handle file reads and writes > 2 GB though so keep
enforcing the 2 GB typed array limit for I/O operations.
Fixes: https://github.com/nodejs/node/issues/31399
Refs: https://github.com/libuv/libuv/pull/1501
PR-URL: https://github.com/nodejs/node/pull/31406
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Previously due to compat reasons 'close' was only emitted if no 'error'.
This removes the compat behavior in order to properly follow expected
streams behavior.
PR-URL: https://github.com/nodejs/node/pull/31408
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
fs streams have some backwards compat behavior that does not
behave well if emitClose: true is passed in options. This
fixes this edge case until the backwards compat is removed.
PR-URL: https://github.com/nodejs/node/pull/31383
Fixes: https://github.com/nodejs/node/issues/31366
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
- Do not set the fd as a property on the native object.
- Use the already-existent `GetFD()` method to pass the
fd from C++ to JS.
- Cache the fd in JS to avoid repeated accesses to the
C++ getter.
- Set the fd to `-1` after close, thus reliably making
subsequent calls using the `FileHandle` return `EBADF`.
Fixes: https://github.com/nodejs/node/issues/31361
PR-URL: https://github.com/nodejs/node/pull/31389
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
This fixes a few bugs in `fs`. E.g., `fs.promises.access` accepted
strings as mode. It should have only accepted numbers. It will now
always validate the flags and the mode argument in an consistent way.
PR-URL: https://github.com/nodejs/node/pull/27044
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
When operating on a FileHandle, the file has already been opened with
the flag given as an option to fs.promises.open(). This means defaulting
to 'a' has no effect here, and FileHandle#appendFile turns out to
exactly be an alias of writeFile. This is now explicit, saving a stack
frame, object copy and assignment.
PR-URL: https://github.com/nodejs/node/pull/31235
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
