diff --git a/lib/fs.js b/lib/fs.js index 2119554c0e72c1..5af4955982b656 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -515,7 +515,7 @@ function close(fd, callback = defaultCloseCallback) { * @returns {void} */ function closeSync(fd) { - binding.close(fd); + binding.closeSync(fd); } /** diff --git a/lib/internal/fs/read/context.js b/lib/internal/fs/read/context.js index b1a5d6ae03e953..c2abd1cd877829 100644 --- a/lib/internal/fs/read/context.js +++ b/lib/internal/fs/read/context.js @@ -15,7 +15,10 @@ const { const { Buffer } = require('buffer'); -const { FSReqCallback, close, read } = internalBinding('fs'); +// It is important to not destructure the binding object, as it will +// cause the creation context to be undefined, and cause fast API calls +// to fail. +const binding = internalBinding('fs'); const { AbortError, @@ -102,11 +105,11 @@ class ReadFileContext { length = MathMin(kReadFileBufferLength, this.size - this.pos); } - const req = new FSReqCallback(); + const req = new binding.FSReqCallback(); req.oncomplete = readFileAfterRead; req.context = this; - read(this.fd, buffer, offset, length, -1, req); + binding.read(this.fd, buffer, offset, length, -1, req); } close(err) { @@ -117,12 +120,12 @@ class ReadFileContext { return; } - const req = new FSReqCallback(); + const req = new binding.FSReqCallback(); req.oncomplete = readFileAfterClose; req.context = this; this.err = err; - close(this.fd, req); + binding.close(this.fd, req); } } diff --git a/src/env.cc b/src/env.cc index 799b36aaebdded..4cde2bf170209a 100644 --- a/src/env.cc +++ b/src/env.cc @@ -1841,12 +1841,29 @@ void Environment::AddUnmanagedFd(int fd) { } } -void Environment::RemoveUnmanagedFd(int fd) { +void Environment::RemoveUnmanagedFd(int fd, + ProcessEmitScheduleType schedule_type) { if (!tracks_unmanaged_fds()) return; size_t removed_count = unmanaged_fds_.erase(fd); if (removed_count == 0) { - ProcessEmitWarning( - this, "File descriptor %d closed but not opened in unmanaged mode", fd); + switch (schedule_type) { + case kSync: + ProcessEmitWarning( + this, + "File descriptor %d closed but not opened in unmanaged mode", + fd); + break; + case kSetImmediate: + SetImmediateThreadsafe([&](Environment* env) { + ProcessEmitWarning( + env, + "File descriptor %d closed but not opened in unmanaged mode", + fd); + }); + break; + default: + UNREACHABLE(); + } } } diff --git a/src/env.h b/src/env.h index b2c54e079b57df..896a7f5bdb8fc0 100644 --- a/src/env.h +++ b/src/env.h @@ -1026,8 +1026,13 @@ class Environment : public MemoryRetainer { uv_buf_t allocate_managed_buffer(const size_t suggested_size); std::unique_ptr release_managed_buffer(const uv_buf_t& buf); + enum ProcessEmitScheduleType { + kSync, + kSetImmediate, + }; + void AddUnmanagedFd(int fd); - void RemoveUnmanagedFd(int fd); + void RemoveUnmanagedFd(int fd, ProcessEmitScheduleType schedule_type = kSync); template void ForEachRealm(T&& iterator) const; diff --git a/src/node_external_reference.h b/src/node_external_reference.h index b80b8727c23fd1..907dfdb414e991 100644 --- a/src/node_external_reference.h +++ b/src/node_external_reference.h @@ -55,6 +55,9 @@ using CFunctionWithInt64Fallback = void (*)(v8::Local, const int64_t, v8::FastApiCallbackOptions&); using CFunctionWithBool = void (*)(v8::Local, bool); +using CFunctionWithUint32AndOptions = void (*)(v8::Local, + const uint32_t, + v8::FastApiCallbackOptions&); // This class manages the external references from the V8 heap // to the C++ addresses in Node.js. @@ -79,6 +82,7 @@ class ExternalReferenceRegistry { V(CFunctionWithDoubleReturnDouble) \ V(CFunctionWithInt64Fallback) \ V(CFunctionWithBool) \ + V(CFunctionWithUint32AndOptions) \ V(const v8::CFunctionInfo*) \ V(v8::FunctionCallback) \ V(v8::AccessorNameGetterCallback) \ diff --git a/src/node_file.cc b/src/node_file.cc index a86482b194bff5..6d992edd975995 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -60,6 +60,7 @@ namespace fs { using v8::Array; using v8::BigInt; +using v8::CFunction; using v8::Context; using v8::EscapableHandleScope; using v8::FastApiCallbackOptions; @@ -971,11 +972,10 @@ void Access(const FunctionCallbackInfo& args) { } } -void Close(const FunctionCallbackInfo& args) { +static void Close(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - const int argc = args.Length(); - CHECK_GE(argc, 1); + CHECK_EQ(args.Length(), 2); // fd, req int fd; if (!GetValidatedFd(env, args[0]).To(&fd)) { @@ -983,20 +983,56 @@ void Close(const FunctionCallbackInfo& args) { } env->RemoveUnmanagedFd(fd); - if (argc > 1) { // close(fd, req) - FSReqBase* req_wrap_async = GetReqWrap(args, 1); - CHECK_NOT_NULL(req_wrap_async); - FS_ASYNC_TRACE_BEGIN0(UV_FS_CLOSE, req_wrap_async) - AsyncCall(env, req_wrap_async, args, "close", UTF8, AfterNoArgs, - uv_fs_close, fd); - } else { // close(fd) - FSReqWrapSync req_wrap_sync("close"); - FS_SYNC_TRACE_BEGIN(close); - SyncCallAndThrowOnError(env, &req_wrap_sync, uv_fs_close, fd); - FS_SYNC_TRACE_END(close); + FSReqBase* req_wrap_async = GetReqWrap(args, 1); + CHECK_NOT_NULL(req_wrap_async); + FS_ASYNC_TRACE_BEGIN0(UV_FS_CLOSE, req_wrap_async) + AsyncCall( + env, req_wrap_async, args, "close", UTF8, AfterNoArgs, uv_fs_close, fd); +} + +// Separate implementations are required to provide fast API for closeSync. +// If both close and closeSync are implemented using the same function, and +// if a fast API implementation is added for closeSync, close(fd, req) will +// also trigger the fast API implementation and cause an incident. +// Ref: https://github.com/nodejs/node/issues/53902 +static void CloseSync(const FunctionCallbackInfo& args) { + Environment* env = Environment::GetCurrent(args); + CHECK_EQ(args.Length(), 1); + + int fd; + if (!GetValidatedFd(env, args[0]).To(&fd)) { + return; + } + env->RemoveUnmanagedFd(fd); + FSReqWrapSync req_wrap_sync("close"); + FS_SYNC_TRACE_BEGIN(close); + SyncCallAndThrowOnError(env, &req_wrap_sync, uv_fs_close, fd); + FS_SYNC_TRACE_END(close); +} + +static void FastCloseSync(Local recv, + const uint32_t fd, + // NOLINTNEXTLINE(runtime/references) This is V8 api. + v8::FastApiCallbackOptions& options) { + Environment* env = Environment::GetCurrent(recv->GetCreationContextChecked()); + + uv_fs_t req; + FS_SYNC_TRACE_BEGIN(close); + int err = uv_fs_close(nullptr, &req, fd, nullptr); + FS_SYNC_TRACE_END(close); + + if (is_uv_error(err)) { + options.fallback = true; + } else { + // Note: Only remove unmanaged fds if the close was successful. + // RemoveUnmanagedFd() can call ProcessEmitWarning() which calls back into + // JS process.emitWarning() and violates the fast API protocol. + env->RemoveUnmanagedFd(fd, Environment::kSetImmediate); } } +CFunction fast_close_sync_(CFunction::Make(FastCloseSync)); + static void ExistsSync(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); Isolate* isolate = env->isolate(); @@ -3547,6 +3583,7 @@ static void CreatePerIsolateProperties(IsolateData* isolate_data, GetFormatOfExtensionlessFile); SetMethod(isolate, target, "access", Access); SetMethod(isolate, target, "close", Close); + SetFastMethod(isolate, target, "closeSync", CloseSync, &fast_close_sync_); SetMethod(isolate, target, "existsSync", ExistsSync); SetMethod(isolate, target, "open", Open); SetMethod(isolate, target, "openFileHandle", OpenFileHandle); @@ -3674,6 +3711,9 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) { registry->Register(GetFormatOfExtensionlessFile); registry->Register(Close); + registry->Register(CloseSync); + registry->Register(FastCloseSync); + registry->Register(fast_close_sync_.GetTypeInfo()); registry->Register(ExistsSync); registry->Register(Open); registry->Register(OpenFileHandle); diff --git a/test/parallel/test-fs-close-fast-api.js b/test/parallel/test-fs-close-fast-api.js new file mode 100644 index 00000000000000..37bed014a76e22 --- /dev/null +++ b/test/parallel/test-fs-close-fast-api.js @@ -0,0 +1,31 @@ +'use strict'; + +const common = require('../common'); +const fs = require('fs'); + +// A large number is selected to ensure that the fast path is called. +// Ref: https://github.com/nodejs/node/issues/53902 +for (let i = 0; i < 100_000; i++) { + try { + fs.closeSync(100); + } catch (e) { + // Ignore all EBADF errors. + // EBADF stands for "Bad file descriptor". + if (e.code !== 'EBADF') { + throw e; + } + } +} + +// This test is to ensure that fs.close() does not crash under pressure. +for (let i = 0; i < 100_000; i++) { + fs.close(100, common.mustCall()); +} + +// This test is to ensure that fs.readFile() does not crash. +// readFile() calls fs.close() internally if the input is not a file descriptor. +// Large number is selected to ensure that the fast path is called. +// Ref: https://github.com/nodejs/node/issues/53902 +for (let i = 0; i < 100_000; i++) { + fs.readFile(__filename, common.mustCall()); +} diff --git a/typings/internalBinding/fs.d.ts b/typings/internalBinding/fs.d.ts index 2d86a412ba686d..810305436b8cbe 100644 --- a/typings/internalBinding/fs.d.ts +++ b/typings/internalBinding/fs.d.ts @@ -70,7 +70,7 @@ declare namespace InternalFSBinding { function chown(path: string, uid: number, gid: number): void; function close(fd: number, req: FSReqCallback): void; - function close(fd: number): void; + function closeSync(fd: number): void; function copyFile(src: StringOrBuffer, dest: StringOrBuffer, mode: number, req: FSReqCallback): void; function copyFile(src: StringOrBuffer, dest: StringOrBuffer, mode: number, req: undefined, ctx: FSSyncContext): void; @@ -258,6 +258,7 @@ export interface FsBinding { chmod: typeof InternalFSBinding.chmod; chown: typeof InternalFSBinding.chown; close: typeof InternalFSBinding.close; + closeSync: typeof InternalFSBinding.closeSync; copyFile: typeof InternalFSBinding.copyFile; cpSyncCheckPaths: typeof InternalFSBinding.cpSyncCheckPaths; fchmod: typeof InternalFSBinding.fchmod;