Skip to content

Commit 3bb9237

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 f0a75ab commit 3bb9237

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
@@ -1288,8 +1288,6 @@ void AppendExceptionLine(Environment* env,
12881288
err_obj->SetHiddenValue(env->processed_string(), True(env->isolate()));
12891289
}
12901290

1291-
char arrow[1024];
1292-
12931291
// Print (filename):(line number): (message).
12941292
node::Utf8Value filename(env->isolate(), message->GetScriptResourceName());
12951293
const char* filename_string = *filename;
@@ -1322,34 +1320,38 @@ void AppendExceptionLine(Environment* env,
13221320
int start = message->GetStartColumn();
13231321
int end = message->GetEndColumn();
13241322

1323+
char arrow[1024];
1324+
int max_off = sizeof(arrow) - 2;
1325+
13251326
int off = snprintf(arrow,
13261327
sizeof(arrow),
13271328
"%s:%i\n%s\n",
13281329
filename_string,
13291330
linenum,
13301331
sourceline_string);
13311332
CHECK_GE(off, 0);
1333+
if (off > max_off) {
1334+
off = max_off;
1335+
}
13321336

13331337
// Print wavy underline (GetUnderline is deprecated).
13341338
for (int i = 0; i < start; i++) {
1335-
if (sourceline_string[i] == '\0' ||
1336-
static_cast<size_t>(off) >= sizeof(arrow)) {
1339+
if (sourceline_string[i] == '\0' || off >= max_off) {
13371340
break;
13381341
}
1339-
CHECK_LT(static_cast<size_t>(off), sizeof(arrow));
1342+
CHECK_LT(off, max_off);
13401343
arrow[off++] = (sourceline_string[i] == '\t') ? '\t' : ' ';
13411344
}
13421345
for (int i = start; i < end; i++) {
1343-
if (sourceline_string[i] == '\0' ||
1344-
static_cast<size_t>(off) >= sizeof(arrow)) {
1346+
if (sourceline_string[i] == '\0' || off >= max_off) {
13451347
break;
13461348
}
1347-
CHECK_LT(static_cast<size_t>(off), sizeof(arrow));
1349+
CHECK_LT(off, max_off);
13481350
arrow[off++] = '^';
13491351
}
1350-
CHECK_LE(static_cast<size_t>(off - 1), sizeof(arrow) - 1);
1351-
arrow[off++] = '\n';
1352-
arrow[off] = '\0';
1352+
CHECK_LE(off, max_off);
1353+
arrow[off] = '\n';
1354+
arrow[off + 1] = '\0';
13531355

13541356
Local<String> arrow_str = String::NewFromUtf8(env->isolate(), arrow);
13551357
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)