Maglev on x64 causes segmentation fault while running TypeScript

[remcohaszing]

Version

v22.0.0

Platform

Linux vali 6.8.0-76060800daily20240311-generic #202403110203171407766522.04~4c8e9a0 SMP PREEMPT_DYNAMIC Thu A x86_64 x86_64 x86_64 GNU/Linux

Subsystem

No response

What steps will reproduce the bug?

On Linux using Node.js 22:

git clone git@github.com:remcohaszing/typescript-bug-58369.git
cd typescript-bug-58369
npm ci
tsc

See also this failed GitHub action: https://github.com/remcohaszing/typescript-bug-58369/actions/runs/8899456400/job/24438867767

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

For this reproduction it’s reproduced consistently on Linux on both my machine and GitHub actions.

While troubleshooting by trimming down the content of node_modules/@types/mdast/index.d.ts, I got into a state where it seemed to happen randomly. The major factor is the 👉 emoji in a comment.

The error did not occur on macOS in the GitHub action, but it did happen consistently for @wooorm on their macbook.

The problem was not reproducible on Windows.

What is the expected behavior? Why is that the expected behavior?

No segmentation fault

What do you see instead?

Segmentation fault (core dumped)

Additional information

This was originally reported to TypeScript: microsoft/TypeScript#58369. This issue contains more information.

This has coincidentally already been fixed for the upcoming TypeScript 5.5. Still, a segfault should not occur.

We were unable to make a smaller reproduction.

[RedYetiDev]

Hi! Could you possibly provide some example code to reproduce this? Preferably code that has been compiled into plain JS.

[remcohaszing]

I would absolutely love to. Unfortunately the bug is reproduced by running tsc. Neither I nor the TypeScript team have been able to make a small reproduction.

It was caused by microsoft/TypeScript#53081 and fixed by microsoft/TypeScript#58339 (unreleased yet).

[RedYetiDev]

AFAICT this doesn't seem like an issue with Node.js itself, but rather a compiler (such as tsc), but I'm no expert, and I'd love to get a second opinion.

[remcohaszing]

Sorry, I now see I forgot to provide a critical piece of information. This is a regression in Node.js 22.0.0. It wasn’t a problem before.

[RedYetiDev]

Ahh, okay, thank you.

[mcollina]

@targos I have a feel we rushed the V8 upgrades.

cc @nodejs/v8 @RafaelGSS

[targos]

It's probably related to V8, but I'm not sure waiting would have changed anything? We released v22.0.0 with the version of V8 that's in current Chrome.

[targos]

Seems specific to Linux or x64 as I cannot reproduce on ARM64 macOS.

[targos]

We also don't know which version of V8 introduced the bug (assuming it's in V8).

[targos]

So, it's specific to x64. I can reproduce with node-v22.0.0-darwin-x64 on Rosetta.

[targos]

I'm going to compile a debug build on one of the Hetzner machines to get a meaningful stack trace.

[wooorm]

I’m on macOS and repro it consistently btw

[targos]

@woorm ARM or Intel?

[wooorm]

It's probably related to V8,

The code that started/stopped crashing in TS had do to with indexing into strings.
The string that TS was looking into starts/stops crashing when there is/isn’t an emoji.

One TS maintainer potentially saw the crash appear/disappear when adding a console.log somewhere. So it may be related to some optimization routine

[wooorm]

I’m on an 2.6 GHz 6-Core Intel Core i7, Sonoma 14.4.1 (23E224).
I have a bunch of the GNU utils tho. So perhaps there could be something that doesn’t happen on mac normally, but my machine looks more like Linux.

[RedYetiDev]

Ignore the repro-exists tag, I didn't mean to add it, and it won't effect anything.

[jakebailey]

Just to give a side by side using https://github.com/remcohaszing/typescript-bug-58369:

$ grep -A8 'function scanJSDocCommentTextToken' ./node_modules/typescript/lib/tsc.js
  function scanJSDocCommentTextToken(inBackticks) {
    fullStartPos = tokenStart = pos;
    tokenFlags = 0 /* None */;
    if (pos >= end) {
      return token = 1 /* EndOfFileToken */;
    }
    for (let ch = text.charCodeAt(pos); pos < end && (!isLineBreak(ch) && ch !== 96 /* backtick */); ch = codePointAt(text, ++pos)) {
      if (!inBackticks) {
        if (ch === 123 /* openBrace */) {
$ node ./node_modules/typescript/lib/tsc.js
[1]    1090384 segmentation fault  node ./node_modules/typescript/lib/tsc.js

Now, add debugger to the loop (console.log works too but is loud):

$ grep -A8 'function scanJSDocCommentTextToken' ./node_modules/typescript/lib/tsc.js
  function scanJSDocCommentTextToken(inBackticks) {
    fullStartPos = tokenStart = pos;
    tokenFlags = 0 /* None */;
    if (pos >= end) {
      return token = 1 /* EndOfFileToken */;
    }
    for (let ch = text.charCodeAt(pos); pos < end && (!isLineBreak(ch) && ch !== 96 /* backtick */); ch = codePointAt(text, ++pos)) {
      debugger; // ADDED
      if (!inBackticks) {
$ node ./node_modules/typescript/lib/tsc.js

I have not been able to extract out a test which just calls the parser via the public API, nor by extracting this code and giving it the same inputs.

[RedYetiDev]

Weird, thanks for the information!

[targos]

(gdb) run node_modules/typescript/lib/tsc.js
Starting program: /home/iojs/tmp-targos/node/out/Debug/node node_modules/typescript/lib/tsc.js
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
[New Thread 0x7ffff7a4f640 (LWP 18826)]
[New Thread 0x7ffff724e640 (LWP 18827)]
[New Thread 0x7ffff6a4d640 (LWP 18828)]
[New Thread 0x7ffff624c640 (LWP 18829)]
[New Thread 0x7ffff5a4b640 (LWP 18830)]
[New Thread 0x7ffff51a9640 (LWP 18831)]

Thread 1 "node" received signal SIGSEGV, Segmentation fault.
0x00005554d878345d in ?? ()
(gdb) bt
#0  0x00005554d878345d in ?? ()
#1  0x00001967bd580c69 in ?? ()
#2  0x000000000000200e in ?? ()
#3  0x0000200e00000000 in ?? ()
#4  0x0000000000000000 in ?? ()

Perfect 🙃

[jakebailey]

Just to note it, you can also add noop(); instead of debugger; if you want a debugger-statement free crasher.

[targos]

This is due to the Maglev compiler. I confirm that ./configure --v8-disable-maglev fixes it.

Maglev was enabled in #51360

/cc @kvakil

[remcohaszing]

The original issue is TypeScript processing @types/mdast, which is used by remark plugins such as remark-emoji. So this is the exact same issue.

[monsanto]

Given that there hasn't been any activity here in a while or on the V8 tracker at all--is there any appetite for reverting Maglev being on by default? Maybe this is an overreaction to Node 22 constantly segfaulting on my desktop, but as-is I wouldn't feel comfortable putting it in production.

It would be different if I could just put --no-maglev-inlining in NODE_OPTIONS, but that doesn't work either.

[dharesign]

FYI this issue with maglev inlining also affects Node 22. With this issue there's no segfault, but the JS doesn't do what it's supposed to.

$ cat foo.js
function issue() {
  let problems = [];

  let i = 0;
  function increment() {
    i = i + 1;
  }
  while (i < 100000000) {
    let prevI = i;
    increment();
    if (i == prevI) {
      problems.push(`${prevI} ${i}`);
    }
  }

  return problems;
}
let didItBreak = issue();
console.log(didItBreak.length);

$ ~/Downloads/node-v22.6.0-darwin-x64/bin/node foo.js
21314

[mcollina]

Apparently there seem to be backport available for the latter.

@nodejs/v8 are we planning an update of v8 before we hit LTS?

[targos]

A full V8 update in v22 is very unlikely (too many breaking changes in the C++ API)

[joyeecheung]

Looking at the related bugs I think we should disable maglev by default for v22, considering it will get stuck with V8 12.4 for ~2 years, and it will probably be increasingly difficult to backport any maglev patches to it even if they get fixed in the upstream. We could continue enabling it on the main branch and maybe consider enabling it in v23 to test it out, but it doesn't look great to ship the crashes to LTS.

[mcollina]

It seems the safest approach, considering that there isn't really much we can do.

[targos]

Maglev was disabled on v22.x with #54384

[jakebailey]

It's still enabled on main, right? So it's still not fixed until V8 is fixed?

[targos]

Do we know that this issue also affects main?

[joyeecheung]

BTW on the V8 slack @deepak1556 mentioned that they were able to reproduce it in Electron with v8 12.4 and 12.6 but not 12.8 so it's likely fixed.

[deepak1556]

Yup the repro mentioned in this post can be run with ELECTRON_RUN_AS_NODE=1 <electron x64 binary> node_modules/.bin/tsc

Simple bisect of our prebuilt binaries showed, first good version is 128.0.6571.0 and last known bad version is 127.0.6521.0. So I can confirm that it is definitely addressed in V8 12.8 but haven't bisected to which commit addressed it.

Here is the chromium changelist url https://chromium.googlesource.com/chromium/src/+log/127.0.6521.0..128.0.6571.0?n=10000&pretty=fuller and V8 changelist https://chromium.googlesource.com/v8/v8/+log/12.7.189..12.8.185?n=10000&pretty=fuller