fs: Raise error when null bytes detected in paths

Reworking of @bnoordhuis's more aggressive approach.

diff --git a/lib/fs.js b/lib/fs.js
index dbca477..8746740 100644 (file)
--- a/lib/fs.js
+++ b/lib/fs.js
@@ -98,6 +98,18 @@ function assertEncoding(encoding) {
   }
 }
 
+function nullCheck(path, callback) {
+  if (('' + path).indexOf('\u0000') !== -1) {
+    var er = new Error('Path must be a string without null bytes.');
+    if (!callback)
+      throw er;
+    process.nextTick(function() {
+      callback(er);
+    });
+    return false;
+  }
+  return true;
+}
 
 fs.Stats = binding.Stats;
 
@@ -134,13 +146,16 @@ fs.Stats.prototype.isSocket = function() {
 };
 
 fs.exists = function(path, callback) {
-  binding.stat(pathModule._makeLong(path), function(err, stats) {
+  if (!nullCheck(path, cb)) return;
+  binding.stat(pathModule._makeLong(path), cb);
+  function cb(err, stats) {
     if (callback) callback(err ? false : true);
-  });
+  }
 };
 
 fs.existsSync = function(path) {
   try {
+    nullCheck(path);
     binding.stat(pathModule._makeLong(path));
     return true;
   } catch (e) {
@@ -362,6 +377,7 @@ fs.open = function(path, flags, mode, callback) {
   callback = makeCallback(arguments[arguments.length - 1]);
   mode = modeNum(mode, 438 /*=0666*/);
 
+  if (!nullCheck(path, callback)) return;
   binding.open(pathModule._makeLong(path),
                stringToFlags(flags),
                mode,
@@ -370,6 +386,7 @@ fs.open = function(path, flags, mode, callback) {
 
 fs.openSync = function(path, flags, mode) {
   mode = modeNum(mode, 438 /*=0666*/);
+  nullCheck(path);
   return binding.open(pathModule._makeLong(path), stringToFlags(flags), mode);
 };
 
@@ -475,12 +492,17 @@ fs.writeSync = function(fd, buffer, offset, length, position) {
 };
 
 fs.rename = function(oldPath, newPath, callback) {
+  callback = makeCallback(callback);
+  if (!nullCheck(oldPath, callback)) return;
+  if (!nullCheck(newPath, callback)) return;
   binding.rename(pathModule._makeLong(oldPath),
                  pathModule._makeLong(newPath),
-                 makeCallback(callback));
+                 callback);
 };
 
 fs.renameSync = function(oldPath, newPath) {
+  nullCheck(oldPath);
+  nullCheck(newPath);
   return binding.rename(pathModule._makeLong(oldPath),
                         pathModule._makeLong(newPath));
 };
@@ -543,10 +565,13 @@ fs.ftruncateSync = function(fd, len) {
 };
 
 fs.rmdir = function(path, callback) {
-  binding.rmdir(pathModule._makeLong(path), makeCallback(callback));
+  callback = makeCallback(callback);
+  if (!nullCheck(path, callback)) return;
+  binding.rmdir(pathModule._makeLong(path), callback);
 };
 
 fs.rmdirSync = function(path) {
+  nullCheck(path);
   return binding.rmdir(pathModule._makeLong(path));
 };
 
@@ -568,12 +593,15 @@ fs.fsyncSync = function(fd) {
 
 fs.mkdir = function(path, mode, callback) {
   if (typeof mode === 'function') callback = mode;
+  callback = makeCallback(callback);
+  if (!nullCheck(path, callback)) return;
   binding.mkdir(pathModule._makeLong(path),
                 modeNum(mode, 511 /*=0777*/),
-                makeCallback(callback));
+                callback);
 };
 
 fs.mkdirSync = function(path, mode) {
+  nullCheck(path);
   return binding.mkdir(pathModule._makeLong(path),
                        modeNum(mode, 511 /*=0777*/));
 };
@@ -587,10 +615,13 @@ fs.sendfileSync = function(outFd, inFd, inOffset, length) {
 };
 
 fs.readdir = function(path, callback) {
-  binding.readdir(pathModule._makeLong(path), makeCallback(callback));
+  callback = makeCallback(callback);
+  if (!nullCheck(path, callback)) return;
+  binding.readdir(pathModule._makeLong(path), callback);
 };
 
 fs.readdirSync = function(path) {
+  nullCheck(path);
   return binding.readdir(pathModule._makeLong(path));
 };
 
@@ -599,11 +630,15 @@ fs.fstat = function(fd, callback) {
 };
 
 fs.lstat = function(path, callback) {
-  binding.lstat(pathModule._makeLong(path), makeCallback(callback));
+  callback = makeCallback(callback);
+  if (!nullCheck(path, callback)) return;
+  binding.lstat(pathModule._makeLong(path), callback);
 };
 
 fs.stat = function(path, callback) {
-  binding.stat(pathModule._makeLong(path), makeCallback(callback));
+  callback = makeCallback(callback);
+  if (!nullCheck(path, callback)) return;
+  binding.stat(pathModule._makeLong(path), callback);
 };
 
 fs.fstatSync = function(fd) {
@@ -611,18 +646,23 @@ fs.fstatSync = function(fd) {
 };
 
 fs.lstatSync = function(path) {
+  nullCheck(path);
   return binding.lstat(pathModule._makeLong(path));
 };
 
 fs.statSync = function(path) {
+  nullCheck(path);
   return binding.stat(pathModule._makeLong(path));
 };
 
 fs.readlink = function(path, callback) {
-  binding.readlink(pathModule._makeLong(path), makeCallback(callback));
+  callback = makeCallback(callback);
+  if (!nullCheck(path, callback)) return;
+  binding.readlink(pathModule._makeLong(path), callback);
 };
 
 fs.readlinkSync = function(path) {
+  nullCheck(path);
   return binding.readlink(pathModule._makeLong(path));
 };
 
@@ -643,6 +683,9 @@ fs.symlink = function(destination, path, type_, callback) {
   var type = (typeof type_ === 'string' ? type_ : null);
   var callback = makeCallback(arguments[arguments.length - 1]);
 
+  if (!nullCheck(destination, callback)) return;
+  if (!nullCheck(path, callback)) return;
+
   binding.symlink(preprocessSymlinkDestination(destination, type),
                   pathModule._makeLong(path),
                   type,
@@ -652,27 +695,39 @@ fs.symlink = function(destination, path, type_, callback) {
 fs.symlinkSync = function(destination, path, type) {
   type = (typeof type === 'string' ? type : null);
 
+  nullCheck(destination);
+  nullCheck(path);
+
   return binding.symlink(preprocessSymlinkDestination(destination, type),
                          pathModule._makeLong(path),
                          type);
 };
 
 fs.link = function(srcpath, dstpath, callback) {
+  callback = makeCallback(callback);
+  if (!nullCheck(srcpath, callback)) return;
+  if (!nullCheck(dstpath, callback)) return;
+
   binding.link(pathModule._makeLong(srcpath),
                pathModule._makeLong(dstpath),
-               makeCallback(callback));
+               callback);
 };
 
 fs.linkSync = function(srcpath, dstpath) {
+  nullCheck(srcpath);
+  nullCheck(dstpath);
   return binding.link(pathModule._makeLong(srcpath),
                       pathModule._makeLong(dstpath));
 };
 
 fs.unlink = function(path, callback) {
-  binding.unlink(pathModule._makeLong(path), makeCallback(callback));
+  callback = makeCallback(callback);
+  if (!nullCheck(path, callback)) return;
+  binding.unlink(pathModule._makeLong(path), callback);
 };
 
 fs.unlinkSync = function(path) {
+  nullCheck(path);
   return binding.unlink(pathModule._makeLong(path));
 };
 
@@ -725,12 +780,15 @@ if (constants.hasOwnProperty('O_SYMLINK')) {
 
 
 fs.chmod = function(path, mode, callback) {
+  callback = makeCallback(callback);
+  if (!nullCheck(path, callback)) return;
   binding.chmod(pathModule._makeLong(path),
                 modeNum(mode),
-                makeCallback(callback));
+                callback);
 };
 
 fs.chmodSync = function(path, mode) {
+  nullCheck(path);
   return binding.chmod(pathModule._makeLong(path), modeNum(mode));
 };
 
@@ -761,10 +819,13 @@ fs.fchownSync = function(fd, uid, gid) {
 };
 
 fs.chown = function(path, uid, gid, callback) {
-  binding.chown(pathModule._makeLong(path), uid, gid, makeCallback(callback));
+  callback = makeCallback(callback);
+  if (!nullCheck(path, callback)) return;
+  binding.chown(pathModule._makeLong(path), uid, gid, callback);
 };
 
 fs.chownSync = function(path, uid, gid) {
+  nullCheck(path);
   return binding.chown(pathModule._makeLong(path), uid, gid);
 };
 
@@ -784,13 +845,16 @@ function toUnixTimestamp(time) {
 fs._toUnixTimestamp = toUnixTimestamp;
 
 fs.utimes = function(path, atime, mtime, callback) {
+  callback = makeCallback(callback);
+  if (!nullCheck(path, callback)) return;
   binding.utimes(pathModule._makeLong(path),
                  toUnixTimestamp(atime),
                  toUnixTimestamp(mtime),
-                 makeCallback(callback));
+                 callback);
 };
 
 fs.utimesSync = function(path, atime, mtime) {
+  nullCheck(path);
   atime = toUnixTimestamp(atime);
   mtime = toUnixTimestamp(mtime);
   binding.utimes(pathModule._makeLong(path), atime, mtime);
@@ -929,6 +993,7 @@ function FSWatcher() {
 util.inherits(FSWatcher, EventEmitter);
 
 FSWatcher.prototype.start = function(filename, persistent) {
+  nullCheck(filename);
   var r = this._handle.start(pathModule._makeLong(filename), persistent);
 
   if (r) {
@@ -942,6 +1007,7 @@ FSWatcher.prototype.close = function() {
 };
 
 fs.watch = function(filename) {
+  nullCheck(filename);
   var watcher;
   var options;
   var listener;
@@ -996,6 +1062,7 @@ util.inherits(StatWatcher, EventEmitter);
 
 
 StatWatcher.prototype.start = function(filename, persistent, interval) {
+  nullCheck(filename);
   this._handle.start(pathModule._makeLong(filename), persistent, interval);
 };
 
@@ -1013,6 +1080,7 @@ function inStatWatchers(filename) {
 
 
 fs.watchFile = function(filename) {
+  nullCheck(filename);
   var stat;
   var listener;
 
@@ -1046,6 +1114,7 @@ fs.watchFile = function(filename) {
 };
 
 fs.unwatchFile = function(filename, listener) {
+  nullCheck(filename);
   if (!inStatWatchers(filename)) return;
 
   var stat = statWatchers[filename];

diff --git a/test/simple/test-fs-null-bytes.js b/test/simple/test-fs-null-bytes.js
new file mode 100644 (file)
index 0000000..5dec223
--- /dev/null
+++ b/test/simple/test-fs-null-bytes.js
@@ -0,0 +1,75 @@
+// Copyright Joyent, Inc. and other Node contributors.
+//
+// Permission is hereby granted, free of charge, to any person obtaining a
+// copy of this software and associated documentation files (the
+// "Software"), to deal in the Software without restriction, including
+// without limitation the rights to use, copy, modify, merge, publish,
+// distribute, sublicense, and/or sell copies of the Software, and to permit
+// persons to whom the Software is furnished to do so, subject to the
+// following conditions:
+//
+// The above copyright notice and this permission notice shall be included
+// in all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
+// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
+// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
+// USE OR OTHER DEALINGS IN THE SOFTWARE.
+
+var common = require('../common');
+var assert = require('assert');
+var fs = require('fs');
+
+function check(async, sync) {
+  var expected = /Path must be a string without null bytes./;
+  var argsSync = Array.prototype.slice.call(arguments, 2);
+  var argsAsync = argsSync.concat(function(er) {
+    assert(er && er.message.match(expected));
+  });
+
+  if (sync)
+    assert.throws(function() {
+      console.error(sync.name, argsSync);
+      sync.apply(null, argsSync);
+    }, expected);
+
+  if (async)
+    async.apply(null, argsAsync);
+}
+
+check(fs.appendFile,  fs.appendFileSync,  'foo\u0000bar');
+check(fs.chmod,       fs.chmodSync,       'foo\u0000bar', '0644');
+check(fs.chown,       fs.chownSync,       'foo\u0000bar', 12, 34);
+check(fs.link,        fs.linkSync,        'foo\u0000bar', 'foobar');
+check(fs.link,        fs.linkSync,        'foobar', 'foo\u0000bar');
+check(fs.lstat,       fs.lstatSync,       'foo\u0000bar');
+check(fs.mkdir,       fs.mkdirSync,       'foo\u0000bar', '0755');
+check(fs.open,        fs.openSync,        'foo\u0000bar', 'r');
+check(fs.readFile,    fs.readFileSync,    'foo\u0000bar');
+check(fs.readdir,     fs.readdirSync,     'foo\u0000bar');
+check(fs.readlink,    fs.readlinkSync,    'foo\u0000bar');
+check(fs.realpath,    fs.realpathSync,    'foo\u0000bar');
+check(fs.rename,      fs.renameSync,      'foo\u0000bar', 'foobar');
+check(fs.rename,      fs.renameSync,      'foobar', 'foo\u0000bar');
+check(fs.rmdir,       fs.rmdirSync,       'foo\u0000bar');
+check(fs.stat,        fs.statSync,        'foo\u0000bar');
+check(fs.symlink,     fs.symlinkSync,     'foo\u0000bar', 'foobar');
+check(fs.symlink,     fs.symlinkSync,     'foobar', 'foo\u0000bar');
+check(fs.truncate,    fs.truncateSync,    'foo\u0000bar');
+check(fs.unlink,      fs.unlinkSync,      'foo\u0000bar');
+check(null,           fs.unwatchFile,     'foo\u0000bar', assert.fail);
+check(fs.utimes,      fs.utimesSync,      'foo\u0000bar', 0, 0);
+check(null,           fs.watch,           'foo\u0000bar', assert.fail);
+check(null,           fs.watchFile,       'foo\u0000bar', assert.fail);
+check(fs.writeFile,   fs.writeFileSync,   'foo\u0000bar');
+
+// an 'error' for exists means that it doesn't exist.
+// one of many reasons why this file is the absolute worst.
+fs.exists('foo\u0000bar', function(exists) {
+  assert(!exists);
+});
+assert(!fs.existsSync('foo\u0000bar'));
+