fix: Update node's simdutf to 3.2.9 to fix AVX-related crashes

[hardfalcon]

Description of Change

Node 18.16.1 ships with simdutf 3.2.2, which contains a faulty detection routine for AVX/AVX2/AVX512 support, leading to illegal instructions and thus crashes on some x86_64 Linux machines that have mitigations for the "gather data sampling" vulnerability enabled.

Fixes #40441

Checklist

• PR description included and stakeholders cc'd

Release Notes

Notes: none

[welcome]

💖 Thanks for opening this pull request! 💖

We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix.

Examples of commit messages with semantic prefixes:

• fix: don't overwrite prevent_default if default wasn't prevented
• feat: add app.isPackaged() method
• docs: app.isDefaultProtocolClient is now available on Linux

Things that will help get your PR across the finish line:

• Follow the JavaScript, C++, and Python coding style.
• Run npm run lint locally to catch formatting errors earlier.
• Document any user-facing changes you've made following the documentation styleguide.
• Include tests when adding/changing behavior.
• Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

[hardfalcon]

Feel free to add a test case for your test suite, run npm test on it, add or remove check boxes from the Checklist section, modify the commit message, or do whatever else you feel necessary to merge this.

[hardfalcon]

I don't have a clue who the correct stakeholder would be, or how to find out who it might be, but looking at other commits that added patches in the patches/node/ directory of the 26-x-y branch, I think @jkleinsc could be the right person.

I don't have any test cases to add, and I won't be running your test suite on my machine (just compiling this to test if it actually solves the problem for me took already 12 hours), so I've taken the liberty of removing the corresponding checkbox from the checklist.

[codebytere]

We're definitely interested in addressing this, but we can't take a diff like this as our own maintenance burden. This is something that should be PR'd to Node.js itself in the v18.x line, as if it's critical enough i would imagine they'd be open to it.

Additionally - you should be using build-tools. It enables access to our build cache and reduces build times significantly.

[hardfalcon]

This issue actually has been fixed in node's v18.x branch four months ago:
https://github.com/nodejs/node/commits/v18.x/deps/simdutf

[codebytere]

@hardfalcon what specifically is the fix? We would potentially be open to taking a smaller patch but we cannot update the entire version here.

[hardfalcon]

This commit: simdutf/simdutf@55b107f

Note that although the commit message suggest it only fixes detection of AVX-512, it also fixes the faulty detection routine for AVX/AVX2.

On Linux systems with a Skylake (or newer) CPU, where the "gather data sampling" vulnerability has not been fixed through a microcode update (for of their CPUs, Intel have released microcode updates that fix the GDS vulnerability, but for certain Skylake models, they have decided to not release such an update), the Linux kernel disables AVX and AVX2 support if either specifically GDS mitigations are enabled, or if mitigations for spectre/meltdown type vulnerabilities in general are enabled. Unfortunately, buggy software that uses incorrect code to detect if AVX/AVX2 support is available only checks if the CPU reports that it supports AVX/AVX2, but does not check if the operating system does also allow it to use AVX/AVX2, which then leads to an illegal instruction, which results in the whole program/process crashing.

A similar issue affects nodejs (and hence electron) directly through a subtle bug in the base64 implementation it uses, but I'm currently trying to get the fix backported into nodejs (help would be appreciated btw).

[codebytere]

@hardfalcon in that case, if you'd like to move this forward, we'd be open to taking back the patch you linked: simdutf/simdutf@55b107f to target the specific crash.

[anonrig]

cc @lemire

[codebytere]

@hardfalcon do you still intend to work on this?

[lemire]

Updating simdutf to the latest version (4.0.3 or better) should be safe.

[codebytere]

@lemire see #40446 (comment) - it's not about whether it's safe, it is that we do not want to take 6k lines of update as a patch to a release branch. We're happy to take the specific fix though.

[hardfalcon]

@hardfalcon do you still intend to work on this?

I've tried for days, and I'll probably still try some more, but I really wouldn't mind if somebody else took care of this (and of the base64 AVX fix I've mentioned).

Quite frankly, the tooling the Electron project (and the Chromium project, from which Electron seems to have inherited most of this madness) is using, and the development processes derived from that tooling, are orders of magnitude worse than I could have imagined (and that is really an accomplishment, because I didn't exactly have a positive opinion of those two projects or the JS ecosystem to begin with). I've spent literally hours just trying to wrap my head around electron/build-tools and what is required to allow me to apply 1-2 trivial patches and compile and test the result, and I'm pretty close to just giving up.

Even just running npm i @electron/build-tools, adding the result to the PATH, then running e init and e sync for the first time ended up downloading and using about 49 GB on my machine, downloading unfathomable amounts of useless stuff like precompiled PE binaries (I'm running this on a Linux machine, why does it download Windows binaries?!), and producing warnings like Your current version of Yarn is out of date. The latest version is "1.22.19", while you're on "1.15.2". (I don't have yarn installed at all, and I executed those commands on a fresh/empty user account, so that outdated yarn version can only come from Electron's build-tools).

Part of this abysmal development experience is caused by outdated or incorrent documentation, but the root cause appears to be the ridiculous development tools and processes the Chromium and Electron projects are using.

[MarshallOfSound]

@hardfalcon I've gone and made an isolated PR here #40536 that includes just the requisite patch to base64. Although the build system may be slightly arcane in some places please consider how you interact with the project, building a project as large as Chromium + Electron + Node is a mammoth task and the build tooling will reflect that.

In this case for making this change the commands I ran on a fresh linux host (to replicate your environment) were:

e init
e sync -vv
cd third_party/electron_node
# Applied the patch to deps/base64/base64
git commit
e patches node # Exports the current state of node patches
cd ../../electron
git add patches # Changes to node patches are here
git commit

[hardfalcon]

@MarshallOfSound: Thanks for taking care of that patch/fix. I haven't chosen to use Electron, but I'm faced with the reality of more and more software being based on it, and having to deal with the fallout of that development. I consider both Chromium and Electron prime examples of technical debt precisely because of the unmanageable complexities that you mention.

Also, thanks a lot for your concise example. IMO, something like that should be part of Electron's official documentation on patches. By the way, one specific issue that is missing even in your example, and that took me literally hours to figure out, was electron/build-tools#526.

[hardfalcon]

In case you want to merge this: I that I haven't tested this yet, I have only tested the complete update to simdutf 3.2.9 until now.

[lemire]

@codebytere I understand. What I am saying is that we preserve backward compatibility.

[hardfalcon]

What should I do about trope's Invalid Backport message?

It's not really a backport of any commit from the main branch because it only uses one specific commit from a simdutf version that is several versions ahead ((3.2.9 vs 3.2.2), whilst the corresponding commit in the main branches took a whole node version update, which in turn incurred an update from simdutf version 3.2.2 to 3.2.12.

[jkleinsc]

• Closing in favor of fix: add patch for simdutf base64 crash #40540

[hardfalcon]

@jkleinsc: Sorry, but I think there's a misunderstanding. There are two distinct AVX-releated bugs in Electron, one in Node's simdutf dependency, and another one in Node's base64 dependency. Despite the erronous commit message of #40540, that PR only contains the fix for the AVX bug in base64, but not the fix for the AVX bug in simdutf. You can easily see this if you simply compare the actual contents/changes of #40446 and #40540.

[welcome]

Congrats on merging your first pull request! 🎉🎉🎉

[release-clerk]

No Release Notes