Permalink
Browse files

fs: don't alter user provided `options` object

This patch makes a copy of the `options` object before the fs module
functions alter it.

PR-URL: #7831
Fixes: #7655
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Nicu Micleușanu <micnic90@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
  • Loading branch information...
thefourtheye committed Oct 9, 2016
1 parent 7084b9c commit 7542bddddaccd2453c72a67119e7badc4f2a3933
Showing with 116 additions and 8 deletions.
  1. +18 −8 lib/fs.js
  2. +98 −0 test/parallel/test-fs-options-immutable.js
@@ -56,6 +56,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(
@@ -1230,11 +1237,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);
@@ -1243,11 +1250,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);
@@ -1305,6 +1312,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;
@@ -1705,7 +1715,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;
@@ -1871,7 +1881,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);
@@ -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));
});
});
}

0 comments on commit 7542bdd

Please sign in to comment.