fs: do not swallow exceptions thrown by completion callbacks

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();