From fe9f5bcd751446ce7755b79a9cf614adf04f0127 Mon Sep 17 00:00:00 2001 From: "Sakthipriyan Vairamani (thefourtheye)" Date: Sun, 9 Oct 2016 19:37:59 +0530 Subject: [PATCH] fs: don't alter user provided `options` object MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This patch makes a copy of the `options` object before the fs module functions alter it. PR-URL: https://github.com/nodejs/node/pull/7831 Fixes: https://github.com/nodejs/node/pull/7655 Reviewed-By: James M Snell Reviewed-By: Nicu Micleușanu Reviewed-By: Rod Vagg --- lib/fs.js | 26 ++++-- test/parallel/test-fs-options-immutable.js | 98 ++++++++++++++++++++++ 2 files changed, 116 insertions(+), 8 deletions(-) create mode 100644 test/parallel/test-fs-options-immutable.js diff --git a/lib/fs.js b/lib/fs.js index 20b6274a666248..d87cdbcd36d48a 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -64,6 +64,13 @@ function getOptions(options, defaultOptions) { return options; } +function copyObject(source, target) { + target = arguments.length >= 2 ? target : {}; + for (const key in source) + target[key] = source[key]; + return target; +} + function rethrow() { // TODO(thefourtheye) Throw error instead of warning in major version > 7 process.emitWarning( @@ -1280,11 +1287,11 @@ fs.appendFile = function(path, data, options, callback) { callback = maybeCallback(arguments[arguments.length - 1]); options = getOptions(options, { encoding: 'utf8', mode: 0o666, flag: 'a' }); - if (!options.flag) - options = util._extend({ flag: 'a' }, options); + // Don't make changes directly on options object + options = copyObject(options); // force append behavior when using a supplied file descriptor - if (isFd(path)) + if (!options.flag || isFd(path)) options.flag = 'a'; fs.writeFile(path, data, options, callback); @@ -1293,11 +1300,11 @@ fs.appendFile = function(path, data, options, callback) { fs.appendFileSync = function(path, data, options) { options = getOptions(options, { encoding: 'utf8', mode: 0o666, flag: 'a' }); - if (!options.flag) - options = util._extend({ flag: 'a' }, options); + // Don't make changes directly on options object + options = copyObject(options); // force append behavior when using a supplied file descriptor - if (isFd(path)) + if (!options.flag || isFd(path)) options.flag = 'a'; fs.writeFileSync(path, data, options); @@ -1355,6 +1362,9 @@ fs.watch = function(filename, options, listener) { } options = getOptions(options, {}); + // Don't make changes directly on options object + options = copyObject(options); + if (options.persistent === undefined) options.persistent = true; if (options.recursive === undefined) options.recursive = false; @@ -1755,7 +1765,7 @@ function ReadStream(path, options) { return new ReadStream(path, options); // a little bit bigger buffer and water marks by default - options = Object.create(getOptions(options, {})); + options = copyObject(getOptions(options, {})); if (options.highWaterMark === undefined) options.highWaterMark = 64 * 1024; @@ -1921,7 +1931,7 @@ function WriteStream(path, options) { if (!(this instanceof WriteStream)) return new WriteStream(path, options); - options = Object.create(getOptions(options, {})); + options = copyObject(getOptions(options, {})); Writable.call(this, options); diff --git a/test/parallel/test-fs-options-immutable.js b/test/parallel/test-fs-options-immutable.js new file mode 100644 index 00000000000000..47796a94a5e79e --- /dev/null +++ b/test/parallel/test-fs-options-immutable.js @@ -0,0 +1,98 @@ +'use strict'; + +/* + * These tests make sure that the `options` object passed to these functions are + * never altered. + * + * Refer: https://github.com/nodejs/node/issues/7655 + */ + +const common = require('../common'); +const assert = require('assert'); +const path = require('path'); +const fs = require('fs'); +const errHandler = (e) => assert.ifError(e); +const options = Object.freeze({}); +common.refreshTmpDir(); + +{ + assert.doesNotThrow(() => + fs.readFile(__filename, options, common.mustCall(errHandler)) + ); + assert.doesNotThrow(() => fs.readFileSync(__filename, options)); +} + +{ + assert.doesNotThrow(() => + fs.readdir(__dirname, options, common.mustCall(errHandler)) + ); + assert.doesNotThrow(() => fs.readdirSync(__dirname, options)); +} + +{ + const sourceFile = path.resolve(common.tmpDir, 'test-readlink'); + const linkFile = path.resolve(common.tmpDir, 'test-readlink-link'); + + fs.writeFileSync(sourceFile, ''); + fs.symlinkSync(sourceFile, linkFile); + + assert.doesNotThrow(() => + fs.readlink(linkFile, options, common.mustCall(errHandler)) + ); + assert.doesNotThrow(() => fs.readlinkSync(linkFile, options)); +} + +{ + const fileName = path.resolve(common.tmpDir, 'writeFile'); + assert.doesNotThrow(() => fs.writeFileSync(fileName, 'ABCD', options)); + assert.doesNotThrow(() => + fs.writeFile(fileName, 'ABCD', options, common.mustCall(errHandler)) + ); +} + +{ + const fileName = path.resolve(common.tmpDir, 'appendFile'); + assert.doesNotThrow(() => fs.appendFileSync(fileName, 'ABCD', options)); + assert.doesNotThrow(() => + fs.appendFile(fileName, 'ABCD', options, common.mustCall(errHandler)) + ); +} + +if (!common.isAix) { + // TODO(thefourtheye) Remove this guard once + // https://github.com/nodejs/node/issues/5085 is fixed + { + let watch; + assert.doesNotThrow(() => watch = fs.watch(__filename, options, () => {})); + watch.close(); + } + + { + assert.doesNotThrow(() => fs.watchFile(__filename, options, () => {})); + fs.unwatchFile(__filename); + } +} + +{ + assert.doesNotThrow(() => fs.realpathSync(__filename, options)); + assert.doesNotThrow(() => + fs.realpath(__filename, options, common.mustCall(errHandler)) + ); +} + +{ + const tempFileName = path.resolve(common.tmpDir, 'mkdtemp-'); + assert.doesNotThrow(() => fs.mkdtempSync(tempFileName, options)); + assert.doesNotThrow(() => + fs.mkdtemp(tempFileName, options, common.mustCall(errHandler)) + ); +} + +{ + const fileName = path.resolve(common.tmpDir, 'streams'); + assert.doesNotThrow(() => { + fs.WriteStream(fileName, options).once('open', () => { + assert.doesNotThrow(() => fs.ReadStream(fileName, options)); + }); + }); +}