From 337cedd9036a9ab453ad2bc8a01c38f7bfff6116 Mon Sep 17 00:00:00 2001 From: Karl Skomski Date: Thu, 3 Sep 2015 10:10:29 +0200 Subject: [PATCH 1/2] src: fix buffer overflow for long exception lines Long exception lines resulted in a stack buffer overflow or assertion because it was assumed snprintf not counts discarded chars or the assertion itself was incorrect: `(off) >= sizeof(arrow)` --- src/node.cc | 24 +++++++++++++----------- test/fixtures/throws_error5.js | 1 + test/parallel/test-error-reporting.js | 6 +++++- 3 files changed, 19 insertions(+), 12 deletions(-) create mode 100644 test/fixtures/throws_error5.js diff --git a/src/node.cc b/src/node.cc index 57fb6df24e380a..6fe76b69a88dda 100644 --- a/src/node.cc +++ b/src/node.cc @@ -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; @@ -1351,6 +1349,9 @@ 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", @@ -1358,27 +1359,28 @@ void AppendExceptionLine(Environment* env, linenum, sourceline_string); CHECK_GE(off, 0); + 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(off) >= sizeof(arrow)) { + if (sourceline_string[i] == '\0' || off >= max_off) { break; } - CHECK_LT(static_cast(off), sizeof(arrow)); + CHECK_LT(off, max_off); arrow[off++] = (sourceline_string[i] == '\t') ? '\t' : ' '; } for (int i = start; i < end; i++) { - if (sourceline_string[i] == '\0' || - static_cast(off) >= sizeof(arrow)) { + if (sourceline_string[i] == '\0' || off >= max_off) { break; } - CHECK_LT(static_cast(off), sizeof(arrow)); + CHECK_LT(off, max_off); arrow[off++] = '^'; } - CHECK_LE(static_cast(off - 1), sizeof(arrow) - 1); - arrow[off++] = '\n'; - arrow[off] = '\0'; + CHECK_LE(off, max_off); + arrow[off] = '\n'; + arrow[off + 1] = '\0'; Local arrow_str = String::NewFromUtf8(env->isolate(), arrow); Local msg; diff --git a/test/fixtures/throws_error5.js b/test/fixtures/throws_error5.js new file mode 100644 index 00000000000000..a0eba5d8b2fc4a --- /dev/null +++ b/test/fixtures/throws_error5.js @@ -0,0 +1 @@ +0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000k0000000000000000000 diff --git a/test/parallel/test-error-reporting.js b/test/parallel/test-error-reporting.js index 0b5c7652871138..cea60cf90f907e 100644 --- a/test/parallel/test-error-reporting.js +++ b/test/parallel/test-error-reporting.js @@ -54,7 +54,11 @@ errExec('throws_error4.js', function(err, stdout, stderr) { assert.ok(/SyntaxError/.test(stderr)); }); +// Long exception line doesn't result in stack overflow +errExec('throws_error5.js', function(err, stdout, stderr) { + assert.ok(/SyntaxError/.test(stderr)); +}); process.on('exit', function() { - assert.equal(4, exits); + assert.equal(5, exits); }); From c154fdec131da7c0ddc23589f538a20a63e24487 Mon Sep 17 00:00:00 2001 From: Karl Skomski Date: Thu, 3 Sep 2015 10:10:30 +0200 Subject: [PATCH 2/2] src: use standard conform snprintf on windows The version used before returned -1 on truncation which is not standard conform --- src/node_internals.h | 18 ++++++++++-------- test/fixtures/throws_error6.js | 1 + test/parallel/test-error-reporting.js | 9 +++++++-- 3 files changed, 18 insertions(+), 10 deletions(-) create mode 100644 test/fixtures/throws_error6.js diff --git a/src/node_internals.h b/src/node_internals.h index aa198666b62c6c..463fca9b79bd0e 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -95,17 +95,19 @@ void GetSockOrPeerName(const v8::FunctionCallbackInfo& 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 -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 diff --git a/test/fixtures/throws_error6.js b/test/fixtures/throws_error6.js new file mode 100644 index 00000000000000..8e650d4bf4f413 --- /dev/null +++ b/test/fixtures/throws_error6.js @@ -0,0 +1 @@ +00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000k000000000 diff --git a/test/parallel/test-error-reporting.js b/test/parallel/test-error-reporting.js index cea60cf90f907e..88ef5d306e3e37 100644 --- a/test/parallel/test-error-reporting.js +++ b/test/parallel/test-error-reporting.js @@ -54,11 +54,16 @@ errExec('throws_error4.js', function(err, stdout, stderr) { assert.ok(/SyntaxError/.test(stderr)); }); -// Long exception line doesn't result in stack overflow +// 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(5, exits); + assert.equal(6, exits); });