Skip to content

Commit a76b761

Browse files
committed
src: fixup fs SyncCall to propagate errors correctly
Propagate errors correctly in the SyncCall utility. Previously the test case would trigger a crash.
1 parent 2ff6c7e commit a76b761

File tree

4 files changed

+72
-15
lines changed

4 files changed

+72
-15
lines changed

src/node_file-inl.h

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -344,7 +344,7 @@ 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,
347+
v8::Maybe<int> SyncCall(Environment* env, v8::Local<v8::Value> ctx,
348348
FSReqWrapSync* req_wrap, const char* syscall,
349349
Func fn, Args... args) {
350350
env->PrintSyncTrace();
@@ -353,14 +353,16 @@ int SyncCall(Environment* env, v8::Local<v8::Value> ctx,
353353
v8::Local<v8::Context> context = env->context();
354354
v8::Local<v8::Object> ctx_obj = ctx.As<v8::Object>();
355355
v8::Isolate* isolate = env->isolate();
356-
ctx_obj->Set(context,
356+
if (ctx_obj->Set(context,
357357
env->errno_string(),
358-
v8::Integer::New(isolate, err)).Check();
359-
ctx_obj->Set(context,
358+
v8::Integer::New(isolate, err)).IsNothing() ||
359+
ctx_obj->Set(context,
360360
env->syscall_string(),
361-
OneByteString(isolate, syscall)).Check();
361+
OneByteString(isolate, syscall)).IsNothing()) {
362+
return v8::Nothing<int>();
363+
}
362364
}
363-
return err;
365+
return v8::Just(err);
364366
}
365367

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

src/node_file.cc

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2239,8 +2239,12 @@ 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, args[4], &req_wrap_sync, "open",
2244+
uv_fs_open, *path, flags, mode).To(&result)) {
2245+
// v8 error occurred while setting the context. propagate!
2246+
return;
2247+
}
22442248
FS_SYNC_TRACE_END(open);
22452249
if (result < 0) {
22462250
return; // syscall failed, no need to continue, error info is in ctx
@@ -2356,8 +2360,12 @@ static void WriteBuffer(const FunctionCallbackInfo<Value>& args) {
23562360
CHECK_EQ(argc, 7);
23572361
FSReqWrapSync req_wrap_sync;
23582362
FS_SYNC_TRACE_BEGIN(write);
2359-
int bytesWritten = SyncCall(env, args[6], &req_wrap_sync, "write",
2360-
uv_fs_write, fd, &uvbuf, 1, pos);
2363+
int bytesWritten;
2364+
if (!SyncCall(env, args[6], &req_wrap_sync, "write",
2365+
uv_fs_write, fd, &uvbuf, 1, pos).To(&bytesWritten)) {
2366+
FS_SYNC_TRACE_END(write, "bytesWritten", 0);
2367+
return;
2368+
}
23612369
FS_SYNC_TRACE_END(write, "bytesWritten", bytesWritten);
23622370
args.GetReturnValue().Set(bytesWritten);
23632371
}
@@ -2515,8 +2523,12 @@ static void WriteString(const FunctionCallbackInfo<Value>& args) {
25152523
uv_buf_t uvbuf = uv_buf_init(buf, len);
25162524
FSReqWrapSync req_wrap_sync("write");
25172525
FS_SYNC_TRACE_BEGIN(write);
2518-
int bytesWritten = SyncCall(env, args[5], &req_wrap_sync, "write",
2519-
uv_fs_write, fd, &uvbuf, 1, pos);
2526+
int bytesWritten;
2527+
if (!SyncCall(env, args[5], &req_wrap_sync, "write",
2528+
uv_fs_write, fd, &uvbuf, 1, pos).To(&bytesWritten)) {
2529+
FS_SYNC_TRACE_END(write, "bytesWritten", 0);
2530+
return;
2531+
}
25202532
FS_SYNC_TRACE_END(write, "bytesWritten", bytesWritten);
25212533
args.GetReturnValue().Set(bytesWritten);
25222534
}

src/node_file.h

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -512,9 +512,9 @@ 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, v8::Local<v8::Value> ctx,
516+
FSReqWrapSync* req_wrap, const char* syscall,
517+
Func fn, Args... args);
518518

519519
// Similar to SyncCall but throws immediately if there is an error.
520520
template <typename Predicate, typename Func, typename... Args>
Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
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+
Object.defineProperty(Object.prototype, 'errno', {
31+
__proto__: null,
32+
set() {
33+
throw new Error('error');
34+
},
35+
get() { return 0; }
36+
});
37+
38+
const fd = openSync(path);
39+
chmodSync(path, 0o600);
40+
41+
throws(() => writeSync(fd, 'test'), {
42+
message: 'error',
43+
});

0 commit comments

Comments
 (0)