From 3bb923740d10b5c2272243a39ebcf4a7093e3c6f Mon Sep 17 00:00:00 2001
From: Karl Skomski <karl@skomski.com>
Date: Thu, 3 Sep 2015 10:10:29 +0200
Subject: [PATCH] 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>
---
 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 46ce047f67..268a24d41d 100644
--- a/src/node.cc
+++ b/src/node.cc
@@ -1288,8 +1288,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;
@@ -1322,6 +1320,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",
@@ -1329,27 +1330,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<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);
     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) {
       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;
diff --git a/test/fixtures/throws_error5.js b/test/fixtures/throws_error5.js
new file mode 100644
index 0000000000..a0eba5d8b2
--- /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 0b5c765287..cea60cf90f 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);
 });