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/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()); +} 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();