Skip to content

Conversation

@joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Nov 20, 2025

Idea was suggested by @deepak1556 IIRC - instead of relying on some flag to decide whether we should install the WASM trap handler, just detect how much virtual memory is accessible and if there's not enough, don't use trap-handler-based bound checks which requires a 8-16GB virtual memory cage currently to allocate any wasm memory.

It's overall less brittle if it's mostly implemented in the upstream because the cage size might change and there might potentially be other requirements (kernel versions or hardware capabilities?) that V8 would know the best about. Going to see if this can be upstreamed or do we have to keep it on the embedder's side.

Upstream CL: https://chromium-review.googlesource.com/c/v8/v8/+/7182060

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/security-wg
  • @nodejs/v8-update

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. v8 engine Issues and PRs related to the V8 dependency. labels Nov 20, 2025
@joyeecheung joyeecheung marked this pull request as draft November 20, 2025 06:58
@joyeecheung joyeecheung changed the title [WIP] only install WASM trap handler installation when there's enough virtual memory [WIP] only install WASM trap handler when there's enough virtual memory Nov 20, 2025
@codecov
Copy link

codecov bot commented Nov 20, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.53%. Comparing base (0b6ae6d) to head (efcca70).
⚠️ Report is 11 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #60788      +/-   ##
==========================================
+ Coverage   88.04%   88.53%   +0.49%     
==========================================
  Files         703      703              
  Lines      208260   208260              
  Branches    40068    40154      +86     
==========================================
+ Hits       183360   184390    +1030     
+ Misses      16840    15885     -955     
+ Partials     8060     7985      -75     

see 105 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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

Labels

needs-ci PRs that need a full CI run. v8 engine Issues and PRs related to the V8 dependency.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants