Skip to content

lib: fix shared buffer growability validation - #65845

Open
panva wants to merge 1 commit into
nodejs:mainfrom
panva:webidl-shared-buffer-growability
Open

lib: fix shared buffer growability validation#65845
panva wants to merge 1 commit into
nodejs:mainfrom
panva:webidl-shared-buffer-growability

Conversation

@panva

@panva panva commented Sep 6, 2026

Copy link
Copy Markdown
Member

Use the intrinsic growable getter instead of buffer.growable so shadowed properties cannot bypass validation or reject fixed buffers.

cc @nodejs/web-standards

Use the intrinsic growable getter instead of buffer.growable so
shadowed properties cannot bypass validation or reject fixed buffers.

Signed-off-by: Filip Skokan <panva.ip@gmail.com>
Assisted-by: GitHub Copilot
@panva panva added the web-standards Issues and PRs related to web-platform APIs and standards compliance. label Sep 6, 2026
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. typings Issues and PRs related to internal TypeScript declarations. util Issues and PRs related to the built-in util module. labels Sep 6, 2026
Comment thread src/node_util.cc
Comment on lines +500 to +517
{
const Local<Object> prototype =
SharedArrayBuffer::New(isolate, 0)->GetPrototypeV2().As<Object>();
const Local<Object> descriptor =
prototype
->GetOwnPropertyDescriptor(
context, FIXED_ONE_BYTE_STRING(isolate, "growable"))
.ToLocalChecked()
.As<Object>();
const Local<Value> getter =
descriptor->Get(context, env->get_string()).ToLocalChecked();
CHECK(getter->IsFunction());
target
->Set(context,
FIXED_ONE_BYTE_STRING(isolate, "getSharedArrayBufferGrowable"),
getter)
.Check();
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is storing this and invoking with FunctionPrototypeCall definitely quicker than a fast binding that returns sab->GetBackingStore()->IsResizableByUserJavaScript()?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I haven't benchmarked against a fast binding. Copilot tried that predicate first, but it wasn't equivalent. Shared Wasm memory can expose fixed and growable SAB wrappers over the same backing store.

// For shared Wasm memories, this field never changes, but may differ from the
// value of the is_resizable_by_js field of SharedArrayBuffers it backs.
// WebAssembly.Memory can create multiple SharedArrayBuffers backed by the
// same BackingStore, some of which are exposed as growable, and some of which
// as fixed-length.

The getter checks the individual buffer's growability, which is what Web IDL requires.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice catch.

The flag check on the SAB itself would be sab.As<ArrayBuffer>()->IsResizableByUserJavaScript() – this checks the flag on the AB handle directly (https://github.com/v8/v8/blob/9f30cc5fc4ad6c3236c522fd23224ddc94591a20/src/api/api.cc#L4374-L4376). SAB handles are AB handles, so providing sab is known to be a SharedArrayBuffer, this should be directly equivalent to sab.growable.

Maybe worth a mini benchmark?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Copilot ran a mini benchmark, about 10-24% faster for this check. But sab.As<ArrayBuffer>() fails with V8_ENABLE_CHECKS because the cast rejects shared buffers.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That's interesting, we already use this relationship elsewhere... although I think technically we cast v8::Value handles that are known to be SABs, rather than casting a v8::SharedArrayBuffer handle directly.

@codecov

codecov Bot commented Sep 6, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.11765% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 90.18%. Comparing base (6f41e41) to head (4e6aadf).
⚠️ Report is 26 commits behind head on main.

Files with missing lines Patch % Lines
src/node_util.cc 92.30% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #65845      +/-   ##
==========================================
+ Coverage   90.17%   90.18%   +0.01%     
==========================================
  Files         770      771       +1     
  Lines      264483   264633     +150     
  Branches    50262    50227      -35     
==========================================
+ Hits       238491   238671     +180     
+ Misses      16981    16958      -23     
+ Partials     9011     9004       -7     
Files with missing lines Coverage Δ
lib/internal/webidl.js 100.00% <100.00%> (ø)
src/node_util.cc 81.79% <92.30%> (+0.39%) ⬆️

... and 35 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.

@panva panva added author ready PRs with CI started, the required approvals, and no outstanding review comments. request-ci Add this label to start a Jenkins CI on a PR. labels Sep 6, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 6, 2026
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

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

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. typings Issues and PRs related to internal TypeScript declarations. util Issues and PRs related to the built-in util module. web-standards Issues and PRs related to web-platform APIs and standards compliance.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants