Skip to content

Commit

Permalink
fs: guard against undefined behavior
Browse files Browse the repository at this point in the history 
Calling close on a file description which is currently in use is
undefined behavior due to implementation details in libuv. Add
a guard against this when using FileHandle.

PR-URL: #34746
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
@ronag
ronag committed Aug 14, 2020
1 parent 5864fca commit a0f87aa
Show file tree
Hide file tree
Showing 3 changed files with 116 additions and 31 deletions.
3 changes: 2 additions & 1 deletion doc/api/fs.md
View file
Expand Up @@ -4548,7 +4548,8 @@ added: v10.0.0
file descriptor is closed, or will be rejected if an error occurs while
closing.

Closes the file descriptor.
Closes the file handle. Will wait for any pending operation on the handle
to complete before completing.

```js
const fsPromises = require('fs').promises;
Expand Down
111 changes: 81 additions & 30 deletions lib/internal/fs/promises.js
View file
Expand Up @@ -14,6 +14,8 @@ const {
MathMin,
NumberIsSafeInteger,
Symbol,
Error,
Promise,
} = primordials;

const {
Expand Down Expand Up @@ -64,6 +66,11 @@ const { promisify } = require('internal/util');

const kHandle = Symbol('kHandle');
const kFd = Symbol('kFd');
const kRefs = Symbol('kRefs');
const kClosePromise = Symbol('kClosePromise');
const kCloseResolve = Symbol('kCloseResolve');
const kCloseReject = Symbol('kCloseReject');

const { kUsePromises } = binding;
const {
JSTransferable, kDeserialize, kTransfer, kTransferList
Expand All @@ -76,6 +83,9 @@ class FileHandle extends JSTransferable {
super();
this[kHandle] = filehandle;
this[kFd] = filehandle ? filehandle.fd : -1;

this[kRefs] = 1;
this[kClosePromise] = null;
}

getAsyncId() {
Expand All @@ -87,70 +97,101 @@ class FileHandle extends JSTransferable {
}

appendFile(data, options) {
return writeFile(this, data, options);
return fsCall(writeFile, this, data, options);
}

chmod(mode) {
return fchmod(this, mode);
return fsCall(fchmod, this, mode);
}

chown(uid, gid) {
return fchown(this, uid, gid);
return fsCall(fchown, this, uid, gid);
}

datasync() {
return fdatasync(this);
return fsCall(fdatasync, this);
}

sync() {
return fsync(this);
return fsCall(fsync, this);
}

read(buffer, offset, length, position) {
return read(this, buffer, offset, length, position);
return fsCall(read, this, buffer, offset, length, position);
}

readv(buffers, position) {
return readv(this, buffers, position);
return fsCall(readv, this, buffers, position);
}

readFile(options) {
return readFile(this, options);
return fsCall(readFile, this, options);
}

stat(options) {
return fstat(this, options);
return fsCall(fstat, this, options);
}

truncate(len = 0) {
return ftruncate(this, len);
return fsCall(ftruncate, this, len);
}

utimes(atime, mtime) {
return futimes(this, atime, mtime);
return fsCall(futimes, this, atime, mtime);
}

write(buffer, offset, length, position) {
return write(this, buffer, offset, length, position);
return fsCall(write, this, buffer, offset, length, position);
}

writev(buffers, position) {
return writev(this, buffers, position);
return fsCall(writev, this, buffers, position);
}

writeFile(data, options) {
return writeFile(this, data, options);
return fsCall(writeFile, this, data, options);
}

close = () => {
this[kFd] = -1;
return this[kHandle].close();
if (this[kFd] === -1) {
return Promise.resolve();
}

if (this[kClosePromise]) {
return this[kClosePromise];
}

this[kRefs]--;
if (this[kRefs] === 0) {
this[kFd] = -1;
this[kClosePromise] = this[kHandle].close().finally(() => {
this[kClosePromise] = undefined;
});
} else {
this[kClosePromise] = new Promise((resolve, reject) => {
this[kCloseResolve] = resolve;
this[kCloseReject] = reject;
}).finally(() => {
this[kClosePromise] = undefined;
this[kCloseReject] = undefined;
this[kCloseResolve] = undefined;
});
}

return this[kClosePromise];
}

[kTransfer]() {
if (this[kClosePromise] || this[kRefs] > 1) {
const DOMException = internalBinding('messaging').DOMException;
throw new DOMException('Cannot transfer FileHandle while in use',
'DataCloneError');
}

const handle = this[kHandle];
this[kFd] = -1;
this[kHandle] = null;
this[kRefs] = 0;

return {
data: { handle },
Expand All @@ -168,9 +209,31 @@ class FileHandle extends JSTransferable {
}
}

function validateFileHandle(handle) {
if (!(handle instanceof FileHandle))
async function fsCall(fn, handle, ...args) {
if (handle[kRefs] === undefined) {
throw new ERR_INVALID_ARG_TYPE('filehandle', 'FileHandle', handle);
}

if (handle.fd === -1) {
// eslint-disable-next-line no-restricted-syntax
const err = new Error('file closed');
err.code = 'EBADF';
err.syscall = fn.name;
throw err;
}

try {
handle[kRefs]++;
return await fn(handle, ...args);
} finally {
handle[kRefs]--;
if (handle[kRefs] === 0) {
handle[kFd] = -1;
handle[kHandle]
.close()
.then(handle[kCloseResolve], handle[kCloseReject]);
}
}
}

async function writeFileHandle(filehandle, data) {
Expand Down Expand Up @@ -249,7 +312,6 @@ async function open(path, flags, mode) {
}

async function read(handle, buffer, offset, length, position) {
validateFileHandle(handle);
validateBuffer(buffer);

if (offset == null) {
Expand Down Expand Up @@ -280,7 +342,6 @@ async function read(handle, buffer, offset, length, position) {
}

async function readv(handle, buffers, position) {
validateFileHandle(handle);
validateBufferArray(buffers);

if (typeof position !== 'number')
Expand All @@ -292,8 +353,6 @@ async function readv(handle, buffers, position) {
}

async function write(handle, buffer, offset, length, position) {
validateFileHandle(handle);

if (buffer.length === 0)
return { bytesWritten: 0, buffer };

Expand Down Expand Up @@ -321,7 +380,6 @@ async function write(handle, buffer, offset, length, position) {
}

async function writev(handle, buffers, position) {
validateFileHandle(handle);
validateBufferArray(buffers);

if (typeof position !== 'number')
Expand All @@ -346,7 +404,6 @@ async function truncate(path, len = 0) {
}

async function ftruncate(handle, len = 0) {
validateFileHandle(handle);
validateInteger(len, 'len');
len = MathMax(0, len);
return binding.ftruncate(handle.fd, len, kUsePromises);
Expand All @@ -364,12 +421,10 @@ async function rmdir(path, options) {
}

async function fdatasync(handle) {
validateFileHandle(handle);
return binding.fdatasync(handle.fd, kUsePromises);
}

async function fsync(handle) {
validateFileHandle(handle);
return binding.fsync(handle.fd, kUsePromises);
}

Expand Down Expand Up @@ -420,7 +475,6 @@ async function symlink(target, path, type_) {
}

async function fstat(handle, options = { bigint: false }) {
validateFileHandle(handle);
const result = await binding.fstat(handle.fd, options.bigint, kUsePromises);
return getStatsFromBinding(result);
}
Expand Down Expand Up @@ -453,7 +507,6 @@ async function unlink(path) {
}

async function fchmod(handle, mode) {
validateFileHandle(handle);
mode = parseFileMode(mode, 'mode');
return binding.fchmod(handle.fd, mode, kUsePromises);
}
Expand Down Expand Up @@ -481,7 +534,6 @@ async function lchown(path, uid, gid) {
}

async function fchown(handle, uid, gid) {
validateFileHandle(handle);
validateUint32(uid, 'uid');
validateUint32(gid, 'gid');
return binding.fchown(handle.fd, uid, gid, kUsePromises);
Expand All @@ -504,7 +556,6 @@ async function utimes(path, atime, mtime) {
}

async function futimes(handle, atime, mtime) {
validateFileHandle(handle);
atime = toUnixTimestamp(atime, 'atime');
mtime = toUnixTimestamp(mtime, 'mtime');
return binding.futimes(handle.fd, atime, mtime, kUsePromises);
Expand Down
33 changes: 33 additions & 0 deletions test/parallel/test-worker-message-port-transfer-filehandle.js
View file
Expand Up @@ -68,3 +68,36 @@ const { once } = require('events');

port1.postMessage('second message');
})().then(common.mustCall());

(async function() {
// Check that a FileHandle with a read in progress cannot be transferred.
const fh = await fs.open(__filename);

const { port1 } = new MessageChannel();

const readPromise = fh.readFile();
assert.throws(() => {
port1.postMessage(fh, [fh]);
}, {
message: 'Cannot transfer FileHandle while in use',
name: 'DataCloneError'
});

assert.deepStrictEqual(await readPromise, await fs.readFile(__filename));
})().then(common.mustCall());

(async function() {
// Check that filehandles with a close in progress cannot be transferred.
const fh = await fs.open(__filename);

const { port1 } = new MessageChannel();

const closePromise = fh.close();
assert.throws(() => {
port1.postMessage(fh, [fh]);
}, {
message: 'Cannot transfer FileHandle while in use',
name: 'DataCloneError'
});
await closePromise;
})().then(common.mustCall());

0 comments on commit a0f87aa

Please sign in to comment.