Skip to content

Commit

Permalink
fs: do not throw exception after creating FSReqCallback
Browse files Browse the repository at this point in the history 
Once an `FSReqCallback` instance is created, it is a GC root until
the underlying fs operation has completed, meaning that it cannot
be garbage collected.

This is a problem when the underlying operation never starts
because an exception is thrown before that happens, for example
as part of parameter validation.

Instead, move all potentially throwing code before the `FSReqCallback`
creation.

PR-URL: #35487
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
  • Loading branch information
@addaleax @danielleadams
addaleax authored and danielleadams committed Oct 6, 2020
1 parent e09f7f0 commit 541be52
Show file tree
Hide file tree
Showing 2 changed files with 55 additions and 34 deletions.
75 changes: 48 additions & 27 deletions lib/fs.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,10 @@
// See https://github.com/libuv/libuv/pull/1501.
const kIoMaxLength = 2 ** 31 - 1;

// When using FSReqCallback, make sure to create the object only *after* all
// parameter validation has happened, so that the objects are not kept in memory
// in case they are created but never used due to an exception.

const {
Map,
MathMax,
Expand Down Expand Up @@ -198,8 +202,10 @@ function access(path, mode, callback) {

path = getValidatedPath(path);
mode = getValidMode(mode, 'access');
callback = makeCallback(callback);

const req = new FSReqCallback();
req.oncomplete = makeCallback(callback);
req.oncomplete = callback;
binding.access(pathModule.toNamespacedPath(path), mode, req);
}

Expand Down Expand Up @@ -307,19 +313,19 @@ function readFile(path, options, callback) {
const context = new ReadFileContext(callback, options.encoding);
context.isUserFd = isFd(path); // File descriptor ownership

const req = new FSReqCallback();
req.context = context;
req.oncomplete = readFileAfterOpen;

if (context.isUserFd) {
process.nextTick(function tick() {
req.oncomplete(null, path);
});
process.nextTick(function tick(context) {
readFileAfterOpen.call({ context }, null, path);
}, context);
return;
}

path = getValidatedPath(path);
const flagsNumber = stringToFlags(options.flags);
path = getValidatedPath(path);

const req = new FSReqCallback();
req.context = context;
req.oncomplete = readFileAfterOpen;
binding.open(pathModule.toNamespacedPath(path),
flagsNumber,
0o666,
Expand Down Expand Up @@ -416,8 +422,10 @@ function readFileSync(path, options) {

function close(fd, callback) {
validateInt32(fd, 'fd', 0);
callback = makeCallback(callback);

const req = new FSReqCallback();
req.oncomplete = makeCallback(callback);
req.oncomplete = callback;
binding.close(fd, req);
}

Expand Down Expand Up @@ -590,12 +598,11 @@ function readv(fd, buffers, position, callback) {

validateInt32(fd, 'fd', /* min */ 0);
validateBufferArray(buffers);
callback = maybeCallback(callback || position);

const req = new FSReqCallback();
req.oncomplete = wrapper;

callback = maybeCallback(callback || position);

if (typeof position !== 'number')
position = null;

Expand Down Expand Up @@ -712,12 +719,11 @@ function writev(fd, buffers, position, callback) {

validateInt32(fd, 'fd', 0);
validateBufferArray(buffers);
callback = maybeCallback(callback || position);

const req = new FSReqCallback();
req.oncomplete = wrapper;

callback = maybeCallback(callback || position);

if (typeof position !== 'number')
position = null;

Expand Down Expand Up @@ -819,8 +825,10 @@ function ftruncate(fd, len = 0, callback) {
validateInt32(fd, 'fd', 0);
validateInteger(len, 'len');
len = MathMax(0, len);
callback = makeCallback(callback);

const req = new FSReqCallback();
req.oncomplete = makeCallback(callback);
req.oncomplete = callback;
binding.ftruncate(fd, len, req);
}

Expand Down Expand Up @@ -989,8 +997,10 @@ function fstat(fd, options = { bigint: false }, callback) {
options = {};
}
validateInt32(fd, 'fd', 0);
callback = makeStatsCallback(callback);

const req = new FSReqCallback(options.bigint);
req.oncomplete = makeStatsCallback(callback);
req.oncomplete = callback;
binding.fstat(fd, options.bigint, req);
}

Expand All @@ -1001,6 +1011,7 @@ function lstat(path, options = { bigint: false }, callback) {
}
callback = makeStatsCallback(callback);
path = getValidatedPath(path);

const req = new FSReqCallback(options.bigint);
req.oncomplete = callback;
binding.lstat(pathModule.toNamespacedPath(path), options.bigint, req);
Expand All @@ -1013,6 +1024,7 @@ function stat(path, options = { bigint: false }, callback) {
}
callback = makeStatsCallback(callback);
path = getValidatedPath(path);

const req = new FSReqCallback(options.bigint);
req.oncomplete = callback;
binding.stat(pathModule.toNamespacedPath(path), options.bigint, req);
Expand Down Expand Up @@ -1070,9 +1082,6 @@ function symlink(target, path, type_, callback_) {
target = getValidatedPath(target, 'target');
path = getValidatedPath(path);

const req = new FSReqCallback();
req.oncomplete = callback;

if (isWindows && type === null) {
let absoluteTarget;
try {
Expand All @@ -1087,18 +1096,25 @@ function symlink(target, path, type_, callback_) {
stat(absoluteTarget, (err, stat) => {
const resolvedType = !err && stat.isDirectory() ? 'dir' : 'file';
const resolvedFlags = stringToSymlinkType(resolvedType);
binding.symlink(preprocessSymlinkDestination(target,
resolvedType,
path),
const destination = preprocessSymlinkDestination(target,
resolvedType,
path);

const req = new FSReqCallback();
req.oncomplete = callback;
binding.symlink(destination,
pathModule.toNamespacedPath(path), resolvedFlags, req);
});
return;
}
}

const destination = preprocessSymlinkDestination(target, type, path);

const flags = stringToSymlinkType(type);
binding.symlink(preprocessSymlinkDestination(target, type, path),
pathModule.toNamespacedPath(path), flags, req);
const req = new FSReqCallback();
req.oncomplete = callback;
binding.symlink(destination, pathModule.toNamespacedPath(path), flags, req);
}

function symlinkSync(target, path, type) {
Expand Down Expand Up @@ -1255,9 +1271,10 @@ function fchown(fd, uid, gid, callback) {
validateInt32(fd, 'fd', 0);
validateInteger(uid, 'uid', -1, kMaxUserId);
validateInteger(gid, 'gid', -1, kMaxUserId);
callback = makeCallback(callback);

const req = new FSReqCallback();
req.oncomplete = makeCallback(callback);
req.oncomplete = callback;
binding.fchown(fd, uid, gid, req);
}

Expand Down Expand Up @@ -1316,8 +1333,10 @@ function futimes(fd, atime, mtime, callback) {
validateInt32(fd, 'fd', 0);
atime = toUnixTimestamp(atime, 'atime');
mtime = toUnixTimestamp(mtime, 'mtime');
callback = makeCallback(callback);

const req = new FSReqCallback();
req.oncomplete = makeCallback(callback);
req.oncomplete = callback;
binding.futimes(fd, atime, mtime, req);
}

Expand Down Expand Up @@ -1920,8 +1939,10 @@ function copyFile(src, dest, mode, callback) {
src = pathModule._makeLong(src);
dest = pathModule._makeLong(dest);
mode = getValidMode(mode, 'copyFile');
callback = makeCallback(callback);

const req = new FSReqCallback();
req.oncomplete = makeCallback(callback);
req.oncomplete = callback;
binding.copyFile(src, dest, mode, req);
}

Expand Down
14 changes: 7 additions & 7 deletions lib/internal/fs/read_file_context.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,18 +100,18 @@ class ReadFileContext {
}

close(err) {
if (this.isUserFd) {
process.nextTick(function tick(context) {
readFileAfterClose.call({ context }, null);
}, this);
return;
}

const req = new FSReqCallback();
req.oncomplete = readFileAfterClose;
req.context = this;
this.err = err;

if (this.isUserFd) {
process.nextTick(function tick() {
req.oncomplete(null);
});
return;
}

close(this.fd, req);
}
}
Expand Down

0 comments on commit 541be52

Please sign in to comment.