Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No abort on require() with null bytes #8277

Closed
wants to merge 2 commits into from
Closed

No abort on require() with null bytes #8277

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
de95b66
fs: return undefined on module read uv fs error
rvagg Jul 23, 2016
39db98c
module: apply null bytes check to module read path
rvagg Jul 23, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions lib/fs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#6413 should land on Monday.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see a follow-up #8292, thanks.

function nullCheck(path, callback) {
if (('' + path).indexOf('\u0000') !== -1) {
var er = new Error('Path must be a string without null bytes');
Expand Down
18 changes: 18 additions & 0 deletions lib/module.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {};
Expand Down Expand Up @@ -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) {
Expand Down
33 changes: 20 additions & 13 deletions src/node_file.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -538,6 +538,7 @@ static void InternalModuleReadFile(const FunctionCallbackInfo<Value>& args) {

std::vector<char> chars;
int64_t offset = 0;
ssize_t numchars;
for (;;) {
const size_t kBlockSize = 32 << 10;
const size_t start = chars.size();
Expand All @@ -548,11 +549,13 @@ static void InternalModuleReadFile(const FunctionCallbackInfo<Value>& 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<size_t>(numchars) < kBlockSize) {
chars.resize(start + numchars);
}
Expand All @@ -566,17 +569,21 @@ static void InternalModuleReadFile(const FunctionCallbackInfo<Value>& 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()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than returning undefined.... can we throw here? Yes, I know that's semver-major but it's better than an abort and it's likely more correct than undefined.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would short circuit the require lookup algorithm which may potentially find a working match in a further iteration. We can do it but it's not a small breaking change and IMO would be unnecessarily disruptive for little gain.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See also the comment above the function:

// Used to speed up module loading.  Returns the contents of the file as
// a string or undefined when the file cannot be opened.  The speedup
// comes from not creating Error objects on failure.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 works for me.

On Sunday, September 4, 2016, Anna Henningsen notifications@github.com
wrote:

In src/node_file.cc
#8277 (comment):

@@ -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()));

See also the comment above the function:

// Used to speed up module loading. Returns the contents of the file as// a string or undefined when the file cannot be opened. The speedup// comes from not creating Error objects on failure.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/nodejs/node/pull/8277/files/39db98ca5e20b1c28b5058c7b049747324d955bd#r77449453,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAa2eWcj_tAWKN-vBdhw8Czm5djPYWULks5qmpA1gaJpZM4Jt1xz
.

} else {
size_t start = 0;
if (chars.size() >= 3 && 0 == memcmp(&chars[0], "\xEF\xBB\xBF", 3)) {
start = 3; // Skip UTF-8 BOM.
}

Local<String> chars_string =
String::NewFromUtf8(env->isolate(),
&chars[start],
String::kNormalString,
chars.size() - start);
args.GetReturnValue().Set(chars_string);
Local<String> 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
Expand Down
86 changes: 52 additions & 34 deletions test/parallel/test-fs-null-bytes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,56 +2,74 @@
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.
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');