From de95b66d2baed193f5112e1c5066c6f40c82bef0 Mon Sep 17 00:00:00 2001 From: Rod Vagg Date: Sat, 23 Jul 2016 15:08:14 +1000 Subject: [PATCH 1/2] fs: return undefined on module read uv fs error PR-URL: https://github.com/nodejs/node/pull/8277 --- src/node_file.cc | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/src/node_file.cc b/src/node_file.cc index 77d11756a2dbe5..7244644fa354f2 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -538,6 +538,7 @@ static void InternalModuleReadFile(const FunctionCallbackInfo& args) { std::vector chars; int64_t offset = 0; + ssize_t numchars; for (;;) { const size_t kBlockSize = 32 << 10; const size_t start = chars.size(); @@ -548,11 +549,13 @@ static void InternalModuleReadFile(const FunctionCallbackInfo& args) { buf.len = kBlockSize; uv_fs_t read_req; - const ssize_t numchars = - uv_fs_read(loop, &read_req, fd, &buf, 1, offset, nullptr); + numchars = uv_fs_read(loop, &read_req, fd, &buf, 1, offset, nullptr); uv_fs_req_cleanup(&read_req); - CHECK_GE(numchars, 0); + if (numchars < 0) { + break; + } + if (static_cast(numchars) < kBlockSize) { chars.resize(start + numchars); } @@ -566,17 +569,21 @@ static void InternalModuleReadFile(const FunctionCallbackInfo& args) { CHECK_EQ(0, uv_fs_close(loop, &close_req, fd, nullptr)); uv_fs_req_cleanup(&close_req); - size_t start = 0; - if (chars.size() >= 3 && 0 == memcmp(&chars[0], "\xEF\xBB\xBF", 3)) { - start = 3; // Skip UTF-8 BOM. - } + if (numchars < 0) { + args.GetReturnValue().Set(Undefined(env->isolate())); + } else { + size_t start = 0; + if (chars.size() >= 3 && 0 == memcmp(&chars[0], "\xEF\xBB\xBF", 3)) { + start = 3; // Skip UTF-8 BOM. + } - Local chars_string = - String::NewFromUtf8(env->isolate(), - &chars[start], - String::kNormalString, - chars.size() - start); - args.GetReturnValue().Set(chars_string); + Local chars_string = + String::NewFromUtf8(env->isolate(), + &chars[start], + String::kNormalString, + chars.size() - start); + args.GetReturnValue().Set(chars_string); + } } // Used to speed up module loading. Returns 0 if the path refers to From 39db98ca5e20b1c28b5058c7b049747324d955bd Mon Sep 17 00:00:00 2001 From: Rod Vagg Date: Sat, 23 Jul 2016 15:29:14 +1000 Subject: [PATCH 2/2] module: apply null bytes check to module read path PR-URL: https://github.com/nodejs/node/pull/8277 --- lib/fs.js | 3 + lib/module.js | 18 ++++++ test/parallel/test-fs-null-bytes.js | 86 +++++++++++++++++------------ 3 files changed, 73 insertions(+), 34 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 2fc22b1bddecc0..78cb73167b1dbc 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -130,6 +130,9 @@ function assertEncoding(encoding) { } } +// This is duplicated in module.js and needs to be moved to internal/fs.js +// once it is OK again to include internal/ resources in fs.js. +// See: https://github.com/nodejs/node/pull/6413 function nullCheck(path, callback) { if (('' + path).indexOf('\u0000') !== -1) { var er = new Error('Path must be a string without null bytes'); diff --git a/lib/module.js b/lib/module.js index fe9700f7a673ae..9a2b65f4f6da7d 100644 --- a/lib/module.js +++ b/lib/module.js @@ -34,6 +34,22 @@ function stat(filename) { stat.cache = null; +// This is duplicated from fs.js and needs to be moved to internal/fs.js +// once it is OK again to include internal/ resources in fs.js. +// See: https://github.com/nodejs/node/pull/6413 +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 (typeof callback !== 'function') + throw er; + process.nextTick(callback, er); + return false; + } + return true; +} + + function Module(id, parent) { this.id = id; this.exports = {}; @@ -135,6 +151,8 @@ function tryExtensions(p, exts, isMain) { var warned = false; Module._findPath = function(request, paths, isMain) { + nullCheck(request); + if (path.isAbsolute(request)) { paths = ['']; } else if (!paths || paths.length === 0) { diff --git a/test/parallel/test-fs-null-bytes.js b/test/parallel/test-fs-null-bytes.js index 3c70d2953ca0f4..a2bc427d77e02c 100644 --- a/test/parallel/test-fs-null-bytes.js +++ b/test/parallel/test-fs-null-bytes.js @@ -2,52 +2,54 @@ var common = require('../common'); var assert = require('assert'); var fs = require('fs'); +var expectedError = /Path must be a string without null bytes/; 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)); + assert(er && er.message.match(expectedError)); assert.equal(er.code, 'ENOENT'); }); if (sync) assert.throws(function() { - console.error(sync.name, argsSync); - sync.apply(null, argsSync); - }, expected); + console.error(`fs.${sync}()`, argsSync); + fs[sync].apply(null, argsSync); + }, expectedError); - if (async) - async.apply(null, argsAsync); + if (async) { + console.error(`fs.${async}()`, argsAsync); + fs[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); -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', common.fail); -check(fs.utimes, fs.utimesSync, 'foo\u0000bar', 0, 0); -check(null, fs.watch, 'foo\u0000bar', common.fail); -check(null, fs.watchFile, 'foo\u0000bar', common.fail); -check(fs.writeFile, fs.writeFileSync, 'foo\u0000bar'); +check('access', 'accessSync', 'foo\u0000bar'); +check('access', 'accessSync', 'foo\u0000bar', 'F_OK'); +check('appendFile', 'appendFileSync', 'foo\u0000bar'); +check('chmod', 'chmodSync', 'foo\u0000bar', '0644'); +check('chown', 'chownSync', 'foo\u0000bar', 12, 34); +check('link', 'linkSync', 'foo\u0000bar', 'foobar'); +check('link', 'linkSync', 'foobar', 'foo\u0000bar'); +check('lstat', 'lstatSync', 'foo\u0000bar'); +check('mkdir', 'mkdirSync', 'foo\u0000bar', '0755'); +check('open', 'openSync', 'foo\u0000bar', 'r'); +check('readFile', 'readFileSync', 'foo\u0000bar'); +check('readdir', 'readdirSync', 'foo\u0000bar'); +check('readlink', 'readlinkSync', 'foo\u0000bar'); +check('realpath', 'realpathSync', 'foo\u0000bar'); +check('rename', 'renameSync', 'foo\u0000bar', 'foobar'); +check('rename', 'renameSync', 'foobar', 'foo\u0000bar'); +check('rmdir', 'rmdirSync', 'foo\u0000bar'); +check('stat', 'statSync', 'foo\u0000bar'); +check('symlink', 'symlinkSync', 'foo\u0000bar', 'foobar'); +check('symlink', 'symlinkSync', 'foobar', 'foo\u0000bar'); +check('truncate', 'truncateSync', 'foo\u0000bar'); +check('unlink', 'unlinkSync', 'foo\u0000bar'); +check(null, 'unwatchFile', 'foo\u0000bar', common.fail); +check('utimes', 'utimesSync', 'foo\u0000bar', 0, 0); +check(null, 'watch', 'foo\u0000bar', common.fail); +check(null, 'watchFile', 'foo\u0000bar', common.fail); +check('writeFile', 'writeFileSync', 'foo\u0000bar'); // an 'error' for exists means that it doesn't exist. // one of many reasons why this file is the absolute worst. @@ -55,3 +57,19 @@ fs.exists('foo\u0000bar', function(exists) { assert(!exists); }); assert(!fs.existsSync('foo\u0000bar')); + +function checkRequire(arg) { + assert.throws(function() { + console.error(`require(${JSON.stringify(arg)})`); + require(arg); + }, expectedError); +} + +checkRequire('\u0000'); +checkRequire('foo\u0000bar'); +checkRequire('foo\u0000'); +checkRequire('foo/\u0000'); +checkRequire('foo/\u0000.js'); +checkRequire('\u0000/foo'); +checkRequire('./foo/\u0000'); +checkRequire('./\u0000/foo');