From c129e8809e209fcadad6912ad11bf478336948ce Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 7 May 2020 02:16:17 +0200 Subject: [PATCH] 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: https://github.com/nodejs/node/pull/33274 Reviewed-By: Colin Ihrig --- 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 974a3b886a3aa0..64bc877bb949a9 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -845,6 +845,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 bfb2dd556073b6..4430d3de42605f 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -782,6 +782,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());