From fa868cb77289b9eecb61a6fa904a969ff9d9b9a3 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 7 May 2020 01:30:36 +0200 Subject: [PATCH 1/2] 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. --- src/node_dir.cc | 10 +++++++--- src/node_file.cc | 25 ++++++++++++++++++------- src/node_file.h | 3 ++- 3 files changed, 27 insertions(+), 11 deletions(-) diff --git a/src/node_dir.cc b/src/node_dir.cc index c4aaf4bcd3e8ba..b0904c3d39ddd0 100644 --- a/src/node_dir.cc +++ b/src/node_dir.cc @@ -197,8 +197,8 @@ static MaybeLocal DirentListToArray( } static void AfterDirRead(uv_fs_t* req) { - FSReqBase* req_wrap = FSReqBase::from_req(req); - FSReqAfterScope after(req_wrap, req); + BaseObjectPtr 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 done = Null(isolate); + after.Clear(); req_wrap->Resolve(done); return; } uv_dir_t* dir = static_cast(req->ptr); - req->ptr = nullptr; Local error; Local 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); } diff --git a/src/node_file.cc b/src/node_file.cc index be18b18fddd413..96adbc9e756688 100644 --- a/src/node_file.cc +++ b/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 wrap { wrap_ }; + Local exception = + UVException(wrap_->env()->isolate(), + req->result, + wrap_->syscall(), + nullptr, + req->path, + wrap_->data()); + Clear(); + wrap->Reject(exception); } bool FSReqAfterScope::Proceed() { diff --git a/src/node_file.h b/src/node_file.h index 7690f272848425..39c9034f669ea7 100644 --- a/src/node_file.h +++ b/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 wrap_; uv_fs_t* req_ = nullptr; v8::HandleScope handle_scope_; v8::Context::Scope context_scope_; From e287d24aaa94fc6de831cb54d707aebf0a01de30 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 7 May 2020 02:16:17 +0200 Subject: [PATCH 2/2] 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. --- doc/api/errors.md | 9 +++++++ lib/internal/errors.js | 3 +++ lib/internal/fs/dir.js | 35 ++++++++++++++++++++++++++++ test/parallel/test-fs-opendir.js | 40 ++++++++++++++++++++++++++++++++ 4 files changed, 87 insertions(+) diff --git a/doc/api/errors.md b/doc/api/errors.md index 172eadf7686794..ecd76a653e029c 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -855,6 +855,15 @@ An unknown Diffie-Hellman group name was given. See The [`fs.Dir`][] was previously closed. + +### `ERR_DIR_CONCURRENT_OPERATION` + + +A synchronous read or close call was attempted on an [`fs.Dir`][] which has +ongoing asynchronous operations. + ### `ERR_DNS_SET_SERVERS_FAILED` diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 2a162f14b24206..ae78b6a517a918 100644 --- a/lib/internal/errors.js +++ b/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', diff --git a/lib/internal/fs/dir.js b/lib/internal/fs/dir.js index e23b2f7725b2a2..b4cd72d3668e25 100644 --- a/lib/internal/fs/dir.js +++ b/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); diff --git a/test/parallel/test-fs-opendir.js b/test/parallel/test-fs-opendir.js index 733d9c7f3e0487..e5233e3a10914a 100644 --- a/test/parallel/test-fs-opendir.js +++ b/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());