From 8a58544624dcd4116b1856a058a19ede2f223183 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 3 Oct 2020 22:44:20 +0200 Subject: [PATCH 1/2] fs: do not throw exception after creating FSReqCallback 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. --- lib/fs.js | 71 +++++++++++++++++----------- lib/internal/fs/read_file_context.js | 14 +++--- 2 files changed, 51 insertions(+), 34 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 67a050b4e8c480..c48605bc722ad9 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -198,8 +198,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); } @@ -307,19 +309,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, @@ -416,8 +418,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); } @@ -590,12 +594,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; @@ -712,12 +715,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; @@ -819,8 +821,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); } @@ -989,8 +993,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); } @@ -1001,6 +1007,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); @@ -1013,6 +1020,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); @@ -1070,9 +1078,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 { @@ -1087,18 +1092,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) { @@ -1255,9 +1267,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); } @@ -1316,8 +1329,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); } @@ -1923,8 +1938,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); } diff --git a/lib/internal/fs/read_file_context.js b/lib/internal/fs/read_file_context.js index d7b0e368006f05..5091a34231665b 100644 --- a/lib/internal/fs/read_file_context.js +++ b/lib/internal/fs/read_file_context.js @@ -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); } } From 4f5c989f51a121c618ddf0eab3732e15112c7cd7 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 4 Oct 2020 17:04:12 +0200 Subject: [PATCH 2/2] fixup! fs: do not throw exception after creating FSReqCallback --- lib/fs.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/fs.js b/lib/fs.js index c48605bc722ad9..9316f6d3aabb23 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -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,