Browse Source

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)`

PR-URL: https://github.com/nodejs/node/pull/2404
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
v5.x
Karl Skomski 9 years ago
committed by Rod Vagg
parent
commit
3bb923740d
  1. 24
      src/node.cc
  2. 1
      test/fixtures/throws_error5.js
  3. 6
      test/parallel/test-error-reporting.js

24
src/node.cc

@ -1288,8 +1288,6 @@ void AppendExceptionLine(Environment* env,
err_obj->SetHiddenValue(env->processed_string(), True(env->isolate())); err_obj->SetHiddenValue(env->processed_string(), True(env->isolate()));
} }
char arrow[1024];
// Print (filename):(line number): (message). // Print (filename):(line number): (message).
node::Utf8Value filename(env->isolate(), message->GetScriptResourceName()); node::Utf8Value filename(env->isolate(), message->GetScriptResourceName());
const char* filename_string = *filename; const char* filename_string = *filename;
@ -1322,6 +1320,9 @@ void AppendExceptionLine(Environment* env,
int start = message->GetStartColumn(); int start = message->GetStartColumn();
int end = message->GetEndColumn(); int end = message->GetEndColumn();
char arrow[1024];
int max_off = sizeof(arrow) - 2;
int off = snprintf(arrow, int off = snprintf(arrow,
sizeof(arrow), sizeof(arrow),
"%s:%i\n%s\n", "%s:%i\n%s\n",
@ -1329,27 +1330,28 @@ void AppendExceptionLine(Environment* env,
linenum, linenum,
sourceline_string); sourceline_string);
CHECK_GE(off, 0); CHECK_GE(off, 0);
if (off > max_off) {
off = max_off;
}
// Print wavy underline (GetUnderline is deprecated). // Print wavy underline (GetUnderline is deprecated).
for (int i = 0; i < start; i++) { for (int i = 0; i < start; i++) {
if (sourceline_string[i] == '\0' || if (sourceline_string[i] == '\0' || off >= max_off) {
static_cast<size_t>(off) >= sizeof(arrow)) {
break; break;
} }
CHECK_LT(static_cast<size_t>(off), sizeof(arrow)); CHECK_LT(off, max_off);
arrow[off++] = (sourceline_string[i] == '\t') ? '\t' : ' '; arrow[off++] = (sourceline_string[i] == '\t') ? '\t' : ' ';
} }
for (int i = start; i < end; i++) { for (int i = start; i < end; i++) {
if (sourceline_string[i] == '\0' || if (sourceline_string[i] == '\0' || off >= max_off) {
static_cast<size_t>(off) >= sizeof(arrow)) {
break; break;
} }
CHECK_LT(static_cast<size_t>(off), sizeof(arrow)); CHECK_LT(off, max_off);
arrow[off++] = '^'; arrow[off++] = '^';
} }
CHECK_LE(static_cast<size_t>(off - 1), sizeof(arrow) - 1); CHECK_LE(off, max_off);
arrow[off++] = '\n'; arrow[off] = '\n';
arrow[off] = '\0'; arrow[off + 1] = '\0';
Local<String> arrow_str = String::NewFromUtf8(env->isolate(), arrow); Local<String> arrow_str = String::NewFromUtf8(env->isolate(), arrow);
Local<Value> msg; Local<Value> msg;

1
test/fixtures/throws_error5.js

@ -0,0 +1 @@
0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000k0000000000000000000

6
test/parallel/test-error-reporting.js

@ -54,7 +54,11 @@ errExec('throws_error4.js', function(err, stdout, stderr) {
assert.ok(/SyntaxError/.test(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() { process.on('exit', function() {
assert.equal(4, exits); assert.equal(5, exits);
}); });

Loading…
Cancel
Save