-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: implement v8 sandbox on ppc64 for electron 26 #265
base: master
Are you sure you want to change the base?
Conversation
Could Raptor fix this issue as well? |
Is this ppc64 specific? Doesn't sound like so. |
It never sagfaulted on amd64 on me, but I'll check it again with the latest electron.
Backtrace would be helpful. |
No issues with
|
Thanks for confirming this! I will check if removing the v8 sandbox patch fixes this and eventually if applying it to electron's node's v8 (in addition to electron's v8) helps. If nothing helps I will get a backtrace. |
Hmm, it gets even more interesting. GitHub workflow for 1.83.1 failed with
I restarted it and it passed successfully from the second time. It doesn't fail on real hardware though… Try restarting the build with +temp-fix, maybe it was a one-time incident? It could be indeed an issue of node or yarn. Sometimes it errs with:
|
Unfortunately not, I've tried many times and without the patch it compiles flawlessly but as soon as I add the v8 sandbox patch if segfaults. How am I supposed to attach a debugger? At this point I'm curious to know if adding the patch to |
@darkbasic Frankly, the v8 version in Electron 26 is quite buggy even outside of the sandbox patches. Patching it to even compile in the first place (which requires commit
I'm not sure how useful it will be to try to hack this exact v8 version into operation. The fact that you are getting segmentation faults (and I see SIGSEGV/SIGILL on release checks) even without my patches with this v8 version indicates something is seriously wrong in the base code itself. I note that none of these problems show up on the current master version of v8, so something between v8 commits Is there any scope to simply updating v8 to a newer version for the ppc64 variant? If not, waiting for Electron 27 might be a better idea at this point. Raptor won't be able to spend significant resources trying to get an obsolete v8 version to work when the current version is operational, unfortunately. |
I'm all in to use the least amount of work to get this working so I am fine to give up on electron 26 and try electron 27. Yet I am a bit dubious that this would solve the issue, because node version barely changes between electron versions. 27 uses 18.17.1 instead of 18.16.1, so unless they updated deps/v8 that won't do us any good. They won't move out of LTS versions so it will take years to get rid of 18.x.x.
Are you suggesting to simply drop |
Which v8 version is actually being executed here, the one in the embedded Chromium tree or the one in nodejs Given the aging issues mentioned above we may have no choice but to try to fix this on the older versions. |
@darkbasic The nodejs version here is so ancient that it doesn't even have arch-specific sandbox code in its v8 tree. I'm not confident in my understanding of which components are being used to actually drive the Electron build process, if I could get clarification on that it would be most helpful. Is there any chance I could get a set of commands required to reproduce the build (and build crash) outside of Gentoo-specific build tooling? Given the complexity of setting up a proper build and debug environment for these components I really need to be able to reproduce from within my Debian v8 development system. Thanks! |
Electron uses its own bundled v8 (electron/v8) and patching electron/v8 alone is enough to get vscode working with nodejs 20 (the installation phase doesn't work with node 20, but that's another issue). Yet electron also bundles its own node.js (v18.16.1 for electron 26) which in turn bundles its own v8 (nodejs/deps/v8), which is a completely different version. I didn't manage to patch that one because of conflicts and I'm not even sure if it will help doing so (node.js 20 has no v8 sandbox patches yet it doesn't segfault).
This is the list of commands required to compile electron 26: https://github.com/PF4Public/gentoo-overlay/blob/master/dev-util/electron/electron-26.4.0.ebuild And this is the list of commands required to compile vscode git master (you will need to follow the electron-26 and temp-fix code paths for that): https://github.com/PF4Public/gentoo-overlay/blob/master/app-editors/vscode/vscode-9999.ebuild Gentoo ebuilds are akin to bash scripts so it won't be too hard to reproduce the steps on any distro, yet compiling electron and especially vscode is full of quirks and generally not an easy task so I'm not sure if that would be the easiest path. |
@darkbasic The more I look at the trees, the more I suspect there is a feature flags conflict between the integrated node build and the Chromium v8 build. From what I can see, the Chromium v8 engine is what is actually running (good), but node itself is built with pointer compression, external code space, and the sandbox all disabled (bad). Before trying to get me some kind of command sequience to replicate this entire mess on my system, can you try aligning the feature flags in node's v8 Bear in mind that on x86 the |
Wouldn't that require patching node's v8? Otherwise I see no way to enable features which are not even implemented on ppc. |
The v8 version in nodejs is so old that it doesn't have any arch-specific code for any of those features except pointer sandboxing (which should be off in both the Chromium v8 and nodejs v8) and pointer compression:
Pointer compression already has the ppc64 arch-specific code in tree in that nodejs version. Please try turning the features on, and if you hit a build failure let me know. |
Weird, I've built vscode git master 4 consecutive times with electron 26.4.0 without the v8 sandbox patches.
Sure, I'll give it a try. |
@madscientist159 can you please confirm if the full v8 sandbox patchset made its way into electron 29? Somehow I'm still getting segfaults but maybe it's unrelated: #317 (comment) |
With
+temp-fix
it fails at compilation time:With
-temp-fix
it fails at installation time:Using system node at compilation time and electron's node during the installation results in a working build.
The shipped ebuild is meant to be used with
-temp-fix
and force-enables system electron at the installation phase.Please check if vscode-9999 with electron 26 behaves the same way on amd64.