Skip to content

Use correct flag to determine if V8 Sandboxing is enabled #1373

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

malshoff
Copy link

@malshoff malshoff commented May 9, 2025

Electron switched to using v8's sandboxing feature as of V21. With this enabled, allocating a buffer using node::Buffer::New crashes the program, because it attempts to allocate memory outside of the allowed memory address range of the sandbox.

[23992:0508/184214.301:ERROR:node_bindings.cc(162)] Fatal error in V8: v8_ArrayBuffer_NewBackingStore When the V8 Sandbox is enabled, ArrayBuffer backing stores must be allocated inside the sandbox address space. Please use an appropriate ArrayBuffer::Allocator to allocate these buffers, or disable the sandbox.

Reproducing the issue is as simple as calling db.serialize() in Electron versions 30.0 and higher.

PR #1036 fixed this for electron versions until 30.x.x+, where this fix broke. Why this only broke in the most recent Electron versions, I'm not sure, but there were 2 issues:

  • The flag being checked is rarely used, and also was not the proper one to use for determining if V8 sandboxing is enabled. That would be V8_ENABLE_SANDBOX . This is also the flag that Electron uses: https://github.com/electron/electron/blob/main/patches/node/support_v8_sandboxed_pointers.patch
  • The conditions were illogical: "if sandboxing is enabled, use the allocation method that should violate sandboxing rules and crash the program". This never should never have worked, and yet it did until v30.

I tested the minimum repro with these changes inside of an Electron app, and it behaves as expected now. (#1372 )

Copy link
Member

@mceachen mceachen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for doing this research!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants