Skip to content

Commit 12391c7

Browse files
addaleaxcodebytere
authored andcommitted
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: #33274 Reviewed-By: Colin Ihrig <[email protected]>
1 parent 182aaf5 commit 12391c7

File tree

3 files changed

+27
-11
lines changed

3 files changed

+27
-11
lines changed

src/node_dir.cc

+7-3
Original file line numberDiff line numberDiff line change
@@ -197,8 +197,8 @@ static MaybeLocal<Array> DirentListToArray(
197197
}
198198

199199
static void AfterDirRead(uv_fs_t* req) {
200-
FSReqBase* req_wrap = FSReqBase::from_req(req);
201-
FSReqAfterScope after(req_wrap, req);
200+
BaseObjectPtr<FSReqBase> req_wrap { FSReqBase::from_req(req) };
201+
FSReqAfterScope after(req_wrap.get(), req);
202202

203203
if (!after.Proceed()) {
204204
return;
@@ -210,12 +210,12 @@ static void AfterDirRead(uv_fs_t* req) {
210210
if (req->result == 0) {
211211
// Done
212212
Local<Value> done = Null(isolate);
213+
after.Clear();
213214
req_wrap->Resolve(done);
214215
return;
215216
}
216217

217218
uv_dir_t* dir = static_cast<uv_dir_t*>(req->ptr);
218-
req->ptr = nullptr;
219219

220220
Local<Value> error;
221221
Local<Array> js_array;
@@ -224,9 +224,13 @@ static void AfterDirRead(uv_fs_t* req) {
224224
req->result,
225225
req_wrap->encoding(),
226226
&error).ToLocal(&js_array)) {
227+
// Clear libuv resources *before* delivering results to JS land because
228+
// that can schedule another operation on the same uv_dir_t. Ditto below.
229+
after.Clear();
227230
return req_wrap->Reject(error);
228231
}
229232

233+
after.Clear();
230234
req_wrap->Resolve(js_array);
231235
}
232236

src/node_file.cc

+18-7
Original file line numberDiff line numberDiff line change
@@ -556,8 +556,15 @@ FSReqAfterScope::FSReqAfterScope(FSReqBase* wrap, uv_fs_t* req)
556556
}
557557

558558
FSReqAfterScope::~FSReqAfterScope() {
559+
Clear();
560+
}
561+
562+
void FSReqAfterScope::Clear() {
563+
if (!wrap_) return;
564+
559565
uv_fs_req_cleanup(wrap_->req());
560-
delete wrap_;
566+
wrap_->Detach();
567+
wrap_.reset();
561568
}
562569

563570
// TODO(joyeecheung): create a normal context object, and
@@ -570,12 +577,16 @@ FSReqAfterScope::~FSReqAfterScope() {
570577
// which is also why the errors should have been constructed
571578
// in JS for more flexibility.
572579
void FSReqAfterScope::Reject(uv_fs_t* req) {
573-
wrap_->Reject(UVException(wrap_->env()->isolate(),
574-
req->result,
575-
wrap_->syscall(),
576-
nullptr,
577-
req->path,
578-
wrap_->data()));
580+
BaseObjectPtr<FSReqBase> wrap { wrap_ };
581+
Local<Value> exception =
582+
UVException(wrap_->env()->isolate(),
583+
req->result,
584+
wrap_->syscall(),
585+
nullptr,
586+
req->path,
587+
wrap_->data());
588+
Clear();
589+
wrap->Reject(exception);
579590
}
580591

581592
bool FSReqAfterScope::Proceed() {

src/node_file.h

+2-1
Original file line numberDiff line numberDiff line change
@@ -184,6 +184,7 @@ class FSReqAfterScope final {
184184
public:
185185
FSReqAfterScope(FSReqBase* wrap, uv_fs_t* req);
186186
~FSReqAfterScope();
187+
void Clear();
187188

188189
bool Proceed();
189190

@@ -195,7 +196,7 @@ class FSReqAfterScope final {
195196
FSReqAfterScope& operator=(const FSReqAfterScope&&) = delete;
196197

197198
private:
198-
FSReqBase* wrap_ = nullptr;
199+
BaseObjectPtr<FSReqBase> wrap_;
199200
uv_fs_t* req_ = nullptr;
200201
v8::HandleScope handle_scope_;
201202
v8::Context::Scope context_scope_;

0 commit comments

Comments
 (0)