src: fix stack-buffer overflow for long exception lines

[skomski]

Long exception lines result in a stack buffer overflow or assertion abort because the assumption that snprintf not counts discarded chars is wrong and (off) >= sizeof(arrow).

==6098==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffe1a973220 at pc 0x000001a0b09a bp 0x7ffe1a972db0 sp 0x7ffe1a972da8
WRITE of size 1 at 0x7ffe1a973220 thread T0
    #0 0x1a0b099 in node::AppendExceptionLine(node::Environment*, v8::Local<v8::Value>, v8::Local<v8::Message>) /home/skomski/Code/io.js/out/../src/node.cc:1409:16
    #1 0x1a3e14a in node::ContextifyScript::New(v8::FunctionCallbackInfo<v8::Value> const&) /home/skomski/Code/io.js/out/../src/node_contextify.cc:492:9

Address 0x7ffe1a973220 is located in stack of thread T0 at offset 1120 in frame
    #0 0x1a0a9af in node::AppendExceptionLine(node::Environment*, v8::Local<v8::Value>, v8::Local<v8::Message>) /home/skomski/Code/io.js/out/../src/node.cc:1334

  This frame has 4 object(s):
    [32, 56) 'scope'
    [96, 1120) 'arrow' <== Memory access at offset 1120 overflows this variable
    [1248, 2288) 'filename'
    [2416, 3456) 'sourceline'

[thefourtheye]

Isn't this redundant, as you have off >= max_off in the if condition above?

[skomski]

Only replaced asserts from before

[skomski]

@bnoordhuis review?

[mscdex]

This can still cause an assertion to be raised when sourceline_string is really long and snprintf() tries to write past sizeof(arrow).

[mscdex]

If the total length of all of the strings being printed to arrow exceeds sizeof(arrow), snprintf() will truncate the output and return -1 to indicate that truncation has happened. When that happens (like in #2581), CHECK_GE(off, 0) will trigger and cause a C++ assertion.

[skomski]

snprintf returns -1 only on an encoding error else it will return the number of characters that would have been written if if n had been sufficiently large. In #2581 it triggers at https://github.com/nodejs/node/blob/master/src/node.cc#L1379

Based on https://groups.google.com/d/msg/comp.std.c/lIvkxXr5_wE/aVuIpm52ToQJ for an encoding error you have to use mbrtowc() or wcrtomb(), directly or indirectly which you only do with "%lc" or "%ls" format specifiers. I think in that case an assert is enough.

[mscdex]

I'm testing on Windows where a custom snprintf() is used which internally uses _vsprintf_p() which MSDN says:

_vsprintf_p and _vswprintf_p return the number of characters written, not including the terminating null character, or a negative value if an output error occurs.

So on Windows, exceeding sizeof(arrow) must constitute an "output error" and that's why I'm seeing off == -1

[skomski]

Yes the custom snprintf should be replaced with something like this:
The _TRUNCATE negates the non-standard -1 behaviour and VS 2015 has
snprintf.

#if _MSC_VER < 1900
int snprintf(char *buffer, size_t n, const char *format,...){
  va_list argp;
  int ret;
  va_start(argp, format);
  ret = _vscprintf(format, argp);
  vsnprintf_s(buffer, n, _TRUNCATE, format, argp);
  va_end(argp);
  return ret;
}
#endif

[skomski]

@indutny

[indutny]

LGTM

[indutny]

CI: https://ci.nodejs.org/job/node-test-pull-request/223/

[mscdex]

Commit message needs to be updated to reflect the main change.

[skomski]

What happened here https://ci.nodejs.org/job/node-test-commit-osx/385/nodes=osx1010/console? Build process simply died?
Updated commit message.

[rvagg]

yep, weird, I can't explain that one

again: https://ci.nodejs.org/job/node-test-pull-request/225/

[orangemocha]

Regarding the failed CI run, I am hypothesizing that maybe the PR got updated soon after the Jenkins job was started, as it shows above "skomski added some commits on Aug 16, 1970"

The CI job was started around 3:52 AM UTC on 9/1/2015. @skomski did you update the PR shortly after that time? http://www.timeanddate.com/worldclock/fixedtime.html?msg=Jenkins+job&iso=20150901T0352&p1=%3A

[skomski]

@orangemocha I pushed the update at Tue, 01 Sep 2015 08:50:29 GMT (Github API).

[orangemocha]

Never mind then. It could be a GitHub fluke. Thanks for checking.

[skomski]

Last CI test https://ci.nodejs.org/job/node-test-pull-request/225/ passed. Ready to initiate merge build?
Should most likely also land in v4.x.

[trevnorris]

LGTM

[trevnorris]

CI: https://ci.nodejs.org/job/node-merge-commit/152/

[trevnorris]

More test failures:

https://ci.nodejs.org/job/node-test-commit-arm/459/nodes=pi1-raspbian-wheezy-1_of_2/tapTestReport/test.tap-83/

https://ci.nodejs.org/job/node-test-commit-linux/431/nodes=centos7-64/tapTestReport/test.tap-395/

[trevnorris]

Trying again: https://ci.nodejs.org/job/node-merge-commit/156/

[orangemocha]

@trevnorris , assuming that
https://ci.nodejs.org/job/node-test-commit-arm/459/nodes=pi1-raspbian-wheezy-1_of_2/tapTestReport/test.tap-83/

and

https://ci.nodejs.org/job/node-test-commit-linux/431/nodes=centos7-64/tapTestReport/test.tap-395/

are just flaky tests and not related to this change?

[orangemocha]

And now test-cluster-disconnect.js:

https://ci.nodejs.org/job/node-test-commit-arm/464/nodes=pi1-raspbian-wheezy-1_of_2/tapTestReport/test.tap-38/

I don't think this is related to this change but I would appreciate a second opinion.

[trevnorris]

@orangemocha None of these failures could be related to the PR. This change only affects how errors are handled before being printed. So a test would need to have failed before this code path is touched

[skomski]

I fixed the wrong dates on the commits.

Needs another node-merge-commit attempt. It should succeed on the third try.

[jbergstroem]

@skomski I think we should skip rewriting history at this point (ref collaborator guide) but its good to know that it's fixed forward.

[trevnorris]

Thanks. Landed on 80bcab9 and 3a731da.