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]>

Author: jasnell

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 <typename Func, typename... Args>
-int SyncCall(Environment* env, v8::Local<v8::Value> ctx,
-             FSReqWrapSync* req_wrap, const char* syscall,
-             Func fn, Args... args) {
+v8::Maybe<int> SyncCall(Environment* env,
+                        v8::Local<v8::Value> 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<v8::Context> context = env->context();
     v8::Local<v8::Object> ctx_obj = ctx.As<v8::Object>();
     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<int>();
+    }
   }
-  return err;
+  return v8::Just(err);
 }
 
 // Similar to SyncCall but throws immediately if there is an error.

src/node_file.cc

@@ -2239,8 +2239,19 @@ static void OpenFileHandle(const FunctionCallbackInfo<Value>& 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<Value>& 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<Value>& 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);
   }

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 <typename Func, typename... Args>
-inline int SyncCall(Environment* env, v8::Local<v8::Value> ctx,
-                    FSReqWrapSync* req_wrap, const char* syscall,
-                    Func fn, Args... args);
+inline v8::Maybe<int> SyncCall(Environment* env,
+                               v8::Local<v8::Value> ctx,
+                               FSReqWrapSync* req_wrap,
+                               const char* syscall,
+                               Func fn,
+                               Args... args);
 
 // Similar to SyncCall but throws immediately if there is an error.
 template <typename Predicate, typename Func, typename... Args>

test/parallel/test-fs-writesync-crash.js

@@ -0,0 +1,42 @@
+'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.
+const error = new Error();
+Object.defineProperty(Object.prototype, 'errno', {
+  __proto__: null,
+  set() {
+    throw error;
+  },
+  get() { return 0; }
+});
+
+const fd = openSync(path);
+chmodSync(path, 0o600);
+
+throws(() => writeSync(fd, 'test'), error);