fs: fix memory corruption issues around fs.Dir

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

src/node_dir.cc

@@ -197,8 +197,8 @@ static MaybeLocal<Array> DirentListToArray(
 }
 
 static void AfterDirRead(uv_fs_t* req) {
-  FSReqBase* req_wrap = FSReqBase::from_req(req);
-  FSReqAfterScope after(req_wrap, req);
+  BaseObjectPtr<FSReqBase> req_wrap { FSReqBase::from_req(req) };
+  FSReqAfterScope after(req_wrap.get(), req);
 
   if (!after.Proceed()) {
     return;
@@ -210,12 +210,12 @@ static void AfterDirRead(uv_fs_t* req) {
   if (req->result == 0) {
     // Done
     Local<Value> done = Null(isolate);
+    after.Clear();
     req_wrap->Resolve(done);
     return;
   }
 
   uv_dir_t* dir = static_cast<uv_dir_t*>(req->ptr);
-  req->ptr = nullptr;
 
   Local<Value> error;
   Local<Array> js_array;
@@ -224,9 +224,13 @@ static void AfterDirRead(uv_fs_t* req) {
                          req->result,
                          req_wrap->encoding(),
                          &error).ToLocal(&js_array)) {
+    // Clear libuv resources *before* delivering results to JS land because
+    // that can schedule another operation on the same uv_dir_t. Ditto below.
+    after.Clear();
     return req_wrap->Reject(error);
   }
 
+  after.Clear();
   req_wrap->Resolve(js_array);
 }
 

src/node_file.cc

@@ -556,8 +556,15 @@ FSReqAfterScope::FSReqAfterScope(FSReqBase* wrap, uv_fs_t* req)
 }
 
 FSReqAfterScope::~FSReqAfterScope() {
+  Clear();
+}
+
+void FSReqAfterScope::Clear() {
+  if (!wrap_) return;
+
   uv_fs_req_cleanup(wrap_->req());
-  delete wrap_;
+  wrap_->Detach();
+  wrap_.reset();
 }
 
 // TODO(joyeecheung): create a normal context object, and
@@ -570,12 +577,16 @@ FSReqAfterScope::~FSReqAfterScope() {
 // which is also why the errors should have been constructed
 // in JS for more flexibility.
 void FSReqAfterScope::Reject(uv_fs_t* req) {
-  wrap_->Reject(UVException(wrap_->env()->isolate(),
-                            req->result,
-                            wrap_->syscall(),
-                            nullptr,
-                            req->path,
-                            wrap_->data()));
+  BaseObjectPtr<FSReqBase> wrap { wrap_ };
+  Local<Value> exception =
+      UVException(wrap_->env()->isolate(),
+                  req->result,
+                  wrap_->syscall(),
+                  nullptr,
+                  req->path,
+                  wrap_->data());
+  Clear();
+  wrap->Reject(exception);
 }
 
 bool FSReqAfterScope::Proceed() {

src/node_file.h

@@ -184,6 +184,7 @@ class FSReqAfterScope final {
  public:
   FSReqAfterScope(FSReqBase* wrap, uv_fs_t* req);
   ~FSReqAfterScope();
+  void Clear();
 
   bool Proceed();
 
@@ -195,7 +196,7 @@ class FSReqAfterScope final {
   FSReqAfterScope& operator=(const FSReqAfterScope&&) = delete;
 
  private:
-  FSReqBase* wrap_ = nullptr;
+  BaseObjectPtr<FSReqBase> wrap_;
   uv_fs_t* req_ = nullptr;
   v8::HandleScope handle_scope_;
   v8::Context::Scope context_scope_;

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