fs: validate copyFile paths in the existing C++ binding - #28
Draft
anonrig wants to merge 59 commits into
Draft
Conversation
PR-URL: nodejs#65114 Reviewed-By: Chengzhong Wu <[email protected]>
Signed-off-by: ulofiai <[email protected]> PR-URL: nodejs#65118 Fixes: nodejs#63638 Refs: libuv/libuv#5152 Refs: libuv/libuv@e640dc9 Reviewed-By: Antoine du Hamel <[email protected]>
Signed-off-by: Matteo Collina <[email protected]> PR-URL: nodejs#65250 Reviewed-By: Aviv Keller <[email protected]> Reviewed-By: René <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Daeyeon Jeong <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Stewart X Addison <[email protected]> Reviewed-By: Mike McCready <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Beth Griggs <[email protected]>
Use undefined as the no-error sentinel when cancelling broadcast and share consumers. This ensures that 0, an empty string, false, and null are propagated instead of being converted to clean completion. Make sync share surface cancellation reasons before handling detached consumers, and add regression coverage for async and sync consumers. Signed-off-by: Kamat, Trivikram <[email protected]> Assisted-by: codex:gpt-5.6-sol PR-URL: nodejs#64705 Fixes: nodejs#64704 Reviewed-By: James M Snell <[email protected]>
SQLite requires that an authorizer callback not modify the connection that invoked it, and counts sqlite3_prepare_v2() and sqlite3_step() as modifications. node:sqlite let the callback call prepare(), exec(), the statement execution methods, and other connection-mutating APIs on the same DatabaseSync. Track authorizer depth on DatabaseSync with an RAII guard around the callback and throw ERR_INVALID_STATE from the affected entry points while it is on the stack. Covering every authorizer invocation, including the re-prepare that SQLite can run during sqlite3_step(), exposed a second and distinct hazard: reentering a statement that is currently being stepped is a use-after-free rather than a contract violation, since finalizing it frees the virtual machine under sqlite3_step() and re-running it resets that machine mid-execution. Any callback SQLite invokes during execution can reach it, so a user-defined function is enough. Track the statements currently being stepped and reject reentry into only those, which leaves a user-defined function free to prepare, run, and finalize its own helper statements. Signed-off-by: Trevor Burnham <[email protected]> Fixes: nodejs#63207 Assisted-by: claude:opus-5 PR-URL: nodejs#65156 Reviewed-By: Xuguang Mei <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
Signed-off-by: Guilherme Araújo <[email protected]> PR-URL: nodejs#62757 Reviewed-By: Xuguang Mei <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Edy Silva <[email protected]>
Replace the async generator backing Symbol.asyncIterator with a hand-rolled iterator. The generator machinery costs several extra promise allocations and microtask hops per chunk: yield awaits the yielded value and resolves the pending request through separate promises. Buffered chunks are now delivered as an already-resolved promise, one microtask sooner than before. Thenable chunks are still awaited before delivery, requests received while a next() is outstanding are queued, and return()/throw() before the first next() complete the iterator without touching the stream. The earlier delivery is observable by code racing an abort against the first chunk. The flatMap AbortSignal test relied on such a race; it is reworked to abort deterministically while two mappers are in flight, asserting the concurrency limit, in-flight cancellation and rejection, without depending on delivery timing or timers. streams/readable-async-iterator.js sync='yes': +32.59% (***) streams/readable-async-iterator.js sync='no': +9.84% (***) Assisted-by: Claude Fable 5 Signed-off-by: Matteo Collina <[email protected]> PR-URL: nodejs#64447 Reviewed-By: Gürgün Dayıoğlu <[email protected]> Reviewed-By: Mattias Buelens <[email protected]> Reviewed-By: Robert Nagy <[email protected]>
The highWaterMark values were passed as properties of the underlying source and sink dictionaries, where they are ignored: a queuing strategy's highWaterMark is read from the constructors' second argument. Every configuration therefore measured the identical workload at the default highWaterMark of 1, which also explains the historically high run-to-run variance of this benchmark family. Pass the strategies as the constructors' second argument and cover the default (1) alongside buffered (1024, 4096) configurations. Signed-off-by: Matteo Collina <[email protected]> PR-URL: nodejs#65138 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gürgün Dayıoğlu <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
Three related reductions on the per-chunk paths: Wrap user sink.write and source.pull callbacks without coercing their result into a promise. When the callback returns a non-thenable (the common synchronous case), fulfillment is guaranteed and no then() lookup is observable, so the fulfilled reaction is enqueued through a single shared resolved promise at the exact microtask position the coerced promise's reaction would have had, skipping the implicit async-wrapper promise per chunk. Thenable results go through PromiseResolve(), which matches the spec's "a promise resolved with" conversion (identity for native promises). Park pipeTo's pump on backpressure by installing a record that duck-types the writer's lazily-materialized [[readyPromise]] record and whose resolve function is the pump continuation itself. Backpressure clearing then resumes the pump directly instead of materializing a fresh promise record plus reaction per flip, and the pump no longer schedules a microtask per batch. writableStreamUpdateBackpressure publishes the new backpressure state before resolving the ready record so the pump observes the updated value. Replace queueMicrotask() on the pipeTo and tee chunk-forwarding paths with a reaction on the shared resolved promise, which enqueues the continuation at the same position without the per-call scheduling overhead. pipe-to improves by 8-14% across all benchmark configurations, with readable-read and tee also improving in spot runs. Signed-off-by: Matteo Collina <[email protected]> PR-URL: nodejs#65138 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gürgün Dayıoğlu <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
The start, pull, and write non-op algorithms are all raw callbacks with an identical empty body now, so a single shared nonOpCallback replaces nonOpStart, nonOpPull, and nonOpWrite. Signed-off-by: Matteo Collina <[email protected]> PR-URL: nodejs#65138 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gürgün Dayıoğlu <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
Signed-off-by: greenhead <[email protected]> PR-URL: nodejs#65265 Refs: https://www.sqlite.org/session/c_changeset_abort.html Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
The pointer range test creates multiple closures from the same function literals and explicitly requests synchronous optimization. V8 can also schedule concurrent recompilation for those closures. Wait for background optimization before closing the dynamic library so compiler jobs cannot outlive the fast FFI metadata they reference. Signed-off-by: Kamat, Trivikram <[email protected]> Assisted-by: codex:gpt-5.6-sol PR-URL: nodejs#65300 Refs: https://github.com/nodejs/reliability/issues?q=sort%3Aupdated-desc%20%22test-ffi-fast-integer-validation%22 Reviewed-By: Aviv Keller <[email protected]> Reviewed-By: Gürgün Dayıoğlu <[email protected]>
The quic implementation calls setWakeUp with the assumption, that it is only executed once per event loop cycle. This assumption is wrong. Only setImmediate will guarantee, that the execution is delayed to later in the event loop and happening once in the event loop. Fixes: nodejs#64035 Signed-off-by: Marten Richter <[email protected]> PR-URL: nodejs#64044 Reviewed-By: James M Snell <[email protected]>
Signed-off-by: Felix P. <[email protected]> PR-URL: nodejs#65268 Reviewed-By: Aviv Keller <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ethan Arrowood <[email protected]>
Signed-off-by: Guilherme Araújo <[email protected]> PR-URL: nodejs#62241 Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: René <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
Signed-off-by: Filip Skokan <[email protected]> Co-authored-by: Aviv Keller <[email protected]> Signed-off-by: Aviv Keller <[email protected]> PR-URL: nodejs#64894 Fixes: nodejs#43583 Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
To hopefully get to the bottom of WPT crashes that have no traces. Signed-off-by: Filip Skokan <[email protected]> Co-authored-by: Aviv Keller <[email protected]> Signed-off-by: Aviv Keller <[email protected]> PR-URL: nodejs#64894 Fixes: nodejs#43583 Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
Signed-off-by: Aviv Keller <[email protected]> PR-URL: nodejs#64894 Fixes: nodejs#43583 Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
Signed-off-by: Filip Skokan <[email protected]> Co-authored-by: Aviv Keller <[email protected]> Signed-off-by: Aviv Keller <[email protected]> PR-URL: nodejs#64894 Fixes: nodejs#43583 Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
Signed-off-by: Aviv Keller <[email protected]> PR-URL: nodejs#64894 Fixes: nodejs#43583 Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
Signed-off-by: NIxxy25 <[email protected]> PR-URL: nodejs#65271 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Aviv Keller <[email protected]>
Signed-off-by: Aviv Keller <[email protected]> PR-URL: nodejs#64986 Reviewed-By: Antoine du Hamel <[email protected]>
Replace `Array.prototype.forEach()` with `for...of` loops across 17 tests in `test/parallel`, so each loop body reads as a plain statement rather than an arrow callback. None of the iterated values are sparse arrays, the one case where `forEach` and `for...of` genuinely differ, so both constructs visit the same elements in the same order. No callback relied on `this`, an early return, or async behaviour, and the number of assertions run in each file is unchanged. Signed-off-by: Phillip Markert <[email protected]> PR-URL: nodejs#65272 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Aviv Keller <[email protected]> Reviewed-By: Ethan Arrowood <[email protected]>
Signed-off-by: freida-code <[email protected]> PR-URL: nodejs#65270 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Aviv Keller <[email protected]> Reviewed-By: Ethan Arrowood <[email protected]>
PR-URL: nodejs#65224 Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
Signed-off-by: Lazizbek Ergashev <[email protected]> PR-URL: nodejs#65283 Fixes: nodejs#65280 Refs: nodejs/node-v0.x-archive#853 Refs: nodejs@3935adc Refs: nodejs#18297 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: James M Snell <[email protected]>
the binary-upload target uses $(TARNAME)-$(OSTYPE)-$(ARCH).tar.xz as the name to upload whereas it is created by the $(BINARYTAR) target as $(BINARYNAME). Since BINARYNAME includes the optional VARIATION when present this gets missed out int he binary-upload target, for example during a release build for Alpine/musl. This commit changes the binary-upload target to use the same variable for the tarball that is used when the file is created. Signed-off-by: Stewart X Addison <[email protected]> PR-URL: nodejs#65282 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gürgün Dayıoğlu <[email protected]>
Signed-off-by: 서울민트초코 <[email protected]> PR-URL: nodejs#65295 Reviewed-By: Aviv Keller <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
Signed-off-by: greenhead <[email protected]> PR-URL: nodejs#65274 Refs: nodejs#55266 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Daeyeon Jeong <[email protected]>
The error thrown for a non-number min is unchanged. Signed-off-by: greenhead <[email protected]> PR-URL: nodejs#65014 Reviewed-By: Daeyeon Jeong <[email protected]>
The kValidateObjectAllowArray flag matches the replaced check: arrays keep passing and the thrown error is unchanged. Signed-off-by: greenhead <[email protected]> PR-URL: nodejs#65015 Reviewed-By: James M Snell <[email protected]>
Use the standard SQLite integer conversion for changes and lastInsertRowid. Throw ERR_OUT_OF_RANGE when a value cannot be represented safely as a Number, or return it as a BigInt when BigInt reads are enabled. Signed-off-by: Kamat, Trivikram <[email protected]> Assisted-by: codex:gpt-5.6-sol PR-URL: nodejs#65178 Fixes: nodejs#65177 Reviewed-By: Stephen Belanger <[email protected]>
Validate Broadcast.push() and Share.pull() signals before registering raw consumers. Return a rejecting iterable for pre-aborted signals without adding a cursor. This prevents failed subscriptions from leaving unreachable cursors that inflate consumerCount and can permanently impose backpressure. Signed-off-by: Kamat, Trivikram <[email protected]> Assisted-by: codex:gpt-5.6-sol PR-URL: nodejs#65299 Fixes: nodejs#65298 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jason Zhang <[email protected]>
Add a WatchdogBinding declaration for internalBinding('watchdog')
and wire it into InternalBindingMap.
Signed-off-by: leah-1ee <[email protected]>
PR-URL: nodejs#65228
Reviewed-By: Daeyeon Jeong <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Add a DiagnosticsChannelBinding declaration for
internalBinding('diagnostics_channel') and wire it into
InternalBindingMap.
Signed-off-by: leah-1ee <[email protected]>
PR-URL: nodejs#65227
Reviewed-By: Daeyeon Jeong <[email protected]>
Reviewed-By: James M Snell <[email protected]>
The inspector can accept a connection before an --inspect-brk target enters its frontend wait. Runtime.runIfWaitingForDebugger can then be handled too early, allowing the target to subsequently block forever. Wait for NodeRuntime.waitingForDebugger before initializing and releasing launched targets. Race the handshake against disconnects and apply it to both interactive and probe startup. Refs: nodejs#64116 Assisted-by: codex:gpt-5.6-sol Co-authored-by: Archkon <[email protected]> Signed-off-by: Archkon <[email protected]> Signed-off-by: Filip Skokan <[email protected]> PR-URL: nodejs#65194 Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: James M Snell <[email protected]>
Add a SignalWrapBinding declaration for internalBinding('signal_wrap')
and wire it into InternalBindingMap.
Signed-off-by: leah-1ee <[email protected]>
PR-URL: nodejs#65229
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daeyeon Jeong <[email protected]>
PR-URL: nodejs#61198 Reviewed-By: Aviv Keller <[email protected]> Reviewed-By: Jordan Harband <[email protected]>
Signed-off-by: Antoine du Hamel <[email protected]> PR-URL: nodejs#65198 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Use concise method functions for Fast API and shared-buffer wrappers, and create native fallback functions with ConstructorBehavior::kThrow, so FFI functions remain non-constructible on all invocation paths. Signed-off-by: Kamat, Trivikram <[email protected]> Assisted-by: codex:gpt-5.6-sol PR-URL: nodejs#65184 Fixes: nodejs#65183 Reviewed-By: Paolo Insogna <[email protected]>
InvokeCallback tested `args[i] == nullptr` and mapped the argument to JS `null`. `args` is libffi's avalue array, and libffi always points each slot at its own storage for the corresponding argument, so the slot pointers are never null and the branch never ran. The check also read as a guarantee the code does not provide: a NULL pointer argument surfaces as the BigInt `0n`, because ToJSArgument converts `ffi_type_pointer` values with BigInt::NewFromUnsigned. Drop the branch rather than reimplementing it in ToJSArgument, which would change behavior by making pointer parameters arrive as either a BigInt or `null`. Signed-off-by: Trivikram Kamat <[email protected]> Assisted-by: claude:opus-5 PR-URL: nodejs#64998 Reviewed-By: Paolo Insogna <[email protected]>
Signed-off-by: avivkeller <[email protected]> PR-URL: nodejs#64590 Reviewed-By: James M Snell <[email protected]>
Long-term itch. Per v8 rules, we're not supposed to be heap allocating v8::Local's; instead we're supposed to be using v8::LocalVector. Create a specialization of MaybeStackBuffer that uses either a stack array of v8::Locals or v8::LocalVector with some additional utility improvements. Signed-off-by: James M Snell <[email protected]> PR-URL: nodejs#65159 Reviewed-By: Stephen Belanger <[email protected]>
`maybeEnableKeylog()` runs as the agent's `'newListener'` handler and
attaches the agent's keylog handler to the sockets the agent already
owns. `agent.sockets` maps a name to an array of sockets, but the loop
treated those arrays as sockets and called `.on()` on them.
Adding a `'keylog'` listener to an agent that already owned a socket
therefore threw `TypeError: sockets[i].on is not a function` out of
`agent.on('keylog', ...)`. Since the throw happened inside the
`'newListener'` handler it propagated before the listener was stored,
so the caller got an exception and no listener. Sockets parked in
`agent.freeSockets` were never visited at all.
Walk both maps the way `Agent.prototype.destroy()` does.
Signed-off-by: Shani Singh <[email protected]>
PR-URL: nodejs#65066
Reviewed-By: Tim Perry <[email protected]>
Fixes: nodejs#64214 Signed-off-by: y1d7ng <[email protected]> PR-URL: nodejs#64227 Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: Claudio Wunder <[email protected]>
Signed-off-by: ulofiai <[email protected]> PR-URL: nodejs#65095 Reviewed-By: Aviv Keller <[email protected]> Reviewed-By: Stefan Stojanovic <[email protected]>
An expired timer can run before the first complete event loop iteration, disabling the histogram before it records any samples. Drive a known number of iterations with setImmediate before checking the histograms, and share the chain between resolution variants. Signed-off-by: Kamat, Trivikram <[email protected]> Assisted-by: codex:gpt-5.6-sol PR-URL: nodejs#64728 Refs: https://github.com/nodejs/reliability/issues?q=sort%3Aupdated-desc%20test-performance-eventloopdelay Reviewed-By: Filip Skokan <[email protected]>
PR-URL: nodejs#65317 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Chemi Atlow <[email protected]>
Signed-off-by: Filip Skokan <[email protected]> PR-URL: nodejs#65320 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Aviv Keller <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
Signed-off-by: geeksilva97 <[email protected]> PR-URL: nodejs#65276 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
Add return types for Blob methods and legacy Base64 helpers so doc-kit does not render them as `void`. Refs: nodejs/doc-kit#953 Signed-off-by: inoway46 <[email protected]> PR-URL: nodejs#65308 Refs: nodejs/doc-kit#953 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
Three entries in the fs documentation described their return
value only in prose, or not at all, so doc-kit could not parse
a return type and fell back to `void`:
* `filehandle[Symbol.asyncDispose]()` and
`dir[Symbol.asyncDispose]()` both return a promise, matching
the existing `Returns: {Promise}` annotations on other async
dispose methods.
* `new fs.Utf8Stream([options])` is a constructor and returns
an instance of the class.
Verified at runtime and by rendering the page locally with
doc-kit.
Refs: nodejs/doc-kit#953
Signed-off-by: Chxxeton <[email protected]>
PR-URL: nodejs#65307
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Aviv Keller <[email protected]>
Reviewed-By: Ulises Gascón <[email protected]>
The prior text covered only two of the seven supported architectures and conflated x86-64 SysV with the stricter Win64 emitter. Signed-off-by: leah-1ee <[email protected]> PR-URL: nodejs#65207 Reviewed-By: Paolo Insogna <[email protected]>
Fixes: nodejs#63852 Signed-off-by: Erik Demaine <[email protected]> PR-URL: nodejs#63856 Reviewed-By: Stefan Stojanovic <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
Signed-off-by: T <[email protected]> PR-URL: nodejs#62941 Fixes: nodejs#61518 Reviewed-By: Aviv Keller <[email protected]>
Bumps [undici](https://github.com/nodejs/undici) from 6.27.0 to 6.28.0. - [Release notes](https://github.com/nodejs/undici/releases) - [Commits](nodejs/undici@v6.27.0...v6.28.0) --- updated-dependencies: - dependency-name: undici dependency-version: 6.28.0 dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> PR-URL: nodejs#65011 Reviewed-By: Colin Ihrig <[email protected]>
Signed-off-by: James Ross <[email protected]> PR-URL: nodejs#64886 Reviewed-By: Aviv Keller <[email protected]> Reviewed-By: Stefan Stojanovic <[email protected]> Reviewed-By: Ulises Gascón <[email protected]>
Move path validation, file URL conversion, NUL checks, mode validation, permission checks, and the copy into a dedicated C++ binding so fs.copyFileSync() no longer goes through getValidatedPath() in JavaScript. VFS dispatch stays in JS and still runs before validation. The copy itself uses uv_fs_copyfile, so flags, mode/timestamp preservation, and UV error shapes stay the same. Also fix FileURLToPath aborting on file URLs with a hostname because the ERR_INVALID_FILE_URL_HOST format string lacked %s. Signed-off-by: Yagiz Nizipli <[email protected]>
Fold getValidatedPath, file URL conversion, and NUL checks into binding.copyFile so the sync, callback, and promises paths share one implementation. Remove the extra copyFileSync binding. Signed-off-by: Cursor Agent <[email protected]> Co-authored-by: Yagiz Nizipli <[email protected]>
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
20 participants
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.
Addresses Matteo’s review on nodejs/node#65376: keep a single
binding.copyFileinstead of addingcopyFileSync.Review
getValidatedPath, file URL conversion, and NUL checks now live in the existing C++CopyFilebinding. The sync, callback, and promises JS entry points all callbinding.copyFilewith the original arguments. The extracopyFileSyncbinding is gone.VFS dispatch still happens in JavaScript before the binding. The copy itself is still
uv_fs_copyfile.The original PR branch
cursor/copyfilesync-cpp-upstream-317cwas updated with the same commit so nodejs/node#65376 has this change.Performance
Same-binary comparison of the old JS
getValidatedPath+binding.copyFilepath vs the unified C++-validatedbinding.copyFile(n=10000, 9 interleaved repeats, Releasev27.0.0-pre):I/O-bound
valid/ file URL stay within noise of the old path. ENOENT is within a few percent. The win is validation-bound input (wrong type), and because validation is onbinding.copyFile, callback and promises get that path too.benchmark/fs/bench-copyFileSync.js(n=10000) on this binary:valid~16.1k ops/sec,invalid(ENOENT) ~260k ops/sec.Testing
cpplint on
src/node_file.ccand ESLint on the JS/test changes are clean. A full Node.js rebuild of this revision was used for the numbers above.