lib: short-circuit WebIDL BufferSource SAB check#62833
lib: short-circuit WebIDL BufferSource SAB check#62833panva wants to merge 2 commits intonodejs:mainfrom
Conversation
Avoid the C++ `isSharedArrayBuffer` binding call on the BufferSource and ArrayBufferView converter hot path by first checking whether the buffer's prototype chain contains `ArrayBuffer.prototype`. Every genuine (non-shared) ArrayBuffer is covered by the fast path; SharedArrayBuffers and prototype-tampered objects still fall through to the authoritative brand-checking binding, preserving the existing SharedArrayBuffer rejection semantics. While here, dispatch the view-kind (TypedArray vs DataView) from the primordial `%TypedArray%.prototype.@@toStringTag` getter directly, dropping the redundant `ArrayBufferIsView` check that `isDataView` would repeat at each call site. Signed-off-by: Filip Skokan <panva.ip@gmail.com>
|
Review requested:
|
panva
added
author ready
panva
removed
request-ci
| // `ArrayBuffer`; only buffers whose prototype chain has been detached or | ||
| // reassigned (and real SABs) fall through to the authoritative brand check. | ||
| function isSharedArrayBufferBacking(buffer) { | ||
| return !ObjectPrototypeIsPrototypeOf(ArrayBufferPrototype, buffer) && |
There was a problem hiding this comment.
Theoretically non-compliant for Object.setPrototypeOf(new SharedArrayBuffer(), ArrayBuffer.prototype) – would presumably be a fairly useless object, I don't know how much we need to care about that sort of thing.
There was a problem hiding this comment.
Good catch, I don't think I have a way of dealing with that. 🤷 nor do I have an opinion on caring about it. What do others think?
There was a problem hiding this comment.
This function is only ever used on buffers returned by getViewedArrayBuffer, right? Since that function uses the primordial .buffer getters which already perform brand checks on the ArrayBufferView, I don't think we can get a forged array buffer here. Unless you first did Object.setPrototypeOf(view.buffer, ArrayBuffer.prototype)? Is that allowed, and does that persist on view.buffer? 🤔
Alternatively, we could call the getter of a property on SharedArrayBuffer.prototype and check if it throws or not, see webidl2js. I don't know if that's faster than calling into C++ though.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #62833 +/- ##
==========================================
- Coverage 89.64% 89.61% -0.03%
==========================================
Files 706 706
Lines 219024 219147 +123
Branches 41960 41980 +20
==========================================
+ Hits 196338 196384 +46
- Misses 14587 14656 +69
- Partials 8099 8107 +8
🚀 New features to boost your workflow:
|
5 participants
Avoid the C++
isSharedArrayBufferbinding call on the BufferSource and ArrayBufferView converter hot path by first checking whether the buffer's prototype chain containsArrayBuffer.prototype. Every genuine (non-shared) ArrayBuffer is covered by the fast path; SharedArrayBuffers and prototype-tampered objects still fall through to the authoritative brand-checking binding, preserving the existing SharedArrayBuffer rejection semantics.While here, dispatch the view-kind (TypedArray vs DataView) from the primordial
%TypedArray%.prototype.@@toStringTaggetter directly, dropping the redundantArrayBufferIsViewcheck thatisDataViewwould repeat at each call site.