fs: guard against undefined behavior

Calling close on a file description which is currently in use is
undefined behavior due to implementation details in libuv. Add
a guard against this when using FileHandle.

PR-URL: #34746
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: James M Snell <[email protected]>

Author: ronag

doc/api/fs.md

@@ -4528,7 +4528,8 @@ added: v10.0.0
   file descriptor is closed, or will be rejected if an error occurs while
   closing.
 
-Closes the file descriptor.
+Closes the file handle. Will wait for any pending operation on the handle
+to complete before completing.
 
 ```js
 const fsPromises = require('fs').promises;

lib/internal/fs/promises.js

@@ -14,6 +14,8 @@ const {
   MathMin,
   NumberIsSafeInteger,
   Symbol,
+  Error,
+  Promise,
 } = primordials;
 
 const {
@@ -64,6 +66,11 @@ const { promisify } = require('internal/util');
 
 const kHandle = Symbol('kHandle');
 const kFd = Symbol('kFd');
+const kRefs = Symbol('kRefs');
+const kClosePromise = Symbol('kClosePromise');
+const kCloseResolve = Symbol('kCloseResolve');
+const kCloseReject = Symbol('kCloseReject');
+
 const { kUsePromises } = binding;
 const {
   JSTransferable, kDeserialize, kTransfer, kTransferList
@@ -76,6 +83,9 @@ class FileHandle extends JSTransferable {
     super();
     this[kHandle] = filehandle;
     this[kFd] = filehandle ? filehandle.fd : -1;
+
+    this[kRefs] = 1;
+    this[kClosePromise] = null;
   }
 
   getAsyncId() {
@@ -87,70 +97,101 @@ class FileHandle extends JSTransferable {
   }
 
   appendFile(data, options) {
-    return writeFile(this, data, options);
+    return fsCall(writeFile, this, data, options);
   }
 
   chmod(mode) {
-    return fchmod(this, mode);
+    return fsCall(fchmod, this, mode);
   }
 
   chown(uid, gid) {
-    return fchown(this, uid, gid);
+    return fsCall(fchown, this, uid, gid);
   }
 
   datasync() {
-    return fdatasync(this);
+    return fsCall(fdatasync, this);
   }
 
   sync() {
-    return fsync(this);
+    return fsCall(fsync, this);
   }
 
   read(buffer, offset, length, position) {
-    return read(this, buffer, offset, length, position);
+    return fsCall(read, this, buffer, offset, length, position);
   }
 
   readv(buffers, position) {
-    return readv(this, buffers, position);
+    return fsCall(readv, this, buffers, position);
   }
 
   readFile(options) {
-    return readFile(this, options);
+    return fsCall(readFile, this, options);
   }
 
   stat(options) {
-    return fstat(this, options);
+    return fsCall(fstat, this, options);
   }
 
   truncate(len = 0) {
-    return ftruncate(this, len);
+    return fsCall(ftruncate, this, len);
   }
 
   utimes(atime, mtime) {
-    return futimes(this, atime, mtime);
+    return fsCall(futimes, this, atime, mtime);
   }
 
   write(buffer, offset, length, position) {
-    return write(this, buffer, offset, length, position);
+    return fsCall(write, this, buffer, offset, length, position);
   }
 
   writev(buffers, position) {
-    return writev(this, buffers, position);
+    return fsCall(writev, this, buffers, position);
   }
 
   writeFile(data, options) {
-    return writeFile(this, data, options);
+    return fsCall(writeFile, this, data, options);
   }
 
   close = () => {
-    this[kFd] = -1;
-    return this[kHandle].close();
+    if (this[kFd] === -1) {
+      return Promise.resolve();
+    }
+
+    if (this[kClosePromise]) {
+      return this[kClosePromise];
+    }
+
+    this[kRefs]--;
+    if (this[kRefs] === 0) {
+      this[kFd] = -1;
+      this[kClosePromise] = this[kHandle].close().finally(() => {
+        this[kClosePromise] = undefined;
+      });
+    } else {
+      this[kClosePromise] = new Promise((resolve, reject) => {
+        this[kCloseResolve] = resolve;
+        this[kCloseReject] = reject;
+      }).finally(() => {
+        this[kClosePromise] = undefined;
+        this[kCloseReject] = undefined;
+        this[kCloseResolve] = undefined;
+      });
+    }
+
+    return this[kClosePromise];
   }
 
   [kTransfer]() {
+    if (this[kClosePromise] || this[kRefs] > 1) {
+      const DOMException = internalBinding('messaging').DOMException;
+      throw new DOMException('Cannot transfer FileHandle while in use',
+                             'DataCloneError');
+    }
+
     const handle = this[kHandle];
     this[kFd] = -1;
     this[kHandle] = null;
+    this[kRefs] = 0;
 
     return {
       data: { handle },
@@ -168,9 +209,31 @@ class FileHandle extends JSTransferable {
   }
 }
 
-function validateFileHandle(handle) {
-  if (!(handle instanceof FileHandle))
+async function fsCall(fn, handle, ...args) {
+  if (handle[kRefs] === undefined) {
     throw new ERR_INVALID_ARG_TYPE('filehandle', 'FileHandle', handle);
+  }
+
+  if (handle.fd === -1) {
+    // eslint-disable-next-line no-restricted-syntax
+    const err = new Error('file closed');
+    err.code = 'EBADF';
+    err.syscall = fn.name;
+    throw err;
+  }
+
+  try {
+    handle[kRefs]++;
+    return await fn(handle, ...args);
+  } finally {
+    handle[kRefs]--;
+    if (handle[kRefs] === 0) {
+      handle[kFd] = -1;
+      handle[kHandle]
+        .close()
+        .then(handle[kCloseResolve], handle[kCloseReject]);
+    }
+  }
 }
 
 async function writeFileHandle(filehandle, data) {
@@ -249,7 +312,6 @@ async function open(path, flags, mode) {
 }
 
 async function read(handle, buffer, offset, length, position) {
-  validateFileHandle(handle);
   validateBuffer(buffer);
 
   if (offset == null) {
@@ -280,7 +342,6 @@ async function read(handle, buffer, offset, length, position) {
 }
 
 async function readv(handle, buffers, position) {
-  validateFileHandle(handle);
   validateBufferArray(buffers);
 
   if (typeof position !== 'number')
@@ -292,8 +353,6 @@ async function readv(handle, buffers, position) {
 }
 
 async function write(handle, buffer, offset, length, position) {
-  validateFileHandle(handle);
-
   if (buffer.length === 0)
     return { bytesWritten: 0, buffer };
 
@@ -321,7 +380,6 @@ async function write(handle, buffer, offset, length, position) {
 }
 
 async function writev(handle, buffers, position) {
-  validateFileHandle(handle);
   validateBufferArray(buffers);
 
   if (typeof position !== 'number')
@@ -346,7 +404,6 @@ async function truncate(path, len = 0) {
 }
 
 async function ftruncate(handle, len = 0) {
-  validateFileHandle(handle);
   validateInteger(len, 'len');
   len = MathMax(0, len);
   return binding.ftruncate(handle.fd, len, kUsePromises);
@@ -364,12 +421,10 @@ async function rmdir(path, options) {
 }
 
 async function fdatasync(handle) {
-  validateFileHandle(handle);
   return binding.fdatasync(handle.fd, kUsePromises);
 }
 
 async function fsync(handle) {
-  validateFileHandle(handle);
   return binding.fsync(handle.fd, kUsePromises);
 }
 
@@ -420,7 +475,6 @@ async function symlink(target, path, type_) {
 }
 
 async function fstat(handle, options = { bigint: false }) {
-  validateFileHandle(handle);
   const result = await binding.fstat(handle.fd, options.bigint, kUsePromises);
   return getStatsFromBinding(result);
 }
@@ -453,7 +507,6 @@ async function unlink(path) {
 }
 
 async function fchmod(handle, mode) {
-  validateFileHandle(handle);
   mode = parseFileMode(mode, 'mode');
   return binding.fchmod(handle.fd, mode, kUsePromises);
 }
@@ -481,7 +534,6 @@ async function lchown(path, uid, gid) {
 }
 
 async function fchown(handle, uid, gid) {
-  validateFileHandle(handle);
   validateUint32(uid, 'uid');
   validateUint32(gid, 'gid');
   return binding.fchown(handle.fd, uid, gid, kUsePromises);
@@ -504,7 +556,6 @@ async function utimes(path, atime, mtime) {
 }
 
 async function futimes(handle, atime, mtime) {
-  validateFileHandle(handle);
   atime = toUnixTimestamp(atime, 'atime');
   mtime = toUnixTimestamp(mtime, 'mtime');
   return binding.futimes(handle.fd, atime, mtime, kUsePromises);

test/parallel/test-worker-message-port-transfer-filehandle.js

@@ -69,3 +69,36 @@ const { once } = require('events');
 
   port1.postMessage('second message');
 })().then(common.mustCall());
+
+(async function() {
+  // Check that a FileHandle with a read in progress cannot be transferred.
+  const fh = await fs.open(__filename);
+
+  const { port1 } = new MessageChannel();
+
+  const readPromise = fh.readFile();
+  assert.throws(() => {
+    port1.postMessage(fh, [fh]);
+  }, {
+    message: 'Cannot transfer FileHandle while in use',
+    name: 'DataCloneError'
+  });
+
+  assert.deepStrictEqual(await readPromise, await fs.readFile(__filename));
+})().then(common.mustCall());
+
+(async function() {
+  // Check that filehandles with a close in progress cannot be transferred.
+  const fh = await fs.open(__filename);
+
+  const { port1 } = new MessageChannel();
+
+  const closePromise = fh.close();
+  assert.throws(() => {
+    port1.postMessage(fh, [fh]);
+  }, {
+    message: 'Cannot transfer FileHandle while in use',
+    name: 'DataCloneError'
+  });
+  await closePromise;
+})().then(common.mustCall());