SIGSEGV in node::GetErrorSource

[devsnek]

I'm planning to debug this at some point in the next week or so but if someone wants to get started sooner here's what I've got:

Using master as of 9949a2e.
Using linux64.

Process 661308 launched: '/home/snek/bin/node-dev' (x86_64)
Process 661308 stopped
* thread #1, name = 'node-dev', stop reason = signal SIGSEGV: invalid address (fault address: 0x0)
 frame #0: 0x0000555555cbcfd9 node-dev`node::GetErrorSource[abi:cxx11](v8::Isolate*, v8::Local<v8::Context>, v8::Local<v8::Message>, bool*) + 1017
node-dev`node::GetErrorSource[abi:cxx11](v8::Isolate*, v8::Local<v8::Context>, v8::Local<v8::Message>, bool*):
-> 0x555555cbcfd9 <+1017>: cmp byte ptr [rdx], 0x0
 0x555555cbcfdc <+1020>: je 0x555555cbcff5 ; <+1045>
 0x555555cbcfde <+1022>: mov byte ptr [rbp + rcx - 0xca0], 0x5e
 0x555555cbcfe6 <+1030>: add rcx, 0x1
(lldb) bt
* thread #1, name = 'node-dev', stop reason = signal SIGSEGV: invalid address (fault address: 0x0)
 * frame #0: 0x0000555555cbcfd9 node-dev`node::GetErrorSource[abi:cxx11](v8::Isolate*, v8::Local<v8::Context>, v8::Local<v8::Message>, bool*) + 1017
 frame #1: 0x0000555555cbd332 node-dev`node::AppendExceptionLine(node::Environment*, v8::Local<v8::Value>, v8::Local<v8::Message>, node::ErrorHandlingMode) + 130
 frame #2: 0x0000555555cbd775 node-dev`node::ReportFatalException(node::Environment*, v8::Local<v8::Value>, v8::Local<v8::Message>, node::EnhanceFatalException) + 133
 frame #3: 0x0000555555cbe84e node-dev`node::errors::TriggerUncaughtException(v8::Isolate*, v8::Local<v8::Value>, v8::Local<v8::Message>, bool) + 446
 frame #4: 0x0000555555cbe3f1 node-dev`node::errors::PerIsolateMessageListener(v8::Local<v8::Message>, v8::Local<v8::Value>) + 721
 frame #5: 0x0000555555f83409 node-dev`v8::internal::MessageHandler::ReportMessageNoExceptions(v8::internal::Isolate*, v8::internal::MessageLocation const*, v8::internal::Handle<v8::internal::Object>, v8::Local<v8::Value>) + 361
 frame #6: 0x0000555555f83253 node-dev`v8::internal::MessageHandler::ReportMessage(v8::internal::Isolate*, v8::internal::MessageLocation const*, v8::internal::Handle<v8::internal::JSMessageObject>) + 835
 frame #7: 0x0000555555f773dd node-dev`v8::internal::Isolate::ReportPendingMessagesImpl(bool) + 493
 frame #8: 0x0000555555f66197 node-dev`v8::internal::(anonymous namespace)::Invoke(v8::internal::Isolate*, v8::internal::(anonymous namespace)::InvokeParams const&) + 2679
 frame #9: 0x0000555555f656ff node-dev`v8::internal::Execution::Call(v8::internal::Isolate*, v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::Object>, int, v8::internal::Handle<v8::internal::Object>*) + 223
 frame #10: 0x0000555555e4f21e node-dev`v8::Function::Call(v8::Local<v8::Context>, v8::Local<v8::Value>, int, v8::Local<v8::Value>*) + 478
 frame #11: 0x0000555555c8f45f node-dev`node::ExecuteBootstrapper(node::Environment*, char const*, std::vector<v8::Local<v8::String>, std::allocator<v8::Local<v8::String> > >*, std::vector<v8::Local<v8::Value>, std::allocator<v8::Local<v8::Value> > >*) + 127
 frame #12: 0x0000555555c9095e node-dev`node::StartExecution(node::Environment*, char const*) + 430
 frame #13: 0x0000555555c90676 node-dev`node::StartExecution(node::Environment*, std::function<v8::MaybeLocal<v8::Value> (node::StartExecutionCallbackInfo const&)>) + 1254
 frame #14: 0x0000555555c2df3c node-dev`node::LoadEnvironment(node::Environment*) + 76
 frame #15: 0x0000555555d0bc27 node-dev`node::NodeMainInstance::Run() + 183
 frame #16: 0x0000555555c923d3 node-dev`node::Start(int, char**) + 259
 frame #17: 0x00007ffff7a76002 libc.so.6`__libc_start_main + 242
 frame #18: 0x0000555555c2863e node-dev`_start + 46
(lldb) 

'use strict';

const { WASI } = require('wasi');
const fs = require('fs');

const wasi = new WASI();

const m = new WebAssembly.Module(fs.readFileSync('./repro.wasm'));
const w = new WebAssembly.Instance(m, {
 wasi_snapshot_preview1: wasi.wasiImport,
});

w.exports._start(); // throws exception

// rustc repro.rs -O --target wasm32-wasi
fn main() {
 panic!()
}

In the more complex code this was reduced from, the exception thrown by running w.exports.whatever() is from the actual wasm unreachable instruction generated by the panic!(). In this reduced test case, the exception thrown is from node wasi, complaining about wasi.start(w) not being called yet. In both cases, node::GetErrorSource is the point of failure.

[gireeshpunathil]

@devsnek - what does re r rdx show?

[devsnek]

@gireeshpunathil consistently 0x0000800000002c6d

[gireeshpunathil]

apparently it looks a bad address. To know what it represented in the code, we would need to trace back the code a little. Let me try to build one from the said commit.

[addaleax]

@devsnek I can’t reproduce with your code example and the commit you’re pointing to.

• Are you using any ./configure flags or similar?
• Can you share the compiled WASM code to eliminate e.g. dependencies on the rustc version?
• Can you share the Node.js binary you used to reproduce this to match it to the stack trace?

[devsnek]

@addaleax

• no configure flags
• https://s.gc.gy/repro_33578.wasm
• https://s.gc.gy/node_33578

[addaleax]

Hm okay, I’m unable to reproduce, unfortunately.

[addaleax]

Fwiw, comparing the binary you posted with the stack trace leads me to believe that the sourceline[i] access here performs an OOB read:

node/src/node_errors.cc

Line 131 in 54b36e4

if (sourceline[i] == '\0' || off >= kUnderlineBufsize) {

[devsnek]

Got around to making a debug build... Looks like you were right.

Process 131488 launched: '/home/snek/Desktop/misc/nodejs/node/out/Debug/node' (x86_64)
Process 131488 stopped
* thread #1, name = 'node', stop reason = signal SIGSEGV: invalid address (fault address: 0x0)
 frame #0: 0x00005555561a5737 node`node::GetErrorSource[abi:cxx11](isolate=0x000055555a840000, context=(val_ = 0x000055555a8c7310), message=(val_ = 0x000055555a8e9400), added_exception_line=0x00007fffffffb0bf) at node_errors.cc:131:9
 128 	 underline_buf[off++] = (sourceline[i] == '\t') ? '\t' : ' ';
 129 	 }
 130 	 for (int i = start; i < end; i++) {
-> 131 	 if (sourceline[i] == '\0' || off >= kUnderlineBufsize) {
 132 	 break;
 133 	 }
 134 	 CHECK_LT(off, kUnderlineBufsize);

The issue is worse than that though... sourceline is an empty string and start is 28485.

[addaleax]

@devsnek I’m not sure why this is happening for you only, but it does seem like odd behavior coming from the V8 side that the start/source position are outside of the source line string here. We should probably bound-check start and end against sourceline.size() either way.