From 75ebfdd2db1c65e3fdc63316c8cb10eb93b7d4e1 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Tue, 1 Apr 2025 09:53:41 -0700 Subject: [PATCH 1/2] src: fixup fs SyncCall to propagate errors correctly Propagate errors correctly in the SyncCall utility. Previously the test case would trigger a crash. --- src/node_file-inl.h | 26 ++++++++----- src/node_file.cc | 47 +++++++++++++++++++++--- src/node_file.h | 9 +++-- test/parallel/test-fs-writesync-crash.js | 43 ++++++++++++++++++++++ 4 files changed, 106 insertions(+), 19 deletions(-) create mode 100644 test/parallel/test-fs-writesync-crash.js diff --git a/src/node_file-inl.h b/src/node_file-inl.h index 29608b8c96b70f..ac74ab8f27f53b 100644 --- a/src/node_file-inl.h +++ b/src/node_file-inl.h @@ -344,23 +344,29 @@ FSReqBase* AsyncCall(Environment* env, // creating an error in the C++ land. // ctx must be checked using value->IsObject() before being passed. template -int SyncCall(Environment* env, v8::Local ctx, - FSReqWrapSync* req_wrap, const char* syscall, - Func fn, Args... args) { +v8::Maybe SyncCall(Environment* env, + v8::Local ctx, + FSReqWrapSync* req_wrap, + const char* syscall, + Func fn, + Args... args) { env->PrintSyncTrace(); int err = fn(env->event_loop(), &(req_wrap->req), args..., nullptr); if (err < 0) { v8::Local context = env->context(); v8::Local ctx_obj = ctx.As(); v8::Isolate* isolate = env->isolate(); - ctx_obj->Set(context, - env->errno_string(), - v8::Integer::New(isolate, err)).Check(); - ctx_obj->Set(context, - env->syscall_string(), - OneByteString(isolate, syscall)).Check(); + if (ctx_obj + ->Set(context, env->errno_string(), v8::Integer::New(isolate, err)) + .IsNothing() || + ctx_obj + ->Set( + context, env->syscall_string(), OneByteString(isolate, syscall)) + .IsNothing()) { + return v8::Nothing(); + } } - return err; + return v8::Just(err); } // Similar to SyncCall but throws immediately if there is an error. diff --git a/src/node_file.cc b/src/node_file.cc index d7cca08521c1c6..2534a29ba82996 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -2239,8 +2239,19 @@ static void OpenFileHandle(const FunctionCallbackInfo& args) { CHECK_EQ(argc, 5); FSReqWrapSync req_wrap_sync; FS_SYNC_TRACE_BEGIN(open); - int result = SyncCall(env, args[4], &req_wrap_sync, "open", - uv_fs_open, *path, flags, mode); + int result; + if (!SyncCall(env, + args[4], + &req_wrap_sync, + "open", + uv_fs_open, + *path, + flags, + mode) + .To(&result)) { + // v8 error occurred while setting the context. propagate! + return; + } FS_SYNC_TRACE_END(open); if (result < 0) { return; // syscall failed, no need to continue, error info is in ctx @@ -2356,8 +2367,20 @@ static void WriteBuffer(const FunctionCallbackInfo& args) { CHECK_EQ(argc, 7); FSReqWrapSync req_wrap_sync; FS_SYNC_TRACE_BEGIN(write); - int bytesWritten = SyncCall(env, args[6], &req_wrap_sync, "write", - uv_fs_write, fd, &uvbuf, 1, pos); + int bytesWritten; + if (!SyncCall(env, + args[6], + &req_wrap_sync, + "write", + uv_fs_write, + fd, + &uvbuf, + 1, + pos) + .To(&bytesWritten)) { + FS_SYNC_TRACE_END(write, "bytesWritten", 0); + return; + } FS_SYNC_TRACE_END(write, "bytesWritten", bytesWritten); args.GetReturnValue().Set(bytesWritten); } @@ -2515,8 +2538,20 @@ static void WriteString(const FunctionCallbackInfo& args) { uv_buf_t uvbuf = uv_buf_init(buf, len); FSReqWrapSync req_wrap_sync("write"); FS_SYNC_TRACE_BEGIN(write); - int bytesWritten = SyncCall(env, args[5], &req_wrap_sync, "write", - uv_fs_write, fd, &uvbuf, 1, pos); + int bytesWritten; + if (!SyncCall(env, + args[5], + &req_wrap_sync, + "write", + uv_fs_write, + fd, + &uvbuf, + 1, + pos) + .To(&bytesWritten)) { + FS_SYNC_TRACE_END(write, "bytesWritten", 0); + return; + } FS_SYNC_TRACE_END(write, "bytesWritten", bytesWritten); args.GetReturnValue().Set(bytesWritten); } diff --git a/src/node_file.h b/src/node_file.h index bdad1ae25f4892..224fa6f7ade375 100644 --- a/src/node_file.h +++ b/src/node_file.h @@ -512,9 +512,12 @@ inline FSReqBase* AsyncCall(Environment* env, // creating an error in the C++ land. // ctx must be checked using value->IsObject() before being passed. template -inline int SyncCall(Environment* env, v8::Local ctx, - FSReqWrapSync* req_wrap, const char* syscall, - Func fn, Args... args); +inline v8::Maybe SyncCall(Environment* env, + v8::Local ctx, + FSReqWrapSync* req_wrap, + const char* syscall, + Func fn, + Args... args); // Similar to SyncCall but throws immediately if there is an error. template diff --git a/test/parallel/test-fs-writesync-crash.js b/test/parallel/test-fs-writesync-crash.js new file mode 100644 index 00000000000000..1a0b35c505aaf1 --- /dev/null +++ b/test/parallel/test-fs-writesync-crash.js @@ -0,0 +1,43 @@ +'use strict'; + +require('../common'); + +const { + writeSync, + writeFileSync, + chmodSync, + openSync, +} = require('node:fs'); + +const { + throws, +} = require('node:assert'); + +// If a file's mode change after it is opened but before it is written to, +// and the Object.prototype is manipulated to throw an error when the errno +// or fd property is set or accessed, then the writeSync call would crash +// the process. This test verifies that the error is properly propagated +// instead. + +const tmpdir = require('../common/tmpdir'); +console.log(tmpdir.path); +tmpdir.refresh(); +const path = `${tmpdir.path}/foo`; +writeFileSync(path, ''); + +// Do this after calling tmpdir.refresh() or that call will fail +// before we get to the part we want to test. +Object.defineProperty(Object.prototype, 'errno', { + __proto__: null, + set() { + throw new Error('error'); + }, + get() { return 0; } +}); + +const fd = openSync(path); +chmodSync(path, 0o600); + +throws(() => writeSync(fd, 'test'), { + message: 'error', +}); From 3ce476337c45ef893db68fbf70b55abd9a0fc866 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Wed, 2 Apr 2025 03:39:54 -0700 Subject: [PATCH 2/2] Update test/parallel/test-fs-writesync-crash.js Co-authored-by: Antoine du Hamel --- test/parallel/test-fs-writesync-crash.js | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/test/parallel/test-fs-writesync-crash.js b/test/parallel/test-fs-writesync-crash.js index 1a0b35c505aaf1..263177b4823d39 100644 --- a/test/parallel/test-fs-writesync-crash.js +++ b/test/parallel/test-fs-writesync-crash.js @@ -27,10 +27,11 @@ writeFileSync(path, ''); // Do this after calling tmpdir.refresh() or that call will fail // before we get to the part we want to test. +const error = new Error(); Object.defineProperty(Object.prototype, 'errno', { __proto__: null, set() { - throw new Error('error'); + throw error; }, get() { return 0; } }); @@ -38,6 +39,4 @@ Object.defineProperty(Object.prototype, 'errno', { const fd = openSync(path); chmodSync(path, 0o600); -throws(() => writeSync(fd, 'test'), { - message: 'error', -}); +throws(() => writeSync(fd, 'test'), error);