Skip to content

No abort on require() with null bytes #8277

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

Closed
rvagg wants to merge 2 commits into from
Closed

No abort on require() with null bytes #8277

Changes from 1 commit
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
Next Next commit
fs: return undefined on module read uv fs error 
PR-URL: #8277
  • Loading branch information
@rvagg
rvagg committed Aug 27, 2016
commit de95b66d2baed193f5112e1c5066c6f40c82bef0
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
Copy Markdown
Member

@jasnell jasnell Sep 4, 2016

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
Copy Markdown
Member Author

@rvagg rvagg Sep 4, 2016

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
Copy Markdown
Member

@addaleax addaleax Sep 4, 2016

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
Copy Markdown
Member

@jasnell jasnell Sep 4, 2016

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