fs: clean up Dir.read() uv_fs_t data before calling into JS

A call into JS can schedule another operation on the same `uv_dir_t`.
In particular, when the handle is closed from the callback for a
directory read operation, there previously was a race condition window:

1. A `dir.read()` operation is submitted to libuv
2. The read operation is finished by libuv, calling `AfterDirRead()`
3. We call into JS
4. JS calls dir.close()
5. libuv posts the close request to a thread in the pool
6. The close request runs, destroying the directory handle
7. `AfterDirRead()` is being exited.

Exiting the `FSReqAfterScope` in step 7 attempts to destroy the original
uv_fs_t` from step 1, which now points to an `uv_dir_t` that has
already been destroyed in step 5.

By forcing the `FSReqAfterScope` to clean up before we call into JS,
we can be sure that no other operations on the same `uv_dir_t` are
submitted concurrently.

This addresses issues observed when running with ASAN/valgrind.

PR-URL: https://github.com/nodejs/node/pull/33274
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

1 files changed, 7 insertions, 3 deletions

diff --git a/src/node_dir.cc b/src/node_dir.cc
index c4aaf4bcd3e..b0904c3d39d 100644
--- a/src/node_dir.cc
+++ b/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);
 }