Skip to content

Commit b028978

Browse files
skomskirvagg
authored andcommitted
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: #2404 Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: Fedor Indutny <[email protected]>
1 parent 8372ea2 commit b028978

File tree

3 files changed

+19
-12
lines changed

3 files changed

+19
-12
lines changed

src/node.cc

+13-11
Original file line numberDiff line numberDiff line change
@@ -1321,8 +1321,6 @@ void AppendExceptionLine(Environment* env,
13211321
err_obj->SetHiddenValue(env->processed_string(), True(env->isolate()));
13221322
}
13231323

1324-
char arrow[1024];
1325-
13261324
// Print (filename):(line number): (message).
13271325
node::Utf8Value filename(env->isolate(), message->GetScriptResourceName());
13281326
const char* filename_string = *filename;
@@ -1355,34 +1353,38 @@ void AppendExceptionLine(Environment* env,
13551353
int start = message->GetStartColumn();
13561354
int end = message->GetEndColumn();
13571355

1356+
char arrow[1024];
1357+
int max_off = sizeof(arrow) - 2;
1358+
13581359
int off = snprintf(arrow,
13591360
sizeof(arrow),
13601361
"%s:%i\n%s\n",
13611362
filename_string,
13621363
linenum,
13631364
sourceline_string);
13641365
CHECK_GE(off, 0);
1366+
if (off > max_off) {
1367+
off = max_off;
1368+
}
13651369

13661370
// Print wavy underline (GetUnderline is deprecated).
13671371
for (int i = 0; i < start; i++) {
1368-
if (sourceline_string[i] == '\0' ||
1369-
static_cast<size_t>(off) >= sizeof(arrow)) {
1372+
if (sourceline_string[i] == '\0' || off >= max_off) {
13701373
break;
13711374
}
1372-
CHECK_LT(static_cast<size_t>(off), sizeof(arrow));
1375+
CHECK_LT(off, max_off);
13731376
arrow[off++] = (sourceline_string[i] == '\t') ? '\t' : ' ';
13741377
}
13751378
for (int i = start; i < end; i++) {
1376-
if (sourceline_string[i] == '\0' ||
1377-
static_cast<size_t>(off) >= sizeof(arrow)) {
1379+
if (sourceline_string[i] == '\0' || off >= max_off) {
13781380
break;
13791381
}
1380-
CHECK_LT(static_cast<size_t>(off), sizeof(arrow));
1382+
CHECK_LT(off, max_off);
13811383
arrow[off++] = '^';
13821384
}
1383-
CHECK_LE(static_cast<size_t>(off - 1), sizeof(arrow) - 1);
1384-
arrow[off++] = '\n';
1385-
arrow[off] = '\0';
1385+
CHECK_LE(off, max_off);
1386+
arrow[off] = '\n';
1387+
arrow[off + 1] = '\0';
13861388

13871389
Local<String> arrow_str = String::NewFromUtf8(env->isolate(), arrow);
13881390
Local<Value> msg;

test/fixtures/throws_error5.js

+1
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

test/parallel/test-error-reporting.js

+5-1
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,11 @@ errExec('throws_error4.js', function(err, stdout, stderr) {
5454
assert.ok(/SyntaxError/.test(stderr));
5555
});
5656

57+
// Long exception line doesn't result in stack overflow
58+
errExec('throws_error5.js', function(err, stdout, stderr) {
59+
assert.ok(/SyntaxError/.test(stderr));
60+
});
5761

5862
process.on('exit', function() {
59-
assert.equal(4, exits);
63+
assert.equal(5, exits);
6064
});

0 commit comments

Comments
 (0)