run wasm out of memory in mac causes node crash due to SIGBUS

[HerrCai0907]

Version

v20.0.0-pre v16.19.0 v18.14.0

Platform

Darwin LSCN1036555 22.3.0 Darwin Kernel Version 22.3.0: Thu Jan 5 20:48:54 PST 2023; root:xnu-8792.81.2~2/RELEASE_ARM64_T6000 arm64

Subsystem

wasm

What steps will reproduce the bug?

run these code in mac, and will get SIGBUS ERROR

node tests/index.cjs 
zsh: bus error  node tests/index.cjs

const fs = require("fs");
const path = require("path");
WebAssembly.compile(fs.readFileSync(path.join(__dirname, "debug.wasm")))
  .then((module) => {
    return WebAssembly.instantiate(module);
  })
  .then((ins) => {
    ins.exports._start();
  });

(module
 (type $none_=>_none (func))
 (global $~lib/memory/__data_end i32 (i32.const 8))
 (global $~lib/memory/__stack_pointer (mut i32) (i32.const 32776))
 (global $~lib/memory/__heap_base i32 (i32.const 32776))
 (memory $0 0)
 (table $0 1 1 funcref)
 (elem $0 (i32.const 1))
 (export "_start" (func $assembly/index/_start))
 (export "memory" (memory $0))
 (func $assembly/index/_start
  memory.size $0
  i32.const 64
  i32.mul
  i32.const 1024
  i32.mul
  i32.const 3
  i32.sub
  i32.load $0
  drop
 )
)

How often does it reproduce? Is there a required condition?

100% reproduce

What is the expected behavior?

behavior should be like in other arch.

wasm://wasm/5c312dfe:1

RuntimeError: memory access out of bounds
at assembly/index/_start (wasm://wasm/5c312dfe:wasm-function[0]:0x63)
at /Users/q540239/oss/as/tests/index.cjs:9:17

What do you see instead?

NA

Additional information

No response

[bnoordhuis]

Ha, I remember asking if we didn't also have to catch SIGBUS to intercept WASM OOB accesses but back then that code was only enabled for Linux, which uses SIGSEGV instead of SIGBUS. Does this patch fix it?

diff --git a/src/node.cc b/src/node.cc
index f92be4b089d..f2e688d7648 100644
--- a/src/node.cc
+++ b/src/node.cc
@@ -553,7 +553,12 @@ static void PlatformInit(ProcessInitializationFlags::Flags flags) {
       memset(&sa, 0, sizeof(sa));
       sa.sa_sigaction = TrapWebAssemblyOrContinue;
       sa.sa_flags = SA_SIGINFO;
-      CHECK_EQ(sigaction(SIGSEGV, &sa, nullptr), 0);
+#if __APPLE__
+      const int nr = SIGBUS;
+#else  // Linux, FreeBSD
+      const int nr = SIGSEGV;
+#endif
+      CHECK_EQ(sigaction(nr, &sa, nullptr), 0);
     }
 #endif  // defined(_WIN32)
     V8::EnableWebAssemblyTrapHandler(false);

[HerrCai0907]

which uses SIGSEGV instead of SIGBUS.

Actually in macos we need to catch two exception, SIGBUS and SIGSEGV

Bus Error occur when a process is trying to access memory that the CPU cannot physically address.In other words the memory tried to access by the program is not a valid memory address.It caused due to alignment issues with the CPU.

Segmentation Fault occur when the program tries to write/read outside the memory allocated for it or when writing memory which can only be read.In other words when the program tries to access the memory to which it doesn’t have access to.

from https://www.geeksforgeeks.org/segmentation-fault-sigsegv-vs-bus-error-sigbus/

[bnoordhuis]

Right, that's good general advice but in this specific case, due to how V8 wires up the trap handler, only SIGBUS is relevant.

[unilynx]

We're seeing the same issue on mac and HerrCai0907's fix works for us. Is there anything we can do to help get this merged in node?

[HerrCai0907]

@unilynx The PR is blocked by ASan and I have no idea about node test environment and don't know how to test and fix this issue.

[unilynx]

@unilynx The PR is blocked by ASan and I have no idea about node test environment and don't know how to test and fix this issue.

I've found https://github.com/nodejs/node/blob/main/.github/workflows/test-asan.yml but that one says runs-on: ubuntu-20.04. Your patch shouldn't have any effect on Linux so maybe that asan failure was just a random occurrence or you forked at an unlucky point ?

Too bad the logs aren't visible anymore but I do see a note at the bottom of https://github.com/nodejs/node/pull/46561/files pointing to test-cluster-primary-error.js - which is not the test you added.

Maybe rebasing your PR would be sufficient?