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

Jump to bottom

src: fix stack-buffer overflow for long exception lines #2404

Closed
wants to merge 2 commits into from
Closed

src: fix stack-buffer overflow for long exception lines #2404

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
337cedd
src: fix buffer overflow for long exception lines
skomski Sep 3, 2015
c154fde
src: use standard conform snprintf on windows
skomski Sep 3, 2015
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
24 changes: 13 additions & 11 deletions src/node.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1317,8 +1317,6 @@ void AppendExceptionLine(Environment* env,
err_obj->SetHiddenValue(env->processed_string(), True(env->isolate()));
}

char arrow[1024];

// Print (filename):(line number): (message).
node::Utf8Value filename(env->isolate(), message->GetScriptResourceName());
const char* filename_string = *filename;
Expand Down Expand Up @@ -1351,34 +1349,38 @@ void AppendExceptionLine(Environment* env,
int start = message->GetStartColumn();
int end = message->GetEndColumn();

char arrow[1024];
int max_off = sizeof(arrow) - 2;

int off = snprintf(arrow,
sizeof(arrow),
"%s:%i\n%s\n",
filename_string,
linenum,
sourceline_string);
CHECK_GE(off, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

This can still cause an assertion to be raised when sourceline_string is really long and snprintf() tries to write past sizeof(arrow).

Copy link
Contributor

Choose a reason for hiding this comment

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

If the total length of all of the strings being printed to arrow exceeds sizeof(arrow), snprintf() will truncate the output and return -1 to indicate that truncation has happened. When that happens (like in #2581), CHECK_GE(off, 0) will trigger and cause a C++ assertion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

snprintf returns -1 only on an encoding error else it will return the number of characters that would have been written if if n had been sufficiently large. In #2581 it triggers at https://github.com/nodejs/node/blob/master/src/node.cc#L1379

Based on https://groups.google.com/d/msg/comp.std.c/lIvkxXr5_wE/aVuIpm52ToQJ for an encoding error you have to use mbrtowc() or wcrtomb(), directly or indirectly which you only do with "%lc" or "%ls" format specifiers. I think in that case an assert is enough.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm testing on Windows where a custom snprintf() is used which internally uses _vsprintf_p() which MSDN says:

_vsprintf_p and _vswprintf_p return the number of characters written, not including the terminating null character, or a negative value if an output error occurs.

So on Windows, exceeding sizeof(arrow) must constitute an "output error" and that's why I'm seeing off == -1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes the custom snprintf should be replaced with something like this:
The _TRUNCATE negates the non-standard -1 behaviour and VS 2015 has
snprintf.

#if _MSC_VER < 1900
int snprintf(char *buffer, size_t n, const char *format,...){
  va_list argp;
  int ret;
  va_start(argp, format);
  ret = _vscprintf(format, argp);
  vsnprintf_s(buffer, n, _TRUNCATE, format, argp);
  va_end(argp);
  return ret;
}
#endif

if (off > max_off) {
off = max_off;
}

// Print wavy underline (GetUnderline is deprecated).
for (int i = 0; i < start; i++) {
if (sourceline_string[i] == '\0' ||
static_cast<size_t>(off) >= sizeof(arrow)) {
if (sourceline_string[i] == '\0' || off >= max_off) {
break;
}
CHECK_LT(static_cast<size_t>(off), sizeof(arrow));
CHECK_LT(off, max_off);
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this redundant, as you have off >= max_off in the if condition above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only replaced asserts from before

arrow[off++] = (sourceline_string[i] == '\t') ? '\t' : ' ';
}
for (int i = start; i < end; i++) {
if (sourceline_string[i] == '\0' ||
static_cast<size_t>(off) >= sizeof(arrow)) {
if (sourceline_string[i] == '\0' || off >= max_off) {
Copy link
Contributor

Choose a reason for hiding this comment

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

off cannot be greater than max_off as the CHECK_LT in the previous loop ensures that, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previous loop is not always executed

break;
}
CHECK_LT(static_cast<size_t>(off), sizeof(arrow));
CHECK_LT(off, max_off);
arrow[off++] = '^';
}
CHECK_LE(static_cast<size_t>(off - 1), sizeof(arrow) - 1);
arrow[off++] = '\n';
arrow[off] = '\0';
CHECK_LE(off, max_off);
arrow[off] = '\n';
arrow[off + 1] = '\0';

Local<String> arrow_str = String::NewFromUtf8(env->isolate(), arrow);
Local<Value> msg;
Expand Down
18 changes: 10 additions & 8 deletions src/node_internals.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,17 +95,19 @@ void GetSockOrPeerName(const v8::FunctionCallbackInfo<v8::Value>& args) {
#ifdef _WIN32
// emulate snprintf() on windows, _snprintf() doesn't zero-terminate the buffer
// on overflow...
// VS 2015 added a standard conform snprintf
#if defined( _MSC_VER ) && (_MSC_VER < 1900)
#include <stdarg.h>
inline static int snprintf(char* buf, unsigned int len, const char* fmt, ...) {
va_list ap;
va_start(ap, fmt);
int n = _vsprintf_p(buf, len, fmt, ap);
if (len)
buf[len - 1] = '\0';
va_end(ap);
return n;
inline static int snprintf(char *buffer, size_t n, const char *format, ...) {
va_list argp;
va_start(argp, format);
int ret = _vscprintf(format, argp);
vsnprintf_s(buffer, n, _TRUNCATE, format, argp);
va_end(argp);
return ret;
}
#endif
#endif

#if defined(__x86_64__)
# define BITS_PER_LONG 64
Expand Down
1 change: 1 addition & 0 deletions test/fixtures/throws_error5.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions test/fixtures/throws_error6.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 10 additions & 1 deletion test/parallel/test-error-reporting.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,16 @@ errExec('throws_error4.js', function(err, stdout, stderr) {
assert.ok(/SyntaxError/.test(stderr));
});

// Specific long exception line doesn't result in stack overflow
errExec('throws_error5.js', function(err, stdout, stderr) {
assert.ok(/SyntaxError/.test(stderr));
});

// Long exception line with length > errorBuffer doesn't result in assertion
errExec('throws_error6.js', function(err, stdout, stderr) {
assert.ok(/SyntaxError/.test(stderr));
});

process.on('exit', function() {
assert.equal(4, exits);
assert.equal(6, exits);
});