Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fs: fix memory corruption issues around fs.Dir #33274

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions doc/api/errors.md
Original file line number Diff line number Diff line change
Expand Up @@ -855,6 +855,15 @@ An unknown Diffie-Hellman group name was given. See

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

<a id="ERR_DIR_CONCURRENT_OPERATION"></a>
### `ERR_DIR_CONCURRENT_OPERATION`
<!-- YAML
added: REPLACEME
-->

A synchronous read or close call was attempted on an [`fs.Dir`][] which has
ongoing asynchronous operations.

<a id="ERR_DNS_SET_SERVERS_FAILED"></a>
### `ERR_DNS_SET_SERVERS_FAILED`

Expand Down
3 changes: 3 additions & 0 deletions lib/internal/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
35 changes: 35 additions & 0 deletions lib/internal/fs/dir.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ const dirBinding = internalBinding('fs_dir');
const {
codes: {
ERR_DIR_CLOSED,
ERR_DIR_CONCURRENT_OPERATION,
ERR_INVALID_CALLBACK,
ERR_MISSING_ARGS
}
Expand All @@ -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) {
Expand All @@ -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, {
Expand Down Expand Up @@ -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)
Expand All @@ -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);
}
Expand All @@ -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,
Expand All @@ -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);
Expand Down Expand Up @@ -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;
Expand All @@ -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);
Expand Down
10 changes: 7 additions & 3 deletions src/node_dir.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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);
}

Expand Down
25 changes: 18 additions & 7 deletions src/node_file.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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<FSReqBase> wrap { wrap_ };
Local<Value> exception =
UVException(wrap_->env()->isolate(),
req->result,
wrap_->syscall(),
nullptr,
req->path,
wrap_->data());
Clear();
wrap->Reject(exception);
}

bool FSReqAfterScope::Proceed() {
Expand Down
3 changes: 2 additions & 1 deletion src/node_file.h
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,7 @@ class FSReqAfterScope final {
public:
FSReqAfterScope(FSReqBase* wrap, uv_fs_t* req);
~FSReqAfterScope();
void Clear();

bool Proceed();

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

private:
FSReqBase* wrap_ = nullptr;
BaseObjectPtr<FSReqBase> wrap_;
uv_fs_t* req_ = nullptr;
v8::HandleScope handle_scope_;
v8::Context::Scope context_scope_;
Expand Down
40 changes: 40 additions & 0 deletions test/parallel/test-fs-opendir.js
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down Expand Up @@ -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());