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 <[email protected]> Reviewed-By: Nicu Micleușanu <[email protected]> Reviewed-By: Rod Vagg <[email protected]>
nodejs · Oct 10, 2016 · fe9f5bc · fe9f5bc
1 parent 169f485
commit fe9f5bc
Show file tree

Hide file tree

Showing 2 changed files with 116 additions and 8 deletions.
diff --git 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
@@ -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));
+    });
+  });
+}