Skip to content

Commit 94c0e06

Browse files
marcopiracciniaduh95
marcopiraccini
authored and
aduh95
committed
fs: do not swallow exceptions thrown by completion callbacks
Since 93644d5 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: #65667 Refs: #57706 Signed-off-by: marcopiraccini <marco.piraccini@gmail.com> PR-URL: #66308 Reviewed-By: Paolo Insogna <paolo@cowtech.it>
1 parent 33d4e6c commit 94c0e06

5 files changed

Lines changed: 195 additions & 73 deletions

File tree

‎src/node_dir.cc‎

Lines changed: 6 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -248,22 +248,14 @@ static void AfterDirRead(uv_fs_t* req) {
248248

249249
uv_dir_t* dir = static_cast<uv_dir_t*>(req->ptr);
250250

251-
TryCatch try_catch(isolate);
252-
Local<Array> js_array;
253-
if (!DirentListToArray(env,
254-
dir->dirents,
255-
static_cast<int>(req->result),
256-
req_wrap->encoding())
257-
.ToLocal(&js_array)) {
251+
ResolveOrReject(req_wrap.get(), [&]() {
252+
MaybeLocal<Array> js_array = DirentListToArray(
253+
env, dir->dirents, static_cast<int>(req->result), req_wrap->encoding());
258254
// Clear libuv resources *before* delivering results to JS land because
259-
// that can schedule another operation on the same uv_dir_t. Ditto below.
255+
// that can schedule another operation on the same uv_dir_t.
260256
after.Clear();
261-
CHECK(try_catch.CanContinue());
262-
return req_wrap->Reject(try_catch.Exception());
263-
}
264-
265-
after.Clear();
266-
req_wrap->Resolve(js_array);
257+
return js_array;
258+
});
267259
}
268260

269261

‎src/node_file-inl.h‎

Lines changed: 27 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -424,6 +424,29 @@ int SyncCallAndThrowOnError(Environment* env,
424424
return SyncCallAndThrowIf(is_uv_error, env, req_wrap, fn, args...);
425425
}
426426

427+
// Delivers the value produced by `produce`, or the exception it threw, to
428+
// `req_wrap`. The TryCatch is left before calling into JS: exceptions thrown
429+
// by the callback, or by the tick queue drained afterwards, must not be caught
430+
// by it.
431+
template <typename Fn>
432+
void ResolveOrReject(FSReqBase* req_wrap, Fn&& produce) {
433+
v8::Isolate* isolate = req_wrap->env()->isolate();
434+
v8::Local<v8::Value> value;
435+
v8::Local<v8::Value> error;
436+
{
437+
v8::TryCatch try_catch(isolate);
438+
if (!produce().ToLocal(&value)) {
439+
CHECK(try_catch.CanContinue());
440+
error = try_catch.Exception();
441+
}
442+
}
443+
if (error.IsEmpty()) {
444+
req_wrap->Resolve(value);
445+
} else {
446+
req_wrap->Reject(error);
447+
}
448+
}
449+
427450
// Closes a file created by mkstemp() whose descriptor cannot be handed to JS.
428451
inline void CloseMkstempFd(int fd) {
429452
uv_fs_t close_req;
@@ -464,28 +487,16 @@ v8::MaybeLocal<v8::Value> MkstempResult(v8::Isolate* isolate,
464487
}
465488

466489
// Resolves `req_wrap` with the result of mkstemp(), or rejects it with the
467-
// exception thrown while building it. The TryCatch is left before calling
468-
// into JS.
490+
// exception thrown while building it.
469491
template <typename EncodePath, typename MakeFile>
470492
void ResolveMkstemp(FSReqBase* req_wrap,
471493
int fd,
472494
EncodePath&& encode_path,
473495
MakeFile&& make_file) {
474496
v8::Isolate* isolate = req_wrap->env()->isolate();
475-
v8::Local<v8::Value> result;
476-
v8::Local<v8::Value> error;
477-
{
478-
v8::TryCatch try_catch(isolate);
479-
if (!MkstempResult(isolate, fd, encode_path, make_file).ToLocal(&result)) {
480-
CHECK(try_catch.CanContinue());
481-
error = try_catch.Exception();
482-
}
483-
}
484-
if (error.IsEmpty()) {
485-
req_wrap->Resolve(result);
486-
} else {
487-
req_wrap->Reject(error);
488-
}
497+
ResolveOrReject(req_wrap, [&]() {
498+
return MkstempResult(isolate, fd, encode_path, make_file);
499+
});
489500
}
490501

491502
} // namespace fs

‎src/node_file.cc‎

Lines changed: 16 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -939,16 +939,10 @@ void AfterMkdirp(uv_fs_t* req) {
939939
std::string first_path(req_wrap->continuation_data()->first_path());
940940
if (first_path.empty())
941941
return req_wrap->Resolve(Undefined(req_wrap->env()->isolate()));
942-
Local<Value> path;
943-
TryCatch try_catch(req_wrap->env()->isolate());
944-
if (!StringBytes::Encode(req_wrap->env()->isolate(),
945-
first_path.c_str(),
946-
req_wrap->encoding())
947-
.ToLocal(&path)) {
948-
CHECK(try_catch.CanContinue());
949-
return req_wrap->Reject(try_catch.Exception());
950-
}
951-
return req_wrap->Resolve(path);
942+
ResolveOrReject(req_wrap, [&]() {
943+
return StringBytes::Encode(
944+
req_wrap->env()->isolate(), first_path.c_str(), req_wrap->encoding());
945+
});
952946
}
953947
}
954948

@@ -957,19 +951,11 @@ void AfterStringPath(uv_fs_t* req) {
957951
FSReqAfterScope after(req_wrap, req);
958952
FS_ASYNC_TRACE_END1(
959953
req->fs_type, req_wrap, "result", static_cast<int>(req->result))
960-
MaybeLocal<Value> link;
961-
962954
if (after.Proceed()) {
963-
TryCatch try_catch(req_wrap->env()->isolate());
964-
link = StringBytes::Encode(
965-
req_wrap->env()->isolate(), req->path, req_wrap->encoding());
966-
if (link.IsEmpty()) {
967-
CHECK(try_catch.CanContinue());
968-
req_wrap->Reject(try_catch.Exception());
969-
} else {
970-
Local<Value> val;
971-
if (link.ToLocal(&val)) req_wrap->Resolve(val);
972-
}
955+
ResolveOrReject(req_wrap, [&]() {
956+
return StringBytes::Encode(
957+
req_wrap->env()->isolate(), req->path, req_wrap->encoding());
958+
});
973959
}
974960
}
975961

@@ -978,20 +964,12 @@ void AfterStringPtr(uv_fs_t* req) {
978964
FSReqAfterScope after(req_wrap, req);
979965
FS_ASYNC_TRACE_END1(
980966
req->fs_type, req_wrap, "result", static_cast<int>(req->result))
981-
MaybeLocal<Value> link;
982-
983967
if (after.Proceed()) {
984-
TryCatch try_catch(req_wrap->env()->isolate());
985-
link = StringBytes::Encode(req_wrap->env()->isolate(),
986-
static_cast<const char*>(req->ptr),
987-
req_wrap->encoding());
988-
if (link.IsEmpty()) {
989-
CHECK(try_catch.CanContinue());
990-
req_wrap->Reject(try_catch.Exception());
991-
} else {
992-
Local<Value> val;
993-
if (link.ToLocal(&val)) req_wrap->Resolve(val);
994-
}
968+
ResolveOrReject(req_wrap, [&]() {
969+
return StringBytes::Encode(req_wrap->env()->isolate(),
970+
static_cast<const char*>(req->ptr),
971+
req_wrap->encoding());
972+
});
995973
}
996974
}
997975

@@ -2746,14 +2724,9 @@ class ReadDirRecursiveRequest {
27462724
walk_.error_path().c_str()));
27472725
}
27482726

2749-
Local<Value> value;
2750-
TryCatch try_catch(isolate);
2751-
if (!MarshalRecursiveReadDir(isolate, walk_, encoding_, with_types_)
2752-
.ToLocal(&value)) {
2753-
CHECK(try_catch.CanContinue());
2754-
return req_wrap->Reject(try_catch.Exception());
2755-
}
2756-
req_wrap->Resolve(value);
2727+
ResolveOrReject(req_wrap.get(), [&]() {
2728+
return MarshalRecursiveReadDir(isolate, walk_, encoding_, with_types_);
2729+
});
27572730
}
27582731

27592732
private:

‎test/cctest/test_node_file.cc‎

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,84 @@
1+
#include "base_object-inl.h"
2+
#include "env-inl.h"
3+
#include "gtest/gtest.h"
4+
#include "node_file-inl.h"
5+
#include "node_test_fixture.h"
6+
7+
using node::fs::BindingData;
8+
using node::fs::FSReqBase;
9+
using v8::Exception;
10+
using v8::FunctionCallbackInfo;
11+
using v8::Global;
12+
using v8::HandleScope;
13+
using v8::Local;
14+
using v8::MaybeLocal;
15+
using v8::Object;
16+
using v8::ObjectTemplate;
17+
using v8::String;
18+
using v8::TryCatch;
19+
using v8::Value;
20+
21+
class NodeFileTest : public EnvironmentTestFixture {};
22+
23+
class TestFSReq final : public FSReqBase {
24+
public:
25+
TestFSReq(BindingData* binding_data, Local<Object> object)
26+
: FSReqBase(binding_data,
27+
object,
28+
node::AsyncWrap::PROVIDER_FSREQCALLBACK,
29+
false) {}
30+
31+
void Resolve(Local<Value> value) override {
32+
resolved_.Reset(env()->isolate(), value);
33+
}
34+
35+
void Reject(Local<Value> error) override {
36+
rejected_.Reset(env()->isolate(), error);
37+
}
38+
39+
void ResolveStat(const uv_stat_t*) override {}
40+
void ResolveStatFs(const uv_statfs_t*) override {}
41+
void SetReturnValue(const FunctionCallbackInfo<Value>&) override {}
42+
43+
SET_MEMORY_INFO_NAME(TestFSReq)
44+
SET_SELF_SIZE(TestFSReq)
45+
46+
Local<Value> resolved() const { return resolved_.Get(env()->isolate()); }
47+
48+
Local<Value> rejected() const { return rejected_.Get(env()->isolate()); }
49+
50+
private:
51+
Global<Value> resolved_;
52+
Global<Value> rejected_;
53+
};
54+
55+
// The reject branch cannot be reached from JavaScript, so test it in cctest.
56+
TEST_F(NodeFileTest, ResolveOrRejectRejectsException) {
57+
const HandleScope handle_scope(isolate_);
58+
const Argv argv;
59+
Env env{handle_scope, argv};
60+
node::LoadEnvironment(*env, "require('fs');");
61+
62+
BindingData* binding_data =
63+
(*env)->principal_realm()->GetBindingData<BindingData>();
64+
ASSERT_NE(binding_data, nullptr);
65+
66+
Local<ObjectTemplate> object_template = ObjectTemplate::New(isolate_);
67+
object_template->SetInternalFieldCount(FSReqBase::kInternalFieldCount);
68+
Local<Object> object =
69+
object_template->NewInstance(env.context()).ToLocalChecked();
70+
auto req = node::MakeDetachedBaseObject<TestFSReq>(binding_data, object);
71+
Local<Value> expected = Exception::Error(
72+
String::NewFromUtf8Literal(isolate_, "expected exception"));
73+
TryCatch try_catch(isolate_);
74+
75+
node::fs::ResolveOrReject(req.get(), [&]() -> MaybeLocal<Value> {
76+
isolate_->ThrowException(expected);
77+
return {};
78+
});
79+
80+
EXPECT_TRUE(req->resolved().IsEmpty());
81+
ASSERT_FALSE(req->rejected().IsEmpty());
82+
EXPECT_TRUE(req->rejected()->StrictEquals(expected));
83+
EXPECT_FALSE(try_catch.HasCaught());
84+
}

‎test/parallel/test-fs-callback-throw.js‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
'use strict';
2+
// Refs: https://github.com/nodejs/node/issues/65667
3+
// Exceptions thrown after an fs operation completes must reach
4+
// 'uncaughtException' instead of being swallowed.
5+
const common = require('../common');
6+
const tmpdir = require('../common/tmpdir');
7+
const assert = require('assert');
8+
const fs = require('fs');
9+
const path = require('path');
10+
11+
tmpdir.refresh();
12+
const dir = tmpdir.path;
13+
const file = path.join(dir, 'file');
14+
const link = path.join(dir, 'link');
15+
fs.writeFileSync(file, '');
16+
17+
// The callback throws.
18+
const callbackCases = {
19+
'mkdtemp': (cb) => fs.mkdtemp(path.join(dir, 'x-'), cb),
20+
'realpath.native': (cb) => fs.realpath.native(dir, cb),
21+
'mkdir recursive': (cb) => fs.mkdir(path.join(dir, 'a', 'b'), { recursive: true }, cb),
22+
'readdir recursive': (cb) => fs.readdir(dir, { recursive: true }, cb),
23+
};
24+
if (common.canCreateSymLink()) {
25+
fs.symlinkSync(file, link);
26+
callbackCases.readlink = (cb) => fs.readlink(link, cb);
27+
}
28+
29+
// A nextTick callback scheduled after the promise settles throws.
30+
const promiseCases = {
31+
'promises.mkdtemp': () => fs.promises.mkdtemp(path.join(dir, 'p-')),
32+
'dir.read': async () => {
33+
const d = await fs.promises.opendir(dir);
34+
await d.read();
35+
process.nextTick(() => d.closeSync());
36+
},
37+
};
38+
39+
const cases = [
40+
...Object.entries(callbackCases).map(([name, run]) => [name, () => {
41+
run(common.mustSucceed(() => { throw new Error(name); }));
42+
}]),
43+
...Object.entries(promiseCases).map(([name, run]) => [name, async () => {
44+
await run();
45+
process.nextTick(() => { throw new Error(name); });
46+
}]),
47+
];
48+
49+
let current;
50+
process.on('uncaughtException', common.mustCall((err) => {
51+
assert.strictEqual(err.message, current);
52+
next();
53+
}, cases.length));
54+
55+
function next() {
56+
const entry = cases.shift();
57+
if (entry === undefined) return;
58+
current = entry[0];
59+
entry[1]();
60+
}
61+
62+
next();

0 commit comments

Comments
 (0)