Skip to content

Commit e144b69

Browse files
jasnellRafaelGSS
authored andcommitted
src: fixup fs SyncCall to propagate errors correctly
Propagate errors correctly in the SyncCall utility. Previously the test case would trigger a crash. PR-URL: #57711 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
1 parent f58c120 commit e144b69

File tree

4 files changed

+105
-19
lines changed

4 files changed

+105
-19
lines changed

src/node_file-inl.h

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -344,23 +344,29 @@ FSReqBase* AsyncCall(Environment* env,
344344
// creating an error in the C++ land.
345345
// ctx must be checked using value->IsObject() before being passed.
346346
template <typename Func, typename... Args>
347-
int SyncCall(Environment* env, v8::Local<v8::Value> ctx,
348-
FSReqWrapSync* req_wrap, const char* syscall,
349-
Func fn, Args... args) {
347+
v8::Maybe<int> SyncCall(Environment* env,
348+
v8::Local<v8::Value> ctx,
349+
FSReqWrapSync* req_wrap,
350+
const char* syscall,
351+
Func fn,
352+
Args... args) {
350353
env->PrintSyncTrace();
351354
int err = fn(env->event_loop(), &(req_wrap->req), args..., nullptr);
352355
if (err < 0) {
353356
v8::Local<v8::Context> context = env->context();
354357
v8::Local<v8::Object> ctx_obj = ctx.As<v8::Object>();
355358
v8::Isolate* isolate = env->isolate();
356-
ctx_obj->Set(context,
357-
env->errno_string(),
358-
v8::Integer::New(isolate, err)).Check();
359-
ctx_obj->Set(context,
360-
env->syscall_string(),
361-
OneByteString(isolate, syscall)).Check();
359+
if (ctx_obj
360+
->Set(context, env->errno_string(), v8::Integer::New(isolate, err))
361+
.IsNothing() ||
362+
ctx_obj
363+
->Set(
364+
context, env->syscall_string(), OneByteString(isolate, syscall))
365+
.IsNothing()) {
366+
return v8::Nothing<int>();
367+
}
362368
}
363-
return err;
369+
return v8::Just(err);
364370
}
365371

366372
// Similar to SyncCall but throws immediately if there is an error.

src/node_file.cc

Lines changed: 41 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2239,8 +2239,19 @@ static void OpenFileHandle(const FunctionCallbackInfo<Value>& args) {
22392239
CHECK_EQ(argc, 5);
22402240
FSReqWrapSync req_wrap_sync;
22412241
FS_SYNC_TRACE_BEGIN(open);
2242-
int result = SyncCall(env, args[4], &req_wrap_sync, "open",
2243-
uv_fs_open, *path, flags, mode);
2242+
int result;
2243+
if (!SyncCall(env,
2244+
args[4],
2245+
&req_wrap_sync,
2246+
"open",
2247+
uv_fs_open,
2248+
*path,
2249+
flags,
2250+
mode)
2251+
.To(&result)) {
2252+
// v8 error occurred while setting the context. propagate!
2253+
return;
2254+
}
22442255
FS_SYNC_TRACE_END(open);
22452256
if (result < 0) {
22462257
return; // syscall failed, no need to continue, error info is in ctx
@@ -2356,8 +2367,20 @@ static void WriteBuffer(const FunctionCallbackInfo<Value>& args) {
23562367
CHECK_EQ(argc, 7);
23572368
FSReqWrapSync req_wrap_sync;
23582369
FS_SYNC_TRACE_BEGIN(write);
2359-
int bytesWritten = SyncCall(env, args[6], &req_wrap_sync, "write",
2360-
uv_fs_write, fd, &uvbuf, 1, pos);
2370+
int bytesWritten;
2371+
if (!SyncCall(env,
2372+
args[6],
2373+
&req_wrap_sync,
2374+
"write",
2375+
uv_fs_write,
2376+
fd,
2377+
&uvbuf,
2378+
1,
2379+
pos)
2380+
.To(&bytesWritten)) {
2381+
FS_SYNC_TRACE_END(write, "bytesWritten", 0);
2382+
return;
2383+
}
23612384
FS_SYNC_TRACE_END(write, "bytesWritten", bytesWritten);
23622385
args.GetReturnValue().Set(bytesWritten);
23632386
}
@@ -2515,8 +2538,20 @@ static void WriteString(const FunctionCallbackInfo<Value>& args) {
25152538
uv_buf_t uvbuf = uv_buf_init(buf, len);
25162539
FSReqWrapSync req_wrap_sync("write");
25172540
FS_SYNC_TRACE_BEGIN(write);
2518-
int bytesWritten = SyncCall(env, args[5], &req_wrap_sync, "write",
2519-
uv_fs_write, fd, &uvbuf, 1, pos);
2541+
int bytesWritten;
2542+
if (!SyncCall(env,
2543+
args[5],
2544+
&req_wrap_sync,
2545+
"write",
2546+
uv_fs_write,
2547+
fd,
2548+
&uvbuf,
2549+
1,
2550+
pos)
2551+
.To(&bytesWritten)) {
2552+
FS_SYNC_TRACE_END(write, "bytesWritten", 0);
2553+
return;
2554+
}
25202555
FS_SYNC_TRACE_END(write, "bytesWritten", bytesWritten);
25212556
args.GetReturnValue().Set(bytesWritten);
25222557
}

src/node_file.h

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -512,9 +512,12 @@ inline FSReqBase* AsyncCall(Environment* env,
512512
// creating an error in the C++ land.
513513
// ctx must be checked using value->IsObject() before being passed.
514514
template <typename Func, typename... Args>
515-
inline int SyncCall(Environment* env, v8::Local<v8::Value> ctx,
516-
FSReqWrapSync* req_wrap, const char* syscall,
517-
Func fn, Args... args);
515+
inline v8::Maybe<int> SyncCall(Environment* env,
516+
v8::Local<v8::Value> ctx,
517+
FSReqWrapSync* req_wrap,
518+
const char* syscall,
519+
Func fn,
520+
Args... args);
518521

519522
// Similar to SyncCall but throws immediately if there is an error.
520523
template <typename Predicate, typename Func, typename... Args>
Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
'use strict';
2+
3+
require('../common');
4+
5+
const {
6+
writeSync,
7+
writeFileSync,
8+
chmodSync,
9+
openSync,
10+
} = require('node:fs');
11+
12+
const {
13+
throws,
14+
} = require('node:assert');
15+
16+
// If a file's mode change after it is opened but before it is written to,
17+
// and the Object.prototype is manipulated to throw an error when the errno
18+
// or fd property is set or accessed, then the writeSync call would crash
19+
// the process. This test verifies that the error is properly propagated
20+
// instead.
21+
22+
const tmpdir = require('../common/tmpdir');
23+
console.log(tmpdir.path);
24+
tmpdir.refresh();
25+
const path = `${tmpdir.path}/foo`;
26+
writeFileSync(path, '');
27+
28+
// Do this after calling tmpdir.refresh() or that call will fail
29+
// before we get to the part we want to test.
30+
const error = new Error();
31+
Object.defineProperty(Object.prototype, 'errno', {
32+
__proto__: null,
33+
set() {
34+
throw error;
35+
},
36+
get() { return 0; }
37+
});
38+
39+
const fd = openSync(path);
40+
chmodSync(path, 0o600);
41+
42+
throws(() => writeSync(fd, 'test'), error);

0 commit comments

Comments
 (0)