Skip to content

Commit

Permalink
fs: fix pre-aborted writeFile AbortSignal file leak
Browse files Browse the repository at this point in the history
Fix an issue in writeFile where a file is opened, and not closed
if the abort signal is aborted after the file was opened
but before writing began.
  • Loading branch information
Nitzan Uziely authored and Linkgoron committed Feb 18, 2021
1 parent 88d9268 commit 6e0162c
Show file tree
Hide file tree
Showing 2 changed files with 59 additions and 3 deletions.
11 changes: 11 additions & 0 deletions lib/internal/fs/promises.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ const {
const { opendir } = require('internal/fs/dir');
const {
parseFileMode,
validateAbortSignal,
validateBoolean,
validateBuffer,
validateInteger,
Expand Down Expand Up @@ -667,11 +668,17 @@ async function writeFile(path, data, options) {
data = Buffer.from(data, options.encoding || 'utf8');
}

validateAbortSignal(options.signal);
if (path instanceof FileHandle)
return writeFileHandle(path, data, options.signal);

if (options.signal?.aborted) {
throw lazyDOMException('The operation was aborted', 'AbortError');
}

const fd = await open(path, flag, options.mode);
if (options.signal?.aborted) {
await fd.close();
throw lazyDOMException('The operation was aborted', 'AbortError');
}
return PromisePrototypeFinally(writeFileHandle(fd, data), fd.close);
Expand All @@ -691,6 +698,10 @@ async function readFile(path, options) {
if (path instanceof FileHandle)
return readFileHandle(path, options);

if (options.signal?.aborted) {
throw lazyDOMException('The operation was aborted', 'AbortError');
}

const fd = await open(path, flag, 0o666);
return PromisePrototypeFinally(readFileHandle(fd, options), fd.close);
}
Expand Down
51 changes: 48 additions & 3 deletions test/parallel/test-fs-promises-writefile.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

const common = require('../common');
const fs = require('fs');
const os = require('os');
const cp = require('child_process');
const fsPromises = fs.promises;
const path = require('path');
const tmpdir = require('../common/tmpdir');
Expand All @@ -11,7 +13,9 @@ const tmpDir = tmpdir.path;
tmpdir.refresh();

const dest = path.resolve(tmpDir, 'tmp.txt');
const otherDest = path.resolve(tmpDir, 'tmp-2.txt');
const otherDest2 = path.resolve(tmpDir, 'tmp-2.txt');
const otherDest3 = path.resolve(tmpDir, 'tmp-3.txt');
const otherDest4 = path.resolve(tmpDir, 'tmp-4.txt');
const buffer = Buffer.from('abc'.repeat(1000));
const buffer2 = Buffer.from('xyz'.repeat(1000));

Expand All @@ -25,9 +29,33 @@ async function doWriteWithCancel() {
const controller = new AbortController();
const { signal } = controller;
process.nextTick(() => controller.abort());
assert.rejects(fsPromises.writeFile(otherDest, buffer, { signal }), {
assert.rejects(fsPromises.writeFile(otherDest2, buffer, { signal }), {
name: 'AbortError'
});
}).then(common.mustCall(()=> {
checkIfFileIsOpen(otherDest2);
}));
}

async function doWriteWithCancelPreSync() {
const controller = new AbortController();
const { signal } = controller;
controller.abort();
assert.rejects(fsPromises.writeFile(otherDest3, buffer, { signal }), {
name: 'AbortError'
}).then(common.mustCall(()=> {
checkIfFileIsOpen(otherDest3);
}));
}

async function doWriteWithCancelPostSync() {
const controller = new AbortController();
const { signal } = controller;
controller.abort();
assert.rejects(fsPromises.writeFile(otherDest4, buffer, { signal }), {
name: 'AbortError'
}).then(common.mustCall(()=> {
checkIfFileIsOpen(otherDest4);
}));
}

async function doAppend() {
Expand Down Expand Up @@ -55,4 +83,21 @@ doWrite()
.then(doAppend)
.then(doRead)
.then(doReadWithEncoding)
.then(doWriteWithCancelPreSync)
.then(doWriteWithCancelPostSync)
.then(common.mustCall());

function checkIfFileIsOpen(fileName) {
const platform = os.platform();
if (platform === 'linux' || platform === 'darwin') {
// Tried other ways like rm/stat, but that didn't work.
cp.exec(`lsof -p ${process.pid}`, common.mustCall((err, value) => {
if (err) {
assert.ifError(err);
return;
}
assert.ok(!value.toString().includes(fileName),
`${fileName} is still open, but should be closed`);
}));
}
}

0 comments on commit 6e0162c

Please sign in to comment.