Skip to content
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

Support for V8 sandbox in the extension host #202385

Open
deepak1556 opened this issue Jan 12, 2024 · 2 comments
Open

Support for V8 sandbox in the extension host #202385

deepak1556 opened this issue Jan 12, 2024 · 2 comments
Assignees
Labels
chromium Issues and items related to Chromium plan-item VS Code - planned item for upcoming upstream Issue identified as 'upstream' component related (exists outside of VS Code)
Milestone

Comments

@deepak1556
Copy link
Collaborator

deepak1556 commented Jan 12, 2024

Issue is starting point to reduce the patch set added for #177338 in our internal builds and a way to get electron/electron#37582 into OSS Electron.

  1. Patch at chromium layer is about a way to initialize the configurable pool for the utility process. At the moment, the configurable pool is only meant to be used by V8 array buffer allocations. First step would be to create a discussion with upstream to see if there is interest in exposing the pool for embedder use case.

  2. Second patch at chromium layer disables BRP feature for utility process since the configurable pool does not work with the feature. This can be also be solved as part of 1)

Once the above points are resolved we should be able to move the PR in Electron forward no patch overhead.

@deepak1556 deepak1556 added upstream Issue identified as 'upstream' component related (exists outside of VS Code) chromium Issues and items related to Chromium labels Jan 12, 2024
@deepak1556 deepak1556 added this to the December / January 2024 milestone Jan 12, 2024
@deepak1556 deepak1556 self-assigned this Jan 12, 2024
@deepak1556 deepak1556 modified the milestones: December / January 2024, February 2024 Jan 23, 2024
@deepak1556
Copy link
Collaborator Author

Upstream issue for discussion https://bugs.chromium.org/p/v8/issues/detail?id=14585

@deepak1556 deepak1556 modified the milestones: February 2024, March 2024 Feb 22, 2024
@deepak1556 deepak1556 modified the milestones: March 2024, April 2024 Mar 22, 2024
@deepak1556 deepak1556 modified the milestones: April 2024, May 2024 Apr 22, 2024
@deepak1556 deepak1556 modified the milestones: June 2024, July 2024 Jun 24, 2024
@deepak1556 deepak1556 modified the milestones: July 2024, August 2024 Jul 23, 2024
@joaomoreno joaomoreno added the plan-item VS Code - planned item for upcoming label Aug 27, 2024
@deepak1556 deepak1556 changed the title Upstream forceallocationstov8sandbox feature of utility process Support for V8 sandbox in the extension host Sep 23, 2024
@deepak1556
Copy link
Collaborator Author

deepak1556 commented Sep 23, 2024

Please read up on the Background section first. For additional context on the V8 sandbox you can refer to the official blog post.

Since Electron v22 we have been floating the patch described in the Solution section with our custom build. However, this is not preferred for the long term for following reasons,

  1. Patch maintenance, partition allocator is a critical component of the chromium project that moves constantly. As behavior changes to the configurable pool can break this patch anytime.
  2. Partition allocator features that don't work with this patch had to be disabled. At the moment, BRP is disabled.
  3. Memory limitations, by sharing the configurable pool between array buffer allocations and all other heap allocations from the extension host process we are increasing the chances of OOM from the allocator.

As a step forward, we approached the V8 team to see if a feature to support array buffers with backing stores outside the V8 sandbox could be possible https://issues.chromium.org/issues/42204529. Thanks to the V8 team for being receptive and proposing ideas that we could prototype. A quick summary of the attempted approach,

  1. Introducing a new external table that would store the buffer pointer and its size (to guarantee spatial memory safety)
  2. Convert sandbox_ptr_t and sandbox_size_t references to the backing stores into ExternalBufferHandle which is a 32-bit index into the (buffer, size) tuple entry of the ExternalBufferTable

The idea was to have this feature behind a compile time flag in upstream V8 or maybe even a patch in OSS Electron if the changes were minimal. But as seen in the prototype it is hard to do either. Moreover, once V8 sandbox starts using hardware support, we cannot have external tables with pointer entries that could write outside the sandbox.

Based on these learnings, we will adopt the following steps towards removing the allocator patch.

  • Get telemetry for extensions and addons that cannot be used with V8 sandbox
    • Native addons that are built today for Electron runtime have proper feedback at compile time and runtime from N-API, node-addon-api and NAN when attempt to use the incompatible v8::ArrayBuffer::NewBackingStore api.
    • It is only older addons/extensions that are not prepared to act on this feedback, the aim of this telemetry is to capture this list.
    • Electron changes feat: add error event for utility process electron/electron#43774
      • On the utility process service, override the default FatalErrorHandler which will create a Node.js diagnostic report
      • Signal the main process with this report over a new interface
      • Abort the process
  • Adopt changes the telemetry changes in release 1.94
  • Remove the allocator patch in release 1.94
  • Generate list of impacted extensions (including version range) and native addons
  • Follow-up with native addons to use v8::ArrayBuffer::Allocator or use a copy approach for the affected array buffer scenarios.
  • Follow-up with impacted extensions and see if they need adoption of newer dependency version that contains the fix
  • Maybe we could auto-update extensions with the fixed versions

Note: Special thanks to Samuel Groß from the V8 team for being responsive on the CLs and guiding me through the prototype.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chromium Issues and items related to Chromium plan-item VS Code - planned item for upcoming upstream Issue identified as 'upstream' component related (exists outside of VS Code)
Projects
None yet
Development

No branches or pull requests

2 participants