fix(codecs-core): resolve negative toArrayBuffer offsets against the end of the buffer - #1977
fix(codecs-core): resolve negative toArrayBuffer offsets against the end of the buffer#1977edycutjong wants to merge 1 commit into
Conversation
…end of the buffer Passing a negative offset to `toArrayBuffer` forwarded it straight to `buffer.slice()`, so the derived end index `bytesOffset + bytesLength` could cross zero. An offset of -1 with a length of 1 became `slice(-1, 0)`, which returns an empty buffer rather than the final byte. Offsets whose end index stayed negative, such as -3 with a length of 1, happened to work, which is why the bug went unnoticed. The offset is now resolved against the end of the buffer before the end index is derived, matching `Array.prototype.slice`. The existing `it.each` cases covered these offsets but only asserted that a new buffer was returned, never its contents, so they passed against the broken behaviour; the new cases assert contents and check parity with native `slice`. Fixes anza-xyz#1960.
🦋 Changeset detectedLatest commit: 77aadde The changes in this PR will be included in the next version bump. This PR includes changesets to release 48 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
BundleMonFiles updated (8)
Unchanged files (142)
Total files change -1015B -0.18% Final result: ✅ View report in BundleMon website ➡️ |
trevor-cortex
left a comment
trevor-cortex
left a comment
There was a problem hiding this comment.
(Submitting as COMMENT since I don't have approve permissions on this repo — this would be an approval.)
This fixes a real bug: toArrayBuffer forwarded negative offsets straight to buffer.slice(), so the derived end index bytesOffset + bytesLength could cross zero and yield an empty buffer (e.g. (-1, 1) → slice(-1, 0)). The offset is now normalized against the end of the buffer before the end index is derived, matching Array.prototype.slice semantics, and the old bytesOffset === -buffer.byteLength special case correctly collapses into the simpler startIndex === 0 check.
I verified the arithmetic against native slice for the covered cases, including clamping (-10 on a 5-byte buffer → start 0) and the previously-broken omitted-length path ((bytes, -2) used to produce slice(-2, 3) → empty). The new content-asserting tests close the gap left by the old identity-only assertions, and the changeset (patch bump) is appropriate for a bug fix.
Two non-blocking observations, both inline:
- Views with non-zero
byteOffset: becausebytesOffset = bytes.byteOffset + offset, a negative user offset on a subarray view can flip positive and be treated as buffer-absolute, so the slice-like semantics only strictly hold whenbytes.byteOffset === 0. Internal callers never pass negative offsets, so this is academic, but worth a doc note at some point. - Identity shortcut broadening: offsets ≤
-byteLengthwith a full-buffer length now hit the identity return (same buffer object) where the old code returned an empty copy. Correct per slice semantics, but the identity aspect isn't covered by a test.
For subsequent reviewers: the coordination note about #1970 checks out — that PR touches the SharedArrayBuffer copy branch while this one touches the offset resolution and return, so they're semantically independent but will conflict textually; whichever lands second needs a trivial rebase. Also note that in the SAB path, buffer.byteLength is the length of the copied view rather than the original buffer, so how negative offsets resolve there for offset views is tied to the #1959 discussion, not this change.
| const startIndex = | ||
| bytesOffset < 0 ? Math.max(buffer.byteLength + bytesOffset, 0) : Math.min(bytesOffset, buffer.byteLength); |
There was a problem hiding this comment.
Non-blocking: since bytesOffset = bytes.byteOffset + (offset ?? 0), a negative user offset on a view with non-zero byteOffset can end up positive and take the Math.min branch, where it's treated as an absolute index into the underlying buffer rather than being resolved against the end. So the Array.prototype.slice analogy in the comment strictly holds only when bytes.byteOffset === 0. All internal callers pass non-negative offsets, so nothing is broken — but a short JSDoc note on the offset parameter documenting that negative offsets resolve against the underlying buffer (not the view) would prevent surprises for external callers.
| [-3, 1, [3]], | ||
| [-3, 3, [3, 4, 5]], | ||
| [-5, 2, [1, 2]], | ||
| [-10, 2, [1, 2]], |
There was a problem hiding this comment.
The (-10, 2) case nicely covers clamping for contents, but there's a related behavior change that isn't locked down: with the new normalization, an offset ≤ -byteLength combined with a full-buffer length (e.g. (-10, 5) here) now hits the identity shortcut and returns the same buffer object, where the old code returned an empty copy. That's correct per slice semantics, but consider adding a case like [-10, 3] to the earlier returns the buffer without modification it.each (around line 14, outside this diff — hence anchoring here) to assert the identity return for over-shooting negative offsets.
3 participants
Problem
toArrayBufferforwards a negative offset straight tobuffer.slice(), so the derived end indexbytesOffset + bytesLengthcan cross zero. An offset of -1 with a length of 1 becomesslice(-1, 0), which returns an empty buffer rather than the final byte. Offsets whose end index stays negative, such as -3 with a length of 1, happen to return the right bytes, which is likely why this went unnoticed.The existing
it.eachblock does cover(-1, 1)and(-2, 1), but only asserts.not.toBe(byteArray.buffer)— it checks identity, never contents — so it passes against the broken behaviour.Summary of Changes
The offset is now resolved against the end of the buffer before the end index is derived, the same way
Array.prototype.slicedoes. This also subsumes the previousbytesOffset === -buffer.byteLengthspecial case, since that offset normalises to0, which is why the early-return condition gets simpler rather than gaining a branch.The new tests assert the returned contents for negative offsets and check parity with native
slice. Six of them fail onmainand pass with this change. No internal caller is affected: of the two call sites that pass an offset,packages/fixed-points/src/codecs.tsandpackages/codecs-numbers/src/utils.ts, both take it from decoder read positions, which are never negative.Verified with the full
@solana/codecs-coresuite in both Node and browser environments (137 tests each), typecheck, lint, and prettier, plus the unit suites of the four downstream packages that calltoArrayBuffer(codecs-numbers,fixed-points,codecs-strings,keys).One coordination note: #1970 is open against the same function for #1959. The changes are independent — that one touches the
SharedArrayBufferbranch, this one touches the return — but they overlap textually. Happy to rebase on top of it once it lands.Fixes #1960