fs: forbid concurrent operations on Dir handle

libuv does not expect concurrent operations on `uv_dir_t` instances,
and will gladly create memory leaks, corrupt data, or crash the
process.

This patch forbids that, and:

- Makes sure that concurrent async operations are run sequentially
- Throws an exception if sync operations are attempted during an
  async operation

The assumption here is that a thrown exception is preferable to
a potential hard crash.

This fully fixes flakiness from `parallel/test-fs-opendir` when
run under ASAN.

PR-URL: #33274
Reviewed-By: Colin Ihrig <[email protected]>

doc/api/errors.md

@@ -855,6 +855,15 @@ An unknown Diffie-Hellman group name was given. See
 
 The [`fs.Dir`][] was previously closed.
 
+<a id="ERR_DIR_CONCURRENT_OPERATION"></a>
+### `ERR_DIR_CONCURRENT_OPERATION`
+<!-- YAML
+added: REPLACEME
+-->
+
+A synchronous read or close call was attempted on an [`fs.Dir`][] which has
+ongoing asynchronous operations.
+
 <a id="ERR_DNS_SET_SERVERS_FAILED"></a>
 ### `ERR_DNS_SET_SERVERS_FAILED`
 

lib/internal/errors.js

@@ -788,6 +788,9 @@ E('ERR_CRYPTO_SIGN_KEY_REQUIRED', 'No key provided to sign', Error);
 E('ERR_CRYPTO_TIMING_SAFE_EQUAL_LENGTH',
   'Input buffers must have the same byte length', RangeError);
 E('ERR_DIR_CLOSED', 'Directory handle was closed', Error);
+E('ERR_DIR_CONCURRENT_OPERATION',
+  'Cannot do synchronous work on directory handle with concurrent ' +
+  'asynchronous operations', Error);
 E('ERR_DNS_SET_SERVERS_FAILED', 'c-ares failed to set servers: "%s" [%s]',
   Error);
 E('ERR_DOMAIN_CALLBACK_NOT_AVAILABLE',

lib/internal/fs/dir.js

@@ -12,6 +12,7 @@ const dirBinding = internalBinding('fs_dir');
 const {
   codes: {
     ERR_DIR_CLOSED,
+    ERR_DIR_CONCURRENT_OPERATION,
     ERR_INVALID_CALLBACK,
     ERR_MISSING_ARGS
   }
@@ -37,6 +38,7 @@ const kDirOptions = Symbol('kDirOptions');
 const kDirReadImpl = Symbol('kDirReadImpl');
 const kDirReadPromisified = Symbol('kDirReadPromisified');
 const kDirClosePromisified = Symbol('kDirClosePromisified');
+const kDirOperationQueue = Symbol('kDirOperationQueue');
 
 class Dir {
   constructor(handle, path, options) {
@@ -46,6 +48,10 @@ class Dir {
     this[kDirPath] = path;
     this[kDirClosed] = false;
 
+    // Either `null` or an Array of pending operations (= functions to be called
+    // once the current operation is done).
+    this[kDirOperationQueue] = null;
+
     this[kDirOptions] = {
       bufferSize: 32,
       ...getOptions(options, {
@@ -79,6 +85,13 @@ class Dir {
       throw new ERR_INVALID_CALLBACK(callback);
     }
 
+    if (this[kDirOperationQueue] !== null) {
+      this[kDirOperationQueue].push(() => {
+        this[kDirReadImpl](maybeSync, callback);
+      });
+      return;
+    }
+
     if (this[kDirBufferedEntries].length > 0) {
       const [ name, type ] = this[kDirBufferedEntries].splice(0, 2);
       if (maybeSync)
@@ -90,6 +103,12 @@ class Dir {
 
     const req = new FSReqCallback();
     req.oncomplete = (err, result) => {
+      process.nextTick(() => {
+        const queue = this[kDirOperationQueue];
+        this[kDirOperationQueue] = null;
+        for (const op of queue) op();
+      });
+
       if (err || result === null) {
         return callback(err, result);
       }
@@ -98,6 +117,7 @@ class Dir {
       getDirent(this[kDirPath], result[0], result[1], callback);
     };
 
+    this[kDirOperationQueue] = [];
     this[kDirHandle].read(
       this[kDirOptions].encoding,
       this[kDirOptions].bufferSize,
@@ -110,6 +130,10 @@ class Dir {
       throw new ERR_DIR_CLOSED();
     }
 
+    if (this[kDirOperationQueue] !== null) {
+      throw new ERR_DIR_CONCURRENT_OPERATION();
+    }
+
     if (this[kDirBufferedEntries].length > 0) {
       const [ name, type ] = this[kDirBufferedEntries].splice(0, 2);
       return getDirent(this[kDirPath], name, type);
@@ -143,6 +167,13 @@ class Dir {
       throw new ERR_INVALID_CALLBACK(callback);
     }
 
+    if (this[kDirOperationQueue] !== null) {
+      this[kDirOperationQueue].push(() => {
+        this.close(callback);
+      });
+      return;
+    }
+
     this[kDirClosed] = true;
     const req = new FSReqCallback();
     req.oncomplete = callback;
@@ -154,6 +185,10 @@ class Dir {
       throw new ERR_DIR_CLOSED();
     }
 
+    if (this[kDirOperationQueue] !== null) {
+      throw new ERR_DIR_CONCURRENT_OPERATION();
+    }
+
     this[kDirClosed] = true;
     const ctx = { path: this[kDirPath] };
     const result = this[kDirHandle].close(undefined, ctx);

test/parallel/test-fs-opendir.js

@@ -33,6 +33,10 @@ const dirclosedError = {
   code: 'ERR_DIR_CLOSED'
 };
 
+const dirconcurrentError = {
+  code: 'ERR_DIR_CONCURRENT_OPERATION'
+};
+
 const invalidCallbackObj = {
   code: 'ERR_INVALID_CALLBACK',
   name: 'TypeError'
@@ -230,3 +234,39 @@ async function doAsyncIterDirClosedTest() {
   assert.throws(() => dir.close(), dirclosedError);
 }
 doAsyncIterDirClosedTest().then(common.mustCall());
+
+// Check that readSync() and closeSync() during read() throw exceptions
+async function doConcurrentAsyncAndSyncOps() {
+  const dir = await fs.promises.opendir(testDir);
+  const promise = dir.read();
+
+  assert.throws(() => dir.closeSync(), dirconcurrentError);
+  assert.throws(() => dir.readSync(), dirconcurrentError);
+
+  await promise;
+  dir.closeSync();
+}
+doConcurrentAsyncAndSyncOps().then(common.mustCall());
+
+// Check that concurrent read() operations don't do weird things.
+async function doConcurrentAsyncOps() {
+  const dir = await fs.promises.opendir(testDir);
+  const promise1 = dir.read();
+  const promise2 = dir.read();
+
+  assertDirent(await promise1);
+  assertDirent(await promise2);
+  dir.closeSync();
+}
+doConcurrentAsyncOps().then(common.mustCall());
+
+// Check that concurrent read() + close() operations don't do weird things.
+async function doConcurrentAsyncMixedOps() {
+  const dir = await fs.promises.opendir(testDir);
+  const promise1 = dir.read();
+  const promise2 = dir.close();
+
+  assertDirent(await promise1);
+  await promise2;
+}
+doConcurrentAsyncMixedOps().then(common.mustCall());