From a0f87aab102580503788e15467013a05ab794b5d Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Wed, 12 Aug 2020 12:27:32 +0200 Subject: [PATCH] fs: guard against undefined behavior 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: https://github.com/nodejs/node/pull/34746 Reviewed-By: Anna Henningsen Reviewed-By: David Carlier Reviewed-By: James M Snell --- doc/api/fs.md | 3 +- lib/internal/fs/promises.js | 111 +++++++++++++----- ...worker-message-port-transfer-filehandle.js | 33 ++++++ 3 files changed, 116 insertions(+), 31 deletions(-) diff --git a/doc/api/fs.md b/doc/api/fs.md index 22f034d82fd45b..4e5ff93e91e9a4 100644 --- a/doc/api/fs.md +++ b/doc/api/fs.md @@ -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; diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index 8a9c292056c59e..a938b382eb1268 100644 --- a/lib/internal/fs/promises.js +++ b/lib/internal/fs/promises.js @@ -14,6 +14,8 @@ const { MathMin, NumberIsSafeInteger, Symbol, + Error, + Promise, } = primordials; const { @@ -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 @@ -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() { @@ -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 }, @@ -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) { @@ -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) { @@ -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') @@ -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 }; @@ -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') @@ -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); @@ -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); } @@ -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); } @@ -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); } @@ -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); @@ -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); diff --git a/test/parallel/test-worker-message-port-transfer-filehandle.js b/test/parallel/test-worker-message-port-transfer-filehandle.js index 2184ad575fa4a4..9fabf01b604fc4 100644 --- a/test/parallel/test-worker-message-port-transfer-filehandle.js +++ b/test/parallel/test-worker-message-port-transfer-filehandle.js @@ -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());