fs: fix potential segfault in async calls

When the async uv_fs_* call errors out synchronously in AsyncDestCall,
the after callbacks (e.g. AfterNoArgs) would delete the req_wrap
in FSReqAfterScope, and AsyncDestCall would set those req_wrap to
nullptr afterwards. But when it returns to the top-layer bindings,
the bindings all call `req_wrap->SetReturnValue()` again without
checking if `req_wrap` is nullptr, causing a segfault.

This has not been caught in any of the tests because we usually do a
lot of argument checking in the JS layer before invoking the uv_fs_*
functions, so it's rare to get a synchronous error from them.

Currently we never need the binding to return the wrap to JS layer,
so we can just call `req_wrap->SetReturnValue()` to return undefined
for normal FSReqWrap and the promise for FSReqPromise in AsyncDestCall
instead of doing this in the top-level bindings.

PR-URL: https://github.com/nodejs/node/pull/18811
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

1 files changed, 7 insertions, 34 deletions

diff --git a/src/node_file.cc b/src/node_file.cc
index 37b0b4e3b9..c9c7570779 100644
--- a/src/node_file.cc
+++ b/src/node_file.cc
@@ -516,6 +516,7 @@ class fs_req_wrap {
   DISALLOW_COPY_AND_ASSIGN(fs_req_wrap);
 };
 
+// Returns nullptr if the operation fails from the start.
 template <typename Func, typename... Args>
 inline FSReqBase* AsyncDestCall(Environment* env,
     FSReqBase* req_wrap,
@@ -530,16 +531,16 @@ inline FSReqBase* AsyncDestCall(Environment* env,
     uv_fs_t* uv_req = req_wrap->req();
     uv_req->result = err;
     uv_req->path = nullptr;
-    after(uv_req);
+    after(uv_req);  // after may delete req_wrap if there is an error
     req_wrap = nullptr;
+  } else {
+    req_wrap->SetReturnValue(args);
   }
 
-  if (req_wrap != nullptr) {
-    args.GetReturnValue().Set(req_wrap->persistent());
-  }
   return req_wrap;
 }
 
+// Returns nullptr if the operation fails from the start.
 template <typename Func, typename... Args>
 inline FSReqBase* AsyncCall(Environment* env,
     FSReqBase* req_wrap,
@@ -618,7 +619,6 @@ void Access(const FunctionCallbackInfo<Value>& args) {
   if (req_wrap != nullptr) {  // access(path, mode, req)
     AsyncCall(env, req_wrap, args, "access", UTF8, AfterNoArgs,
               uv_fs_access, *path, mode);
-    req_wrap->SetReturnValue(args);
   } else {  // access(path, mode, undefined, ctx)
     CHECK_EQ(argc, 4);
     fs_req_wrap req_wrap;
@@ -640,7 +640,6 @@ void Close(const FunctionCallbackInfo<Value>& args) {
   if (req_wrap != nullptr) {  // close(fd, req)
     AsyncCall(env, req_wrap, args, "close", UTF8, AfterNoArgs,
               uv_fs_close, fd);
-    req_wrap->SetReturnValue(args);
   } else {  // close(fd, undefined, ctx)
     CHECK_EQ(argc, 3);
     fs_req_wrap req_wrap;
@@ -749,7 +748,6 @@ static void Stat(const FunctionCallbackInfo<Value>& args) {
   if (req_wrap != nullptr) {  // stat(path, req)
     AsyncCall(env, req_wrap, args, "stat", UTF8, AfterStat,
               uv_fs_stat, *path);
-    req_wrap->SetReturnValue(args);
   } else {  // stat(path, undefined, ctx)
     CHECK_EQ(argc, 3);
     fs_req_wrap req_wrap;
@@ -774,7 +772,6 @@ static void LStat(const FunctionCallbackInfo<Value>& args) {
   if (req_wrap != nullptr) {  // lstat(path, req)
     AsyncCall(env, req_wrap, args, "lstat", UTF8, AfterStat,
               uv_fs_lstat, *path);
-    req_wrap->SetReturnValue(args);
   } else {  // lstat(path, undefined, ctx)
     CHECK_EQ(argc, 3);
     fs_req_wrap req_wrap;
@@ -799,7 +796,6 @@ static void FStat(const FunctionCallbackInfo<Value>& args) {
   if (req_wrap != nullptr) {  // fstat(fd, req)
     AsyncCall(env, req_wrap, args, "fstat", UTF8, AfterStat,
               uv_fs_fstat, fd);
-    req_wrap->SetReturnValue(args);
   } else {  // fstat(fd, undefined, ctx)
     CHECK_EQ(argc, 3);
     fs_req_wrap req_wrap;
@@ -853,7 +849,6 @@ static void Link(const FunctionCallbackInfo<Value>& args) {
   if (req_wrap != nullptr) {  // link(src, dest, req)
     AsyncDestCall(env, req_wrap, args, "link", *dest, dest.length(), UTF8,
                   AfterNoArgs, uv_fs_link, *src, *dest);
-    req_wrap->SetReturnValue(args);
   } else {  // link(src, dest)
     CHECK_EQ(argc, 4);
     fs_req_wrap req;
@@ -877,7 +872,6 @@ static void ReadLink(const FunctionCallbackInfo<Value>& args) {
   if (req_wrap != nullptr) {  // readlink(path, encoding, req)
     AsyncCall(env, req_wrap, args, "readlink", encoding, AfterStringPtr,
               uv_fs_readlink, *path);
-    req_wrap->SetReturnValue(args);
   } else {
     CHECK_EQ(argc, 4);
     fs_req_wrap req;
@@ -918,7 +912,6 @@ static void Rename(const FunctionCallbackInfo<Value>& args) {
   if (req_wrap != nullptr) {
     AsyncDestCall(env, req_wrap, args, "rename", *new_path, new_path.length(),
                   UTF8, AfterNoArgs, uv_fs_rename, *old_path, *new_path);
-    req_wrap->SetReturnValue(args);
   } else {
     CHECK_EQ(argc, 4);
     fs_req_wrap req;
@@ -942,7 +935,6 @@ static void FTruncate(const FunctionCallbackInfo<Value>& args) {
   if (req_wrap != nullptr) {
     AsyncCall(env, req_wrap, args, "ftruncate", UTF8, AfterNoArgs,
               uv_fs_ftruncate, fd, len);
-    req_wrap->SetReturnValue(args);
   } else {
     CHECK_EQ(argc, 4);
     fs_req_wrap req;
@@ -963,7 +955,6 @@ static void Fdatasync(const FunctionCallbackInfo<Value>& args) {
   if (req_wrap != nullptr) {
     AsyncCall(env, req_wrap, args, "fdatasync", UTF8, AfterNoArgs,
               uv_fs_fdatasync, fd);
-    req_wrap->SetReturnValue(args);
   } else {
     CHECK_EQ(argc, 3);
     fs_req_wrap req;
@@ -984,7 +975,6 @@ static void Fsync(const FunctionCallbackInfo<Value>& args) {
   if (req_wrap != nullptr) {
     AsyncCall(env, req_wrap, args, "fsync", UTF8, AfterNoArgs,
               uv_fs_fsync, fd);
-    req_wrap->SetReturnValue(args);
   } else {
     CHECK_EQ(argc, 3);
     fs_req_wrap req;
@@ -1005,7 +995,6 @@ static void Unlink(const FunctionCallbackInfo<Value>& args) {
   if (req_wrap != nullptr) {
     AsyncCall(env, req_wrap, args, "unlink", UTF8, AfterNoArgs,
               uv_fs_unlink, *path);
-    req_wrap->SetReturnValue(args);
   } else {
     CHECK_EQ(argc, 3);
     fs_req_wrap req;
@@ -1025,7 +1014,6 @@ static void RMDir(const FunctionCallbackInfo<Value>& args) {
   if (req_wrap != nullptr) {
     AsyncCall(env, req_wrap, args, "rmdir", UTF8, AfterNoArgs,
               uv_fs_rmdir, *path);
-    req_wrap->SetReturnValue(args);
   } else {
     SYNC_CALL(rmdir, *path, *path)
   }
@@ -1046,7 +1034,6 @@ static void MKDir(const FunctionCallbackInfo<Value>& args) {
   if (req_wrap != nullptr) {
     AsyncCall(env, req_wrap, args, "mkdir", UTF8, AfterNoArgs,
               uv_fs_mkdir, *path, mode);
-    req_wrap->SetReturnValue(args);
   } else {
     SYNC_CALL(mkdir, *path, *path, mode)
   }
@@ -1064,7 +1051,6 @@ static void RealPath(const FunctionCallbackInfo<Value>& args) {
   if (req_wrap != nullptr) {
     AsyncCall(env, req_wrap, args, "realpath", encoding, AfterStringPtr,
               uv_fs_realpath, *path);
-    req_wrap->SetReturnValue(args);
   } else {
     SYNC_CALL(realpath, *path, *path);
     const char* link_path = static_cast<const char*>(SYNC_REQ.ptr);
@@ -1096,7 +1082,6 @@ static void ReadDir(const FunctionCallbackInfo<Value>& args) {
   if (req_wrap != nullptr) {
     AsyncCall(env, req_wrap, args, "scandir", encoding, AfterScanDir,
               uv_fs_scandir, *path, 0 /*flags*/);
-    req_wrap->SetReturnValue(args);
   } else {
     SYNC_CALL(scandir, *path, *path, 0 /*flags*/)
 
@@ -1167,7 +1152,6 @@ static void Open(const FunctionCallbackInfo<Value>& args) {
   if (req_wrap != nullptr) {
     AsyncCall(env, req_wrap, args, "open", UTF8, AfterInteger,
               uv_fs_open, *path, flags, mode);
-    req_wrap->SetReturnValue(args);
   } else {
     SYNC_CALL(open, *path, *path, flags, mode)
     args.GetReturnValue().Set(SYNC_RESULT);
@@ -1192,7 +1176,6 @@ static void OpenFileHandle(const FunctionCallbackInfo<Value>& args) {
   if (req_wrap != nullptr) {
     AsyncCall(env, req_wrap, args, "open", UTF8, AfterOpenFileHandle,
               uv_fs_open, *path, flags, mode);
-    req_wrap->SetReturnValue(args);
   } else {
     SYNC_CALL(open, *path, *path, flags, mode)
     HandleScope scope(env->isolate());
@@ -1217,7 +1200,6 @@ static void CopyFile(const FunctionCallbackInfo<Value>& args) {
   if (req_wrap != nullptr) {
     AsyncCall(env, req_wrap, args, "copyfile", UTF8, AfterNoArgs,
               uv_fs_copyfile, *src, *dest, flags);
-    req_wrap->SetReturnValue(args);
   } else {
     SYNC_DEST_CALL(copyfile, *src, *dest, *src, *dest, flags)
   }
@@ -1260,7 +1242,7 @@ static void WriteBuffer(const FunctionCallbackInfo<Value>& args) {
   if (req_wrap != nullptr) {
     AsyncCall(env, req_wrap, args, "write", UTF8, AfterInteger,
               uv_fs_write, fd, &uvbuf, 1, pos);
-    return req_wrap->SetReturnValue(args);
+    return;
   }
 
   SYNC_CALL(write, nullptr, fd, &uvbuf, 1, pos)
@@ -1297,7 +1279,7 @@ static void WriteBuffers(const FunctionCallbackInfo<Value>& args) {
   if (req_wrap != nullptr) {
     AsyncCall(env, req_wrap, args, "write", UTF8, AfterInteger,
               uv_fs_write, fd, *iovs, iovs.length(), pos);
-    return req_wrap->SetReturnValue(args);
+    return;
   }
 
   SYNC_CALL(write, nullptr, fd, *iovs, iovs.length(), pos)
@@ -1365,7 +1347,6 @@ static void WriteString(const FunctionCallbackInfo<Value>& args) {
   if (req_wrap != nullptr) {
     AsyncCall(env, req_wrap, args, "write", UTF8, AfterInteger,
               uv_fs_write, fd, &uvbuf, 1, pos);
-    req_wrap->SetReturnValue(args);
   } else {
     SYNC_CALL(write, nullptr, fd, &uvbuf, 1, pos)
     return args.GetReturnValue().Set(SYNC_RESULT);
@@ -1420,7 +1401,6 @@ static void Read(const FunctionCallbackInfo<Value>& args) {
   if (req_wrap != nullptr) {
     AsyncCall(env, req_wrap, args, "read", UTF8, AfterInteger,
               uv_fs_read, fd, &uvbuf, 1, pos);
-    req_wrap->SetReturnValue(args);
   } else {
     SYNC_CALL(read, 0, fd, &uvbuf, 1, pos)
     args.GetReturnValue().Set(SYNC_RESULT);
@@ -1446,7 +1426,6 @@ static void Chmod(const FunctionCallbackInfo<Value>& args) {
   if (req_wrap != nullptr) {
     AsyncCall(env, req_wrap, args, "chmod", UTF8, AfterNoArgs,
               uv_fs_chmod, *path, mode);
-    req_wrap->SetReturnValue(args);
   } else {
     SYNC_CALL(chmod, *path, *path, mode);
   }
@@ -1469,7 +1448,6 @@ static void FChmod(const FunctionCallbackInfo<Value>& args) {
   if (req_wrap != nullptr) {
     AsyncCall(env, req_wrap, args, "fchmod", UTF8, AfterNoArgs,
               uv_fs_fchmod, fd, mode);
-    req_wrap->SetReturnValue(args);
   } else {
     SYNC_CALL(fchmod, 0, fd, mode);
   }
@@ -1497,7 +1475,6 @@ static void Chown(const FunctionCallbackInfo<Value>& args) {
   if (req_wrap != nullptr) {
     AsyncCall(env, req_wrap, args, "chown", UTF8, AfterNoArgs,
               uv_fs_chown, *path, uid, gid);
-    req_wrap->SetReturnValue(args);
   } else {
     SYNC_CALL(chown, *path, *path, uid, gid);
   }
@@ -1522,7 +1499,6 @@ static void FChown(const FunctionCallbackInfo<Value>& args) {
   if (req_wrap != nullptr) {
     AsyncCall(env, req_wrap, args, "fchown", UTF8, AfterNoArgs,
               uv_fs_fchown, fd, uid, gid);
-    req_wrap->SetReturnValue(args);
   } else {
     SYNC_CALL(fchown, 0, fd, uid, gid);
   }
@@ -1546,7 +1522,6 @@ static void UTimes(const FunctionCallbackInfo<Value>& args) {
   if (req_wrap != nullptr) {
     AsyncCall(env, req_wrap, args, "utime", UTF8, AfterNoArgs,
               uv_fs_utime, *path, atime, mtime);
-    req_wrap->SetReturnValue(args);
   } else {
     SYNC_CALL(utime, *path, *path, atime, mtime);
   }
@@ -1567,7 +1542,6 @@ static void FUTimes(const FunctionCallbackInfo<Value>& args) {
   if (req_wrap != nullptr) {
     AsyncCall(env, req_wrap, args, "futime", UTF8, AfterNoArgs,
               uv_fs_futime, fd, atime, mtime);
-    req_wrap->SetReturnValue(args);
   } else {
     SYNC_CALL(futime, 0, fd, atime, mtime);
   }
@@ -1587,7 +1561,6 @@ static void Mkdtemp(const FunctionCallbackInfo<Value>& args) {
   if (req_wrap != nullptr) {
     AsyncCall(env, req_wrap, args, "mkdtemp", encoding, AfterStringPath,
               uv_fs_mkdtemp, *tmpl);
-    req_wrap->SetReturnValue(args);
   } else {
     SYNC_CALL(mkdtemp, *tmpl, *tmpl);
     const char* path = static_cast<const char*>(SYNC_REQ.path);