fs: ignore prototype properties

This is mainly a minor refactoring to reduce the code overhead with the side effect of removing support for prototype properties in passed through options. Copying options is mostly done for enumerable own properties and prototype properties are ignored.
nodejs · Jan 12, 2020 · 28fc814 · 28fc814
1 parent 4455f60
commit 28fc814
Show file tree

Hide file tree

Showing 5 changed files with 47 additions and 275 deletions.
diff --git a/lib/fs.js b/lib/fs.js
@@ -67,7 +67,6 @@ const { FSReqCallback, statValues } = binding;
 const { toPathIfFileURL } = require('internal/url');
 const internalUtil = require('internal/util');
 const {
-  copyObject,
   Dirent,
   getDirents,
   getOptions,
@@ -1321,25 +1320,20 @@ function appendFile(path, data, options, callback) {
   callback = maybeCallback(callback || options);
   options = getOptions(options, { encoding: 'utf8', mode: 0o666, flag: 'a' });
 
-  // Don't make changes directly on options object
-  options = copyObject(options);
-
   // Force append behavior when using a supplied file descriptor
   if (!options.flag || isFd(path))
-    options.flag = 'a';
+    options = { ...options, flag: 'a' };
 
   fs.writeFile(path, data, options, callback);
 }
 
 function appendFileSync(path, data, options) {
   options = getOptions(options, { encoding: 'utf8', mode: 0o666, flag: 'a' });
 
-  // Don't make changes directly on options object
-  options = copyObject(options);
-
   // Force append behavior when using a supplied file descriptor
-  if (!options.flag || isFd(path))
-    options.flag = 'a';
+  if (!options.flag || isFd(path)) {
+    options = { ...options, flag: 'a' };
+  }
 
   fs.writeFileSync(path, data, options);
 }
@@ -1348,10 +1342,7 @@ function watch(filename, options, listener) {
   if (typeof options === 'function') {
     listener = options;
   }
-  options = getOptions(options, {});
-
-  // Don't make changes directly on options object
-  options = copyObject(options);
+  options = { ...getOptions(options, {}) };
 
   if (options.persistent === undefined) options.persistent = true;
   if (options.recursive === undefined) options.recursive = false;

diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js
@@ -25,7 +25,6 @@ const {
 const { isUint8Array } = require('internal/util/types');
 const { rimrafPromises } = require('internal/fs/rimraf');
 const {
-  copyObject,
   getDirents,
   getOptions,
   getStatsFromBinding,
@@ -496,8 +495,9 @@ async function writeFile(path, data, options) {
 
 async function appendFile(path, data, options) {
   options = getOptions(options, { encoding: 'utf8', mode: 0o666, flag: 'a' });
-  options = copyObject(options);
-  options.flag = options.flag || 'a';
+  if (!options.flag) {
+    options = { ...options, flag: 'a' };
+  }
   return writeFile(path, data, options);
 }
 

diff --git a/lib/internal/fs/streams.js b/lib/internal/fs/streams.js
@@ -20,7 +20,6 @@ const { validateNumber } = require('internal/validators');
 const fs = require('fs');
 const { Buffer } = require('buffer');
 const {
-  copyObject,
   getOptions,
 } = require('internal/fs/utils');
 const { Readable, Writable } = require('stream');
@@ -69,7 +68,7 @@ function ReadStream(path, options) {
     return new ReadStream(path, options);
 
   // A little bit bigger buffer and water marks by default
-  options = copyObject(getOptions(options, {}));
+  options = { ...getOptions(options, {}) };
   if (options.highWaterMark === undefined)
     options.highWaterMark = 64 * 1024;
 
@@ -292,10 +291,8 @@ function WriteStream(path, options) {
   if (!(this instanceof WriteStream))
     return new WriteStream(path, options);
 
-  options = copyObject(getOptions(options, {}));
-
   // Only buffers are supported.
-  options.decodeStrings = true;
+  options = { ...getOptions(options, {}), decodeStrings: true };
 
   // For backwards compat do not emit close on destroy.
   if (options.emitClose === undefined) {

diff --git a/lib/internal/fs/utils.js b/lib/internal/fs/utils.js
@@ -166,49 +166,38 @@ for (const name of ReflectOwnKeys(Dirent.prototype)) {
   };
 }
 
-function copyObject(source) {
-  const target = {};
-  for (const key in source)
-    target[key] = source[key];
-  return target;
-}
-
 function getDirents(path, [names, types], callback) {
-  let i;
-  if (typeof callback === 'function') {
-    const len = names.length;
-    let toFinish = 0;
-    callback = once(callback);
-    for (i = 0; i < len; i++) {
-      const type = types[i];
-      if (type === UV_DIRENT_UNKNOWN) {
-        const name = names[i];
-        const idx = i;
-        toFinish++;
-        lazyLoadFs().lstat(pathModule.join(path, name), (err, stats) => {
-          if (err) {
-            callback(err);
-            return;
-          }
-          names[idx] = new DirentFromStats(name, stats);
-          if (--toFinish === 0) {
-            callback(null, names);
-          }
-        });
-      } else {
-        names[i] = new Dirent(names[i], types[i]);
-      }
-    }
-    if (toFinish === 0) {
-      callback(null, names);
-    }
-  } else {
-    const len = names.length;
-    for (i = 0; i < len; i++) {
+  if (typeof callback !== 'function') {
+    for (let i = 0; i < names.length; i++) {
       names[i] = getDirent(path, names[i], types[i]);
     }
     return names;
   }
+  let toFinish = 0;
+  callback = once(callback);
+  for (let i = 0; i < names.length; i++) {
+    const type = types[i];
+    if (type === UV_DIRENT_UNKNOWN) {
+      const name = names[i];
+      const idx = i;
+      toFinish++;
+      lazyLoadFs().lstat(pathModule.join(path, name), (err, stats) => {
+        if (err) {
+          callback(err);
+          return;
+        }
+        names[idx] = new DirentFromStats(name, stats);
+        if (--toFinish === 0) {
+          callback(null, names);
+        }
+      });
+    } else {
+      names[i] = new Dirent(names[i], types[i]);
+    }
+  }
+  if (toFinish === 0) {
+    callback(null, names);
+  }
 }
 
 function getDirent(path, name, type, callback) {
@@ -239,9 +228,7 @@ function getOptions(options, defaultOptions) {
   }
 
   if (typeof options === 'string') {
-    defaultOptions = { ...defaultOptions };
-    defaultOptions.encoding = options;
-    options = defaultOptions;
+    options = { ...defaultOptions, encoding: options };
   } else if (typeof options !== 'object') {
     throw new ERR_INVALID_ARG_TYPE('options', ['string', 'Object'], options);
   }
@@ -271,13 +258,16 @@ function handleErrorFromBinding(ctx) {
 // Check if the path contains null types if it is a string nor Uint8Array,
 // otherwise return silently.
 const nullCheck = hideStackFrames((path, propName, throwError = true) => {
-  const pathIsString = typeof path === 'string';
-  const pathIsUint8Array = isUint8Array(path);
-
   // We can only perform meaningful checks on strings and Uint8Arrays.
-  if ((!pathIsString && !pathIsUint8Array) ||
-      (pathIsString && !path.includes('\u0000')) ||
-      (pathIsUint8Array && !path.includes(0))) {
+  if (typeof path === 'string') {
+    if (!path.includes('\u0000')) {
+      return;
+    }
+  } else if (isUint8Array(path)) {
+    if (!path.includes(0)) {
+      return;
+    }
+  } else {
     return;
   }
 
@@ -654,7 +644,6 @@ const getValidMode = hideStackFrames((mode, type) => {
 module.exports = {
   assertEncoding,
   BigIntStats,  // for testing
-  copyObject,
   Dirent,
   getDirent,
   getDirents,