Keep scanning in indexOf() when a resizable buffer shrank - #1644
Open
andreasrosdal wants to merge 4 commits into
Open
Keep scanning in indexOf() when a resizable buffer shrank#1644andreasrosdal wants to merge 4 commits into
andreasrosdal wants to merge 4 commits into
Conversation
When the fromIndex argument shrinks the underlying resizable ArrayBuffer
from its valueOf(), %TypedArray%.prototype.indexOf() gives up and returns
-1 even though the element it is looking for is still inside the surviving
prefix:
const rab = new ArrayBuffer(8, {maxByteLength: 8});
const ta = new Int8Array(rab);
for (let i = 0; i < 8; i++) ta[i] = i;
ta.indexOf(2, {valueOf() { rab.resize(4); return 0; }}); // -1, want 2
lastIndexOf() already survives this: it falls through to the clamped scan
below. indexOf() and includes() do not, because the early-out branch is
entered for every `special` whenever len exceeds the current element count.
Restrict that branch to includes(), which genuinely cannot use the clamped
scan (it reports "undefined" for the indices that vanished, so it has to
reason about the original length). indexOf() then reaches the same clamped
scan as lastIndexOf(): len, k and stop are pinned to the new length, so the
scan stays in bounds and finds the elements that are still there.
Detached buffers are unaffected: typed_array_is_oob() still takes every
`special` down the early-out path.
Not addressed here: includes() still misses a value that survived the
shrink (`ta.includes(2, evil)` is false above, V8 says true). Fixing that
means scanning the clamped prefix *and* reporting a match for undefined
against the truncated tail, which is a larger change than this one.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014mv33YvfHz7t9mmkituBnn
Contributor
|
This needs a test. |
Covers the scan that %TypedArray%.prototype.indexOf() now performs when the fromIndex coercion shrinks the underlying resizable ArrayBuffer: elements that survive in the clamped prefix are found, the ones that vanished with the truncated tail are not, and fromIndex keeps resolving against the length read before the coercion. Every element type gets a case because each scan path is written out separately (uint8 goes through memchr), plus a length tracking view at an offset, a shrink to zero, a fixed length view that goes out of bounds, and a detached buffer -- the last two still return -1 without raising. A growing buffer is checked too: the scan must not run past the length that was read before the coercion. The includes() cases pin down the branch the fix leaves in place, where the indices the shrink removed still read as undefined. Without the fix the first case already fails with -1 instead of 2. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kgm5WqGGFTKvG6vEsbUbvc
The changed bail-out is shared by lastIndexOf() and includes(), so pin what each of the three does with the same shrink: lastIndexOf scans downward from a start clamped into the surviving prefix, including for a negative fromIndex resolved against the original length. Also covers the two things that are easy to lose in a bail-out path: indexOf compares with strict equality while includes uses SameValueZero, which only shows up for NaN and -0; and a fromIndex coercion that throws propagates, unlike a resize, while a view that is already out of bounds on entry still throws for all three. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K6eRbuuuCujKgQkrHgvMrc
Now that only "includes" bails out of a merely shrunk buffer, everything that reaches the tail of that block has typed_array_is_oob(p) true, so the lastIndexOf test and its comment describe a path that can no longer be taken. Fold the block into the single goto it always performs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When the
fromIndexargument shrinks the underlying resizable ArrayBuffer from itsvalueOf(),%TypedArray%.prototype.indexOf()gives up and returns-1even though the element it is looking for is still inside the surviving prefix:lastIndexOf()already survives this: it falls through to the clamped scan below.indexOf()andincludes()do not, because the early-out branch is entered for everyspecialwheneverlenexceeds the current element count.Restrict that branch to
includes(), which genuinely cannot use the clamped scan (it reportsundefinedfor the indices that vanished, so it has to reason about the original length).indexOf()then reaches the same clamped scan aslastIndexOf():len,kandstopare pinned to the new length, so the scan stays in bounds and finds the elements that are still there.Detached buffers are unaffected:
typed_array_is_oob()still takes everyspecialdown the early-out path.Not addressed here:
includes()still misses a value that survived the shrink (ta.includes(2, evil)isfalseabove, V8 saystrue). Fixing that means scanning the clamped prefix and reporting a match forundefinedagainst the truncated tail, which is a larger change than this one.🤖 Generated with Claude Code
https://claude.ai/code/session_014mv33YvfHz7t9mmkituBnn
Generated by Claude Code