FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fs: forbid concurrent operations on Dir handle · nodejs/node@c7c420e · GitHub

/ node Public

Commit c7c420e

Browse files
addaleax
authored and
codebytere
committed
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: #33274 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
1 parent 12391c7 commit c7c420e

4 files changed

Lines changed: 87 additions & 0 deletions

File tree

‎doc/api/errors.md‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -855,6 +855,15 @@ An unknown Diffie-Hellman group name was given. See
855855

856856
The [`fs.Dir`][] was previously closed.
857857

858+
<a id="ERR_DIR_CONCURRENT_OPERATION"></a>
859+
### `ERR_DIR_CONCURRENT_OPERATION`
860+
<!-- YAML
861+
added: REPLACEME
862+
-->
863+
864+
A synchronous read or close call was attempted on an [`fs.Dir`][] which has
865+
ongoing asynchronous operations.
866+
858867
<a id="ERR_DNS_SET_SERVERS_FAILED"></a>
859868
### `ERR_DNS_SET_SERVERS_FAILED`
860869

‎lib/internal/errors.js‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -788,6 +788,9 @@ E('ERR_CRYPTO_SIGN_KEY_REQUIRED', 'No key provided to sign', Error);
788788
E('ERR_CRYPTO_TIMING_SAFE_EQUAL_LENGTH',
789789
'Input buffers must have the same byte length', RangeError);
790790
E('ERR_DIR_CLOSED', 'Directory handle was closed', Error);
791+
E('ERR_DIR_CONCURRENT_OPERATION',
792+
'Cannot do synchronous work on directory handle with concurrent ' +
793+
'asynchronous operations', Error);
791794
E('ERR_DNS_SET_SERVERS_FAILED', 'c-ares failed to set servers: "%s" [%s]',
792795
Error);
793796
E('ERR_DOMAIN_CALLBACK_NOT_AVAILABLE',

‎lib/internal/fs/dir.js‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ const dirBinding = internalBinding('fs_dir');
1212
const {
1313
codes: {
1414
ERR_DIR_CLOSED,
15+
ERR_DIR_CONCURRENT_OPERATION,
1516
ERR_INVALID_CALLBACK,
1617
ERR_MISSING_ARGS
1718
}
@@ -37,6 +38,7 @@ const kDirOptions = Symbol('kDirOptions');
3738
const kDirReadImpl = Symbol('kDirReadImpl');
3839
const kDirReadPromisified = Symbol('kDirReadPromisified');
3940
const kDirClosePromisified = Symbol('kDirClosePromisified');
41+
const kDirOperationQueue = Symbol('kDirOperationQueue');
4042

4143
class Dir {
4244
constructor(handle, path, options) {
@@ -46,6 +48,10 @@ class Dir {
4648
this[kDirPath] = path;
4749
this[kDirClosed] = false;
4850

51+
// Either `null` or an Array of pending operations (= functions to be called
52+
// once the current operation is done).
53+
this[kDirOperationQueue] = null;
54+
4955
this[kDirOptions] = {
5056
bufferSize: 32,
5157
...getOptions(options, {
@@ -79,6 +85,13 @@ class Dir {
7985
throw new ERR_INVALID_CALLBACK(callback);
8086
}
8187

88+
if (this[kDirOperationQueue] !== null) {
89+
this[kDirOperationQueue].push(() => {
90+
this[kDirReadImpl](maybeSync, callback);
91+
});
92+
return;
93+
}
94+
8295
if (this[kDirBufferedEntries].length > 0) {
8396
const [ name, type ] = this[kDirBufferedEntries].splice(0, 2);
8497
if (maybeSync)
@@ -90,6 +103,12 @@ class Dir {
90103

91104
const req = new FSReqCallback();
92105
req.oncomplete = (err, result) => {
106+
process.nextTick(() => {
107+
const queue = this[kDirOperationQueue];
108+
this[kDirOperationQueue] = null;
109+
for (const op of queue) op();
110+
});
111+
93112
if (err || result === null) {
94113
return callback(err, result);
95114
}
@@ -98,6 +117,7 @@ class Dir {
98117
getDirent(this[kDirPath], result[0], result[1], callback);
99118
};
100119

120+
this[kDirOperationQueue] = [];
101121
this[kDirHandle].read(
102122
this[kDirOptions].encoding,
103123
this[kDirOptions].bufferSize,
@@ -110,6 +130,10 @@ class Dir {
110130
throw new ERR_DIR_CLOSED();
111131
}
112132

133+
if (this[kDirOperationQueue] !== null) {
134+
throw new ERR_DIR_CONCURRENT_OPERATION();
135+
}
136+
113137
if (this[kDirBufferedEntries].length > 0) {
114138
const [ name, type ] = this[kDirBufferedEntries].splice(0, 2);
115139
return getDirent(this[kDirPath], name, type);
@@ -143,6 +167,13 @@ class Dir {
143167
throw new ERR_INVALID_CALLBACK(callback);
144168
}
145169

170+
if (this[kDirOperationQueue] !== null) {
171+
this[kDirOperationQueue].push(() => {
172+
this.close(callback);
173+
});
174+
return;
175+
}
176+
146177
this[kDirClosed] = true;
147178
const req = new FSReqCallback();
148179
req.oncomplete = callback;
@@ -154,6 +185,10 @@ class Dir {
154185
throw new ERR_DIR_CLOSED();
155186
}
156187

188+
if (this[kDirOperationQueue] !== null) {
189+
throw new ERR_DIR_CONCURRENT_OPERATION();
190+
}
191+
157192
this[kDirClosed] = true;
158193
const ctx = { path: this[kDirPath] };
159194
const result = this[kDirHandle].close(undefined, ctx);

‎test/parallel/test-fs-opendir.js‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,10 @@ const dirclosedError = {
3333
code: 'ERR_DIR_CLOSED'
3434
};
3535

36+
const dirconcurrentError = {
37+
code: 'ERR_DIR_CONCURRENT_OPERATION'
38+
};
39+
3640
const invalidCallbackObj = {
3741
code: 'ERR_INVALID_CALLBACK',
3842
name: 'TypeError'
@@ -230,3 +234,39 @@ async function doAsyncIterDirClosedTest() {
230234
assert.throws(() => dir.close(), dirclosedError);
231235
}
232236
doAsyncIterDirClosedTest().then(common.mustCall());
237+
238+
// Check that readSync() and closeSync() during read() throw exceptions
239+
async function doConcurrentAsyncAndSyncOps() {
240+
const dir = await fs.promises.opendir(testDir);
241+
const promise = dir.read();
242+
243+
assert.throws(() => dir.closeSync(), dirconcurrentError);
244+
assert.throws(() => dir.readSync(), dirconcurrentError);
245+
246+
await promise;
247+
dir.closeSync();
248+
}
249+
doConcurrentAsyncAndSyncOps().then(common.mustCall());
250+
251+
// Check that concurrent read() operations don't do weird things.
252+
async function doConcurrentAsyncOps() {
253+
const dir = await fs.promises.opendir(testDir);
254+
const promise1 = dir.read();
255+
const promise2 = dir.read();
256+
257+
assertDirent(await promise1);
258+
assertDirent(await promise2);
259+
dir.closeSync();
260+
}
261+
doConcurrentAsyncOps().then(common.mustCall());
262+
263+
// Check that concurrent read() + close() operations don't do weird things.
264+
async function doConcurrentAsyncMixedOps() {
265+
const dir = await fs.promises.opendir(testDir);
266+
const promise1 = dir.read();
267+
const promise2 = dir.close();
268+
269+
assertDirent(await promise1);
270+
await promise2;
271+
}
272+
doConcurrentAsyncMixedOps().then(common.mustCall());

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL