From e817c365ab9a67dd50d7d07d1fe368118ec60874 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 3 Oct 2020 22:44:20 +0200 Subject: [PATCH] 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. PR-URL: https://github.com/nodejs/node/pull/35487 Reviewed-By: James M Snell Reviewed-By: Joyee Cheung --- lib/fs.js | 75 ++++++++++++++++++---------- lib/internal/fs/read_file_context.js | 14 +++--- 2 files changed, 55 insertions(+), 34 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 26ceaba5260dd6..e118b065be2290 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, @@ -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); } @@ -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, @@ -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); } @@ -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; @@ -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; @@ -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); } @@ -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); } @@ -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); @@ -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); @@ -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 { @@ -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) { @@ -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); } @@ -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); } @@ -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); } 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); } }