From 84f74cee954aa02164b74748ee26c5f0f70af016 Mon Sep 17 00:00:00 2001 From: marcopiraccini Date: Sat, 26 Sep 2026 13:55:52 +0200 Subject: [PATCH 1/2] fs: do not swallow exceptions thrown by completion callbacks Since 93644d51269 the TryCatch guarding StringBytes::Encode in the completion callbacks of mkdtemp, realpath.native, readlink, recursive mkdir, recursive readdir and dir.read is still active when the JS callback runs. Exceptions thrown by the callback, or by the nextTick queue drained after it, are caught by it and never reported. Leave the TryCatch before calling into JS. Fixes: https://github.com/nodejs/node/issues/65667 Refs: https://github.com/nodejs/node/pull/57706 Signed-off-by: marcopiraccini --- src/node_dir.cc | 20 +++----- src/node_file-inl.h | 23 +++++++++ src/node_file.cc | 59 +++++++---------------- test/parallel/test-fs-callback-throw.js | 62 +++++++++++++++++++++++++ 4 files changed, 107 insertions(+), 57 deletions(-) create mode 100644 test/parallel/test-fs-callback-throw.js diff --git a/src/node_dir.cc b/src/node_dir.cc index eca5140b6423..ca7fc340bcdb 100644 --- a/src/node_dir.cc +++ b/src/node_dir.cc @@ -248,22 +248,14 @@ static void AfterDirRead(uv_fs_t* req) { uv_dir_t* dir = static_cast(req->ptr); - TryCatch try_catch(isolate); - Local js_array; - if (!DirentListToArray(env, - dir->dirents, - static_cast(req->result), - req_wrap->encoding()) - .ToLocal(&js_array)) { + ResolveOrReject(req_wrap.get(), [&]() { + MaybeLocal js_array = DirentListToArray( + env, dir->dirents, static_cast(req->result), req_wrap->encoding()); // Clear libuv resources *before* delivering results to JS land because - // that can schedule another operation on the same uv_dir_t. Ditto below. + // that can schedule another operation on the same uv_dir_t. after.Clear(); - CHECK(try_catch.CanContinue()); - return req_wrap->Reject(try_catch.Exception()); - } - - after.Clear(); - req_wrap->Resolve(js_array); + return js_array; + }); } diff --git a/src/node_file-inl.h b/src/node_file-inl.h index ebd97c1c8c52..5a32d002490f 100644 --- a/src/node_file-inl.h +++ b/src/node_file-inl.h @@ -413,6 +413,29 @@ int SyncCallAndThrowOnError(Environment* env, return SyncCallAndThrowIf(is_uv_error, env, req_wrap, fn, args...); } +// Delivers the value produced by `produce`, or the exception it threw, to +// `req_wrap`. The TryCatch is left before calling into JS: exceptions thrown +// by the callback, or by the tick queue drained afterwards, must not be caught +// by it. +template +void ResolveOrReject(FSReqBase* req_wrap, Fn&& produce) { + v8::Isolate* isolate = req_wrap->env()->isolate(); + v8::Local value; + v8::Local error; + { + v8::TryCatch try_catch(isolate); + if (!produce().ToLocal(&value)) { + CHECK(try_catch.CanContinue()); + error = try_catch.Exception(); + } + } + if (error.IsEmpty()) { + req_wrap->Resolve(value); + } else { + req_wrap->Reject(error); + } +} + } // namespace fs } // namespace node diff --git a/src/node_file.cc b/src/node_file.cc index b624e9da41bd..8610a04509f8 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -900,16 +900,10 @@ void AfterMkdirp(uv_fs_t* req) { std::string first_path(req_wrap->continuation_data()->first_path()); if (first_path.empty()) return req_wrap->Resolve(Undefined(req_wrap->env()->isolate())); - Local path; - TryCatch try_catch(req_wrap->env()->isolate()); - if (!StringBytes::Encode(req_wrap->env()->isolate(), - first_path.c_str(), - req_wrap->encoding()) - .ToLocal(&path)) { - CHECK(try_catch.CanContinue()); - return req_wrap->Reject(try_catch.Exception()); - } - return req_wrap->Resolve(path); + ResolveOrReject(req_wrap, [&]() { + return StringBytes::Encode( + req_wrap->env()->isolate(), first_path.c_str(), req_wrap->encoding()); + }); } } @@ -918,19 +912,11 @@ void AfterStringPath(uv_fs_t* req) { FSReqAfterScope after(req_wrap, req); FS_ASYNC_TRACE_END1( req->fs_type, req_wrap, "result", static_cast(req->result)) - MaybeLocal link; - if (after.Proceed()) { - TryCatch try_catch(req_wrap->env()->isolate()); - link = StringBytes::Encode( - req_wrap->env()->isolate(), req->path, req_wrap->encoding()); - if (link.IsEmpty()) { - CHECK(try_catch.CanContinue()); - req_wrap->Reject(try_catch.Exception()); - } else { - Local val; - if (link.ToLocal(&val)) req_wrap->Resolve(val); - } + ResolveOrReject(req_wrap, [&]() { + return StringBytes::Encode( + req_wrap->env()->isolate(), req->path, req_wrap->encoding()); + }); } } @@ -939,20 +925,12 @@ void AfterStringPtr(uv_fs_t* req) { FSReqAfterScope after(req_wrap, req); FS_ASYNC_TRACE_END1( req->fs_type, req_wrap, "result", static_cast(req->result)) - MaybeLocal link; - if (after.Proceed()) { - TryCatch try_catch(req_wrap->env()->isolate()); - link = StringBytes::Encode(req_wrap->env()->isolate(), - static_cast(req->ptr), - req_wrap->encoding()); - if (link.IsEmpty()) { - CHECK(try_catch.CanContinue()); - req_wrap->Reject(try_catch.Exception()); - } else { - Local val; - if (link.ToLocal(&val)) req_wrap->Resolve(val); - } + ResolveOrReject(req_wrap, [&]() { + return StringBytes::Encode(req_wrap->env()->isolate(), + static_cast(req->ptr), + req_wrap->encoding()); + }); } } @@ -2700,14 +2678,9 @@ class ReadDirRecursiveRequest { walk_.error_path().c_str())); } - Local value; - TryCatch try_catch(isolate); - if (!MarshalRecursiveReadDir(isolate, walk_, encoding_, with_types_) - .ToLocal(&value)) { - CHECK(try_catch.CanContinue()); - return req_wrap->Reject(try_catch.Exception()); - } - req_wrap->Resolve(value); + ResolveOrReject(req_wrap.get(), [&]() { + return MarshalRecursiveReadDir(isolate, walk_, encoding_, with_types_); + }); } private: diff --git a/test/parallel/test-fs-callback-throw.js b/test/parallel/test-fs-callback-throw.js new file mode 100644 index 000000000000..b35f33ccfe49 --- /dev/null +++ b/test/parallel/test-fs-callback-throw.js @@ -0,0 +1,62 @@ +'use strict'; +// Refs: https://github.com/nodejs/node/issues/65667 +// Exceptions thrown after an fs operation completes must reach +// 'uncaughtException' instead of being swallowed. +const common = require('../common'); +const tmpdir = require('../common/tmpdir'); +const assert = require('assert'); +const fs = require('fs'); +const path = require('path'); + +tmpdir.refresh(); +const dir = tmpdir.path; +const file = path.join(dir, 'file'); +const link = path.join(dir, 'link'); +fs.writeFileSync(file, ''); + +// The callback throws. +const callbackCases = { + 'mkdtemp': (cb) => fs.mkdtemp(path.join(dir, 'x-'), cb), + 'realpath.native': (cb) => fs.realpath.native(dir, cb), + 'mkdir recursive': (cb) => fs.mkdir(path.join(dir, 'a', 'b'), { recursive: true }, cb), + 'readdir recursive': (cb) => fs.readdir(dir, { recursive: true }, cb), +}; +if (common.canCreateSymLink()) { + fs.symlinkSync(file, link); + callbackCases.readlink = (cb) => fs.readlink(link, cb); +} + +// A nextTick callback scheduled after the promise settles throws. +const promiseCases = { + 'promises.mkdtemp': () => fs.promises.mkdtemp(path.join(dir, 'p-')), + 'dir.read': async () => { + const d = await fs.promises.opendir(dir); + await d.read(); + process.nextTick(() => d.closeSync()); + }, +}; + +const cases = [ + ...Object.entries(callbackCases).map(([name, run]) => [name, () => { + run(common.mustSucceed(() => { throw new Error(name); })); + }]), + ...Object.entries(promiseCases).map(([name, run]) => [name, async () => { + await run(); + process.nextTick(() => { throw new Error(name); }); + }]), +]; + +let current; +process.on('uncaughtException', common.mustCall((err) => { + assert.strictEqual(err.message, current); + next(); +}, cases.length)); + +function next() { + const entry = cases.shift(); + if (entry === undefined) return; + current = entry[0]; + entry[1](); +} + +next(); From b1076883da8ced2118e66ece996d299d469ad412 Mon Sep 17 00:00:00 2001 From: marcopiraccini Date: Mon, 28 Sep 2026 13:57:05 +0200 Subject: [PATCH 2/2] fixup: test coverage Signed-off-by: marcopiraccini --- test/cctest/test_node_file.cc | 84 +++++++++++++++++++++++++++++++++++ 1 file changed, 84 insertions(+) create mode 100644 test/cctest/test_node_file.cc diff --git a/test/cctest/test_node_file.cc b/test/cctest/test_node_file.cc new file mode 100644 index 000000000000..c0d9cd17bca1 --- /dev/null +++ b/test/cctest/test_node_file.cc @@ -0,0 +1,84 @@ +#include "base_object-inl.h" +#include "env-inl.h" +#include "gtest/gtest.h" +#include "node_file-inl.h" +#include "node_test_fixture.h" + +using node::fs::BindingData; +using node::fs::FSReqBase; +using v8::Exception; +using v8::FunctionCallbackInfo; +using v8::Global; +using v8::HandleScope; +using v8::Local; +using v8::MaybeLocal; +using v8::Object; +using v8::ObjectTemplate; +using v8::String; +using v8::TryCatch; +using v8::Value; + +class NodeFileTest : public EnvironmentTestFixture {}; + +class TestFSReq final : public FSReqBase { + public: + TestFSReq(BindingData* binding_data, Local object) + : FSReqBase(binding_data, + object, + node::AsyncWrap::PROVIDER_FSREQCALLBACK, + false) {} + + void Resolve(Local value) override { + resolved_.Reset(env()->isolate(), value); + } + + void Reject(Local error) override { + rejected_.Reset(env()->isolate(), error); + } + + void ResolveStat(const uv_stat_t*) override {} + void ResolveStatFs(const uv_statfs_t*) override {} + void SetReturnValue(const FunctionCallbackInfo&) override {} + + SET_MEMORY_INFO_NAME(TestFSReq) + SET_SELF_SIZE(TestFSReq) + + Local resolved() const { return resolved_.Get(env()->isolate()); } + + Local rejected() const { return rejected_.Get(env()->isolate()); } + + private: + Global resolved_; + Global rejected_; +}; + +// The reject branch cannot be reached from JavaScript, so test it in cctest. +TEST_F(NodeFileTest, ResolveOrRejectRejectsException) { + const HandleScope handle_scope(isolate_); + const Argv argv; + Env env{handle_scope, argv}; + node::LoadEnvironment(*env, "require('fs');"); + + BindingData* binding_data = + (*env)->principal_realm()->GetBindingData(); + ASSERT_NE(binding_data, nullptr); + + Local object_template = ObjectTemplate::New(isolate_); + object_template->SetInternalFieldCount(FSReqBase::kInternalFieldCount); + Local object = + object_template->NewInstance(env.context()).ToLocalChecked(); + auto req = node::MakeDetachedBaseObject(binding_data, object); + Local expected = Exception::Error( + String::NewFromUtf8Literal(isolate_, "expected exception")); + TryCatch try_catch(isolate_); + + node::fs::ResolveOrReject(req.get(), [&]() -> MaybeLocal { + isolate_->ThrowException(expected); + return {}; + }); + + EXPECT_TRUE(req->resolved().IsEmpty()); + ASSERT_FALSE(req->rejected().IsEmpty()); + EXPECT_TRUE(req->rejected()->StrictEquals(expected)); + EXPECT_FALSE(try_catch.HasCaught()); +}