Skip to content

Commit

Permalink
fs: ensure nullCheck() callback is a function
Browse files Browse the repository at this point in the history
Currently, nullCheck() will attempt to invoke any truthy value
as a function if the path argument contains a null character.
This commit validates that the callback is actually a function
before trying to invoke it. fs.access() was vulnerable to this
bug, as nullCheck() was called prior to type checking its
callback.

PR-URL: #887
Reviewed-By: Ben Noordhuis <[email protected]>
  • Loading branch information
cjihrig committed Feb 19, 2015
1 parent ed240f4 commit ecef871
Show file tree
Hide file tree
Showing 3 changed files with 10 additions and 5 deletions.
8 changes: 4 additions & 4 deletions lib/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ function nullCheck(path, callback) {
if (('' + path).indexOf('\u0000') !== -1) {
var er = new Error('Path must be a string without null bytes.');
er.code = 'ENOENT';
if (!callback)
if (typeof callback !== 'function')
throw er;
process.nextTick(function() {
callback(er);
Expand Down Expand Up @@ -169,16 +169,16 @@ fs.Stats.prototype.isSocket = function() {
});

fs.access = function(path, mode, callback) {
if (!nullCheck(path, callback))
return;

if (typeof mode === 'function') {
callback = mode;
mode = fs.F_OK;
} else if (typeof callback !== 'function') {
throw new TypeError('callback must be a function');
}

if (!nullCheck(path, callback))
return;

mode = mode | 0;
var req = new FSReqWrap();
req.oncomplete = makeCallback(callback);
Expand Down
4 changes: 4 additions & 0 deletions test/parallel/test-fs-access.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,10 @@ assert.throws(function() {
fs.access(__filename, fs.F_OK);
}, /callback must be a function/);

assert.throws(function() {
fs.access(__filename, fs.F_OK, {});
}, /callback must be a function/);

assert.doesNotThrow(function() {
fs.accessSync(__filename);
});
Expand Down
3 changes: 2 additions & 1 deletion test/parallel/test-fs-null-bytes.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ function check(async, sync) {
async.apply(null, argsAsync);
}

check(fs.access, fs.accessSync, 'foo\u0000bar');
check(fs.access, fs.accessSync, 'foo\u0000bar', fs.F_OK);
check(fs.appendFile, fs.appendFileSync, 'foo\u0000bar');
check(fs.chmod, fs.chmodSync, 'foo\u0000bar', '0644');
check(fs.chown, fs.chownSync, 'foo\u0000bar', 12, 34);
Expand Down Expand Up @@ -52,4 +54,3 @@ fs.exists('foo\u0000bar', function(exists) {
assert(!exists);
});
assert(!fs.existsSync('foo\u0000bar'));

0 comments on commit ecef871

Please sign in to comment.