From 3e21dd91c13c051cf49acb793881617cce2e01f8 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 11 Jul 2020 01:57:04 +0200 Subject: [PATCH] src: add option to track unmanaged file descriptors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add the ability to track “raw” file descriptors, i.e. integers returned by `fs.open()`, and close them on `Environment` shutdown, to match the behavior for all other resource types (which are also closed on shutdown). PR-URL: https://github.com/nodejs/node/pull/34303 Reviewed-By: James M Snell Reviewed-By: Ben Noordhuis Reviewed-By: Benjamin Gruenbaum --- src/env-inl.h | 4 ++++ src/env.cc | 24 ++++++++++++++++++++++++ src/env.h | 6 ++++++ src/node.h | 5 ++++- src/node_file.cc | 6 ++++++ src/node_file.h | 6 +++++- 6 files changed, 49 insertions(+), 2 deletions(-) diff --git a/src/env-inl.h b/src/env-inl.h index 612174adba8558..216dead9aa972a 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -871,6 +871,10 @@ inline bool Environment::owns_inspector() const { return flags_ & EnvironmentFlags::kOwnsInspector; } +inline bool Environment::tracks_unmanaged_fds() const { + return flags_ & EnvironmentFlags::kTrackUnmanagedFds; +} + inline uint64_t Environment::thread_id() const { return thread_id_; } diff --git a/src/env.cc b/src/env.cc index ecf6a4cd2d2d99..67d7365073c31b 100644 --- a/src/env.cc +++ b/src/env.cc @@ -680,6 +680,12 @@ void Environment::RunCleanup() { } CleanupHandles(); } + + for (const int fd : unmanaged_fds_) { + uv_fs_t close_req; + uv_fs_close(nullptr, &close_req, fd, nullptr); + uv_fs_req_cleanup(&close_req); + } } void Environment::RunAtExitCallbacks() { @@ -1045,6 +1051,24 @@ Environment* Environment::worker_parent_env() const { return worker_context()->env(); } +void Environment::AddUnmanagedFd(int fd) { + if (!tracks_unmanaged_fds()) return; + auto result = unmanaged_fds_.insert(fd); + if (!result.second) { + ProcessEmitWarning( + this, "File descriptor %d opened in unmanaged mode twice", fd); + } +} + +void Environment::RemoveUnmanagedFd(int fd) { + 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); + } +} + void Environment::BuildEmbedderGraph(Isolate* isolate, EmbedderGraph* graph, void* data) { diff --git a/src/env.h b/src/env.h index e3d0a9b6128b73..e7ec30f966bcf8 100644 --- a/src/env.h +++ b/src/env.h @@ -1066,6 +1066,7 @@ class Environment : public MemoryRetainer { inline bool should_not_register_esm_loader() const; inline bool owns_process_state() const; inline bool owns_inspector() const; + inline bool tracks_unmanaged_fds() const; inline uint64_t thread_id() const; inline worker::Worker* worker_context() const; Environment* worker_parent_env() const; @@ -1277,6 +1278,9 @@ class Environment : public MemoryRetainer { void RunAndClearNativeImmediates(bool only_refed = false); void RunAndClearInterrupts(); + void AddUnmanagedFd(int fd); + void RemoveUnmanagedFd(int fd); + private: inline void ThrowError(v8::Local (*fun)(v8::Local), const char* errmsg); @@ -1432,6 +1436,8 @@ class Environment : public MemoryRetainer { ArrayBufferAllocatorList; ArrayBufferAllocatorList* keep_alive_allocators_ = nullptr; + std::unordered_set unmanaged_fds_; + std::function process_exit_handler_ { DefaultProcessExitHandler }; diff --git a/src/node.h b/src/node.h index ce8db8dbcf1850..001d4972fadb67 100644 --- a/src/node.h +++ b/src/node.h @@ -400,7 +400,10 @@ enum Flags : uint64_t { // Set if Node.js should not run its own esm loader. This is needed by some // embedders, because it's possible for the Node.js esm loader to conflict // with another one in an embedder environment, e.g. Blink's in Chromium. - kNoRegisterESMLoader = 1 << 3 + kNoRegisterESMLoader = 1 << 3, + // Set this flag to make Node.js track "raw" file descriptors, i.e. managed + // by fs.open() and fs.close(), and close them during FreeEnvironment(). + kTrackUnmanagedFds = 1 << 4 }; } // namespace EnvironmentFlags diff --git a/src/node_file.cc b/src/node_file.cc index ecf49dead7cdaa..48e8f0a6eb3de9 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -604,6 +604,9 @@ void AfterInteger(uv_fs_t* req) { FSReqBase* req_wrap = FSReqBase::from_req(req); FSReqAfterScope after(req_wrap, req); + if (req->result >= 0 && req_wrap->is_plain_open()) + req_wrap->env()->AddUnmanagedFd(req->result); + if (after.Proceed()) req_wrap->Resolve(Integer::New(req_wrap->env()->isolate(), req->result)); } @@ -813,6 +816,7 @@ void Close(const FunctionCallbackInfo& args) { CHECK(args[0]->IsInt32()); int fd = args[0].As()->Value(); + env->RemoveUnmanagedFd(fd); FSReqBase* req_wrap_async = GetReqWrap(env, args[1]); if (req_wrap_async != nullptr) { // close(fd, req) @@ -1653,6 +1657,7 @@ static void Open(const FunctionCallbackInfo& args) { FSReqBase* req_wrap_async = GetReqWrap(env, args[3]); if (req_wrap_async != nullptr) { // open(path, flags, mode, req) + req_wrap_async->set_is_plain_open(true); AsyncCall(env, req_wrap_async, args, "open", UTF8, AfterInteger, uv_fs_open, *path, flags, mode); } else { // open(path, flags, mode, undefined, ctx) @@ -1662,6 +1667,7 @@ static void Open(const FunctionCallbackInfo& args) { int result = SyncCall(env, args[4], &req_wrap_sync, "open", uv_fs_open, *path, flags, mode); FS_SYNC_TRACE_END(open); + if (result >= 0) env->AddUnmanagedFd(result); args.GetReturnValue().Set(result); } } diff --git a/src/node_file.h b/src/node_file.h index 20621af528ea5a..7fb92cf148fbb9 100644 --- a/src/node_file.h +++ b/src/node_file.h @@ -66,6 +66,9 @@ class FSReqBase : public ReqWrap { const char* data() const { return has_data_ ? *buffer_ : nullptr; } enum encoding encoding() const { return encoding_; } bool use_bigint() const { return use_bigint_; } + bool is_plain_open() const { return is_plain_open_; } + + void set_is_plain_open(bool value) { is_plain_open_ = value; } FSContinuationData* continuation_data() const { return continuation_data_.get(); @@ -87,8 +90,9 @@ class FSReqBase : public ReqWrap { std::unique_ptr continuation_data_; enum encoding encoding_ = UTF8; bool has_data_ = false; - const char* syscall_ = nullptr; bool use_bigint_ = false; + bool is_plain_open_ = false; + const char* syscall_ = nullptr; // Typically, the content of buffer_ is something like a file name, so // something around 64 bytes should be enough.