fs: do not swallow exceptions thrown by completion callbacks

Since 93644d5 the TryCatch guarding StringBytes::Encode in the
completion callbacks of mkdtemp, realpath.native, readlink, recursive
mkdir, recursive readdir and dir.read is still active when the JS
callback runs. Exceptions thrown by the callback, or by the nextTick
queue drained after it, are caught by it and never reported.

Leave the TryCatch before calling into JS.

Fixes: #65667
Refs: #57706
Signed-off-by: marcopiraccini <marco.piraccini@gmail.com>

Author: marcopiraccini

src/node_dir.cc

@@ -248,22 +248,14 @@ static void AfterDirRead(uv_fs_t* req) {
 
   uv_dir_t* dir = static_cast<uv_dir_t*>(req->ptr);
 
-  TryCatch try_catch(isolate);
-  Local<Array> js_array;
-  if (!DirentListToArray(env,
-                         dir->dirents,
-                         static_cast<int>(req->result),
-                         req_wrap->encoding())
-           .ToLocal(&js_array)) {
+  ResolveOrReject(req_wrap.get(), [&]() {
+    MaybeLocal<Array> js_array = DirentListToArray(
+        env, dir->dirents, static_cast<int>(req->result), req_wrap->encoding());
     // Clear libuv resources *before* delivering results to JS land because
-    // that can schedule another operation on the same uv_dir_t. Ditto below.
+    // that can schedule another operation on the same uv_dir_t.
     after.Clear();
-    CHECK(try_catch.CanContinue());
-    return req_wrap->Reject(try_catch.Exception());
-  }
-
-  after.Clear();
-  req_wrap->Resolve(js_array);
+    return js_array;
+  });
 }
 
 

src/node_file-inl.h

@@ -413,6 +413,29 @@ int SyncCallAndThrowOnError(Environment* env,
   return SyncCallAndThrowIf(is_uv_error, env, req_wrap, fn, args...);
 }
 
+// Delivers the value produced by `produce`, or the exception it threw, to
+// `req_wrap`. The TryCatch is left before calling into JS: exceptions thrown
+// by the callback, or by the tick queue drained afterwards, must not be caught
+// by it.
+template <typename Fn>
+void ResolveOrReject(FSReqBase* req_wrap, Fn&& produce) {
+  v8::Isolate* isolate = req_wrap->env()->isolate();
+  v8::Local<v8::Value> value;
+  v8::Local<v8::Value> error;
+  {
+    v8::TryCatch try_catch(isolate);
+    if (!produce().ToLocal(&value)) {
+      CHECK(try_catch.CanContinue());
+      error = try_catch.Exception();
+    }
+  }
+  if (error.IsEmpty()) {
+    req_wrap->Resolve(value);
+  } else {
+    req_wrap->Reject(error);
+  }
+}
+
 }  // namespace fs
 }  // namespace node
 

src/node_file.cc

@@ -900,16 +900,10 @@ void AfterMkdirp(uv_fs_t* req) {
     std::string first_path(req_wrap->continuation_data()->first_path());
     if (first_path.empty())
       return req_wrap->Resolve(Undefined(req_wrap->env()->isolate()));
-    Local<Value> path;
-    TryCatch try_catch(req_wrap->env()->isolate());
-    if (!StringBytes::Encode(req_wrap->env()->isolate(),
-                             first_path.c_str(),
-                             req_wrap->encoding())
-             .ToLocal(&path)) {
-      CHECK(try_catch.CanContinue());
-      return req_wrap->Reject(try_catch.Exception());
-    }
-    return req_wrap->Resolve(path);
+    ResolveOrReject(req_wrap, [&]() {
+      return StringBytes::Encode(
+          req_wrap->env()->isolate(), first_path.c_str(), req_wrap->encoding());
+    });
   }
 }
 
@@ -918,19 +912,11 @@ void AfterStringPath(uv_fs_t* req) {
   FSReqAfterScope after(req_wrap, req);
   FS_ASYNC_TRACE_END1(
       req->fs_type, req_wrap, "result", static_cast<int>(req->result))
-  MaybeLocal<Value> link;
-
   if (after.Proceed()) {
-    TryCatch try_catch(req_wrap->env()->isolate());
-    link = StringBytes::Encode(
-        req_wrap->env()->isolate(), req->path, req_wrap->encoding());
-    if (link.IsEmpty()) {
-      CHECK(try_catch.CanContinue());
-      req_wrap->Reject(try_catch.Exception());
-    } else {
-      Local<Value> val;
-      if (link.ToLocal(&val)) req_wrap->Resolve(val);
-    }
+    ResolveOrReject(req_wrap, [&]() {
+      return StringBytes::Encode(
+          req_wrap->env()->isolate(), req->path, req_wrap->encoding());
+    });
   }
 }
 
@@ -939,20 +925,12 @@ void AfterStringPtr(uv_fs_t* req) {
   FSReqAfterScope after(req_wrap, req);
   FS_ASYNC_TRACE_END1(
       req->fs_type, req_wrap, "result", static_cast<int>(req->result))
-  MaybeLocal<Value> link;
-
   if (after.Proceed()) {
-    TryCatch try_catch(req_wrap->env()->isolate());
-    link = StringBytes::Encode(req_wrap->env()->isolate(),
-                               static_cast<const char*>(req->ptr),
-                               req_wrap->encoding());
-    if (link.IsEmpty()) {
-      CHECK(try_catch.CanContinue());
-      req_wrap->Reject(try_catch.Exception());
-    } else {
-      Local<Value> val;
-      if (link.ToLocal(&val)) req_wrap->Resolve(val);
-    }
+    ResolveOrReject(req_wrap, [&]() {
+      return StringBytes::Encode(req_wrap->env()->isolate(),
+                                 static_cast<const char*>(req->ptr),
+                                 req_wrap->encoding());
+    });
   }
 }
 
@@ -2700,14 +2678,9 @@ class ReadDirRecursiveRequest {
                                           walk_.error_path().c_str()));
     }
 
-    Local<Value> value;
-    TryCatch try_catch(isolate);
-    if (!MarshalRecursiveReadDir(isolate, walk_, encoding_, with_types_)
-             .ToLocal(&value)) {
-      CHECK(try_catch.CanContinue());
-      return req_wrap->Reject(try_catch.Exception());
-    }
-    req_wrap->Resolve(value);
+    ResolveOrReject(req_wrap.get(), [&]() {
+      return MarshalRecursiveReadDir(isolate, walk_, encoding_, with_types_);
+    });
   }
 
  private:

test/parallel/test-fs-callback-throw.js

@@ -0,0 +1,62 @@
+'use strict';
+// Refs: https://github.com/nodejs/node/issues/65667
+// Exceptions thrown after an fs operation completes must reach
+// 'uncaughtException' instead of being swallowed.
+const common = require('../common');
+const tmpdir = require('../common/tmpdir');
+const assert = require('assert');
+const fs = require('fs');
+const path = require('path');
+
+tmpdir.refresh();
+const dir = tmpdir.path;
+const file = path.join(dir, 'file');
+const link = path.join(dir, 'link');
+fs.writeFileSync(file, '');
+
+// The callback throws.
+const callbackCases = {
+  'mkdtemp': (cb) => fs.mkdtemp(path.join(dir, 'x-'), cb),
+  'realpath.native': (cb) => fs.realpath.native(dir, cb),
+  'mkdir recursive': (cb) => fs.mkdir(path.join(dir, 'a', 'b'), { recursive: true }, cb),
+  'readdir recursive': (cb) => fs.readdir(dir, { recursive: true }, cb),
+};
+if (common.canCreateSymLink()) {
+  fs.symlinkSync(file, link);
+  callbackCases.readlink = (cb) => fs.readlink(link, cb);
+}
+
+// A nextTick callback scheduled after the promise settles throws.
+const promiseCases = {
+  'promises.mkdtemp': () => fs.promises.mkdtemp(path.join(dir, 'p-')),
+  'dir.read': async () => {
+    const d = await fs.promises.opendir(dir);
+    await d.read();
+    process.nextTick(() => d.closeSync());
+  },
+};
+
+const cases = [
+  ...Object.entries(callbackCases).map(([name, run]) => [name, () => {
+    run(common.mustSucceed(() => { throw new Error(name); }));
+  }]),
+  ...Object.entries(promiseCases).map(([name, run]) => [name, async () => {
+    await run();
+    process.nextTick(() => { throw new Error(name); });
+  }]),
+];
+
+let current;
+process.on('uncaughtException', common.mustCall((err) => {
+  assert.strictEqual(err.message, current);
+  next();
+}, cases.length));
+
+function next() {
+  const entry = cases.shift();
+  if (entry === undefined) return;
+  current = entry[0];
+  entry[1]();
+}
+
+next();