fs: do not throw exception after creating FSReqCallback

Once an `FSReqCallback` instance is created, it is a GC root until
the underlying fs operation has completed, meaning that it cannot
be garbage collected.

This is a problem when the underlying operation never starts
because an exception is thrown before that happens, for example
as part of parameter validation.

Instead, move all potentially throwing code before the `FSReqCallback`
creation.

PR-URL: #35487
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

Author: addaleax

lib/fs.js

@@ -28,6 +28,10 @@
 // See https://github.com/libuv/libuv/pull/1501.
 const kIoMaxLength = 2 ** 31 - 1;
 
+// When using FSReqCallback, make sure to create the object only *after* all
+// parameter validation has happened, so that the objects are not kept in memory
+// in case they are created but never used due to an exception.
+
 const {
   Map,
   MathMax,
@@ -198,8 +202,10 @@ function access(path, mode, callback) {
 
   path = getValidatedPath(path);
   mode = getValidMode(mode, 'access');
+  callback = makeCallback(callback);
+
   const req = new FSReqCallback();
-  req.oncomplete = makeCallback(callback);
+  req.oncomplete = callback;
   binding.access(pathModule.toNamespacedPath(path), mode, req);
 }
 
@@ -307,19 +313,19 @@ function readFile(path, options, callback) {
   const context = new ReadFileContext(callback, options.encoding);
   context.isUserFd = isFd(path); // File descriptor ownership
 
-  const req = new FSReqCallback();
-  req.context = context;
-  req.oncomplete = readFileAfterOpen;
-
   if (context.isUserFd) {
-    process.nextTick(function tick() {
-      req.oncomplete(null, path);
-    });
+    process.nextTick(function tick(context) {
+      readFileAfterOpen.call({ context }, null, path);
+    }, context);
     return;
   }
 
-  path = getValidatedPath(path);
   const flagsNumber = stringToFlags(options.flags);
+  path = getValidatedPath(path);
+
+  const req = new FSReqCallback();
+  req.context = context;
+  req.oncomplete = readFileAfterOpen;
   binding.open(pathModule.toNamespacedPath(path),
                flagsNumber,
                0o666,
@@ -416,8 +422,10 @@ function readFileSync(path, options) {
 
 function close(fd, callback) {
   validateInt32(fd, 'fd', 0);
+  callback = makeCallback(callback);
+
   const req = new FSReqCallback();
-  req.oncomplete = makeCallback(callback);
+  req.oncomplete = callback;
   binding.close(fd, req);
 }
 
@@ -590,12 +598,11 @@ function readv(fd, buffers, position, callback) {
 
   validateInt32(fd, 'fd', /* min */ 0);
   validateBufferArray(buffers);
+  callback = maybeCallback(callback || position);
 
   const req = new FSReqCallback();
   req.oncomplete = wrapper;
 
-  callback = maybeCallback(callback || position);
-
   if (typeof position !== 'number')
     position = null;
 
@@ -712,12 +719,11 @@ function writev(fd, buffers, position, callback) {
 
   validateInt32(fd, 'fd', 0);
   validateBufferArray(buffers);
+  callback = maybeCallback(callback || position);
 
   const req = new FSReqCallback();
   req.oncomplete = wrapper;
 
-  callback = maybeCallback(callback || position);
-
   if (typeof position !== 'number')
     position = null;
 
@@ -819,8 +825,10 @@ function ftruncate(fd, len = 0, callback) {
   validateInt32(fd, 'fd', 0);
   validateInteger(len, 'len');
   len = MathMax(0, len);
+  callback = makeCallback(callback);
+
   const req = new FSReqCallback();
-  req.oncomplete = makeCallback(callback);
+  req.oncomplete = callback;
   binding.ftruncate(fd, len, req);
 }
 
@@ -989,8 +997,10 @@ function fstat(fd, options = { bigint: false }, callback) {
     options = {};
   }
   validateInt32(fd, 'fd', 0);
+  callback = makeStatsCallback(callback);
+
   const req = new FSReqCallback(options.bigint);
-  req.oncomplete = makeStatsCallback(callback);
+  req.oncomplete = callback;
   binding.fstat(fd, options.bigint, req);
 }
 
@@ -1001,6 +1011,7 @@ function lstat(path, options = { bigint: false }, callback) {
   }
   callback = makeStatsCallback(callback);
   path = getValidatedPath(path);
+
   const req = new FSReqCallback(options.bigint);
   req.oncomplete = callback;
   binding.lstat(pathModule.toNamespacedPath(path), options.bigint, req);
@@ -1013,6 +1024,7 @@ function stat(path, options = { bigint: false }, callback) {
   }
   callback = makeStatsCallback(callback);
   path = getValidatedPath(path);
+
   const req = new FSReqCallback(options.bigint);
   req.oncomplete = callback;
   binding.stat(pathModule.toNamespacedPath(path), options.bigint, req);
@@ -1070,9 +1082,6 @@ function symlink(target, path, type_, callback_) {
   target = getValidatedPath(target, 'target');
   path = getValidatedPath(path);
 
-  const req = new FSReqCallback();
-  req.oncomplete = callback;
-
   if (isWindows && type === null) {
     let absoluteTarget;
     try {
@@ -1087,18 +1096,25 @@ function symlink(target, path, type_, callback_) {
       stat(absoluteTarget, (err, stat) => {
         const resolvedType = !err && stat.isDirectory() ? 'dir' : 'file';
         const resolvedFlags = stringToSymlinkType(resolvedType);
-        binding.symlink(preprocessSymlinkDestination(target,
-                                                     resolvedType,
-                                                     path),
+        const destination = preprocessSymlinkDestination(target,
+                                                         resolvedType,
+                                                         path);
+
+        const req = new FSReqCallback();
+        req.oncomplete = callback;
+        binding.symlink(destination,
                         pathModule.toNamespacedPath(path), resolvedFlags, req);
       });
       return;
     }
   }
 
+  const destination = preprocessSymlinkDestination(target, type, path);
+
   const flags = stringToSymlinkType(type);
-  binding.symlink(preprocessSymlinkDestination(target, type, path),
-                  pathModule.toNamespacedPath(path), flags, req);
+  const req = new FSReqCallback();
+  req.oncomplete = callback;
+  binding.symlink(destination, pathModule.toNamespacedPath(path), flags, req);
 }
 
 function symlinkSync(target, path, type) {
@@ -1255,9 +1271,10 @@ function fchown(fd, uid, gid, callback) {
   validateInt32(fd, 'fd', 0);
   validateInteger(uid, 'uid', -1, kMaxUserId);
   validateInteger(gid, 'gid', -1, kMaxUserId);
+  callback = makeCallback(callback);
 
   const req = new FSReqCallback();
-  req.oncomplete = makeCallback(callback);
+  req.oncomplete = callback;
   binding.fchown(fd, uid, gid, req);
 }
 
@@ -1316,8 +1333,10 @@ function futimes(fd, atime, mtime, callback) {
   validateInt32(fd, 'fd', 0);
   atime = toUnixTimestamp(atime, 'atime');
   mtime = toUnixTimestamp(mtime, 'mtime');
+  callback = makeCallback(callback);
+
   const req = new FSReqCallback();
-  req.oncomplete = makeCallback(callback);
+  req.oncomplete = callback;
   binding.futimes(fd, atime, mtime, req);
 }
 
@@ -1920,8 +1939,10 @@ function copyFile(src, dest, mode, callback) {
   src = pathModule._makeLong(src);
   dest = pathModule._makeLong(dest);
   mode = getValidMode(mode, 'copyFile');
+  callback = makeCallback(callback);
+
   const req = new FSReqCallback();
-  req.oncomplete = makeCallback(callback);
+  req.oncomplete = callback;
   binding.copyFile(src, dest, mode, req);
 }
 

lib/internal/fs/read_file_context.js

@@ -100,18 +100,18 @@ class ReadFileContext {
   }
 
   close(err) {
+    if (this.isUserFd) {
+      process.nextTick(function tick(context) {
+        readFileAfterClose.call({ context }, null);
+      }, this);
+      return;
+    }
+
     const req = new FSReqCallback();
     req.oncomplete = readFileAfterClose;
     req.context = this;
     this.err = err;
 
-    if (this.isUserFd) {
-      process.nextTick(function tick() {
-        req.oncomplete(null);
-      });
-      return;
-    }
-
     close(this.fd, req);
   }
 }