Skip to content

Conversation

nickva
Copy link
Contributor

@nickva nickva commented Jun 29, 2025

Previously, in issue #418 and pr #417 I misread the spec that in subarray we should be able to access the byteOffset from a detached buffer. Thinking more about it, something didn't seem right and I started a discussion in the TC39 group [1].

It turns out we shouldn't be able to read the byteOffset from detched buffers. Instead, the spec says we should just read the byteOffset value before we access start and finish. In the test262 test [2] the buffer is detached when accessing the end inside the valueOf() conversion, and the test expects to see the byteOffset before it was detached.

So to fix it, ensure we access and save the byteOffset value first, then get the start and finish.

[1] https://es.discourse.group/t/typedarray-subarray-byteoffset-with-detached-buffers/2381

[2] https://github.com/tc39/test262/blob/main/test/built-ins/TypedArray/prototype/subarray/byteoffset-with-detached-buffer.js

@nickva nickva closed this Jun 29, 2025
@nickva nickva deleted the fix-typedarray-byteoffset branch June 29, 2025 18:45
@nickva nickva restored the fix-typedarray-byteoffset branch June 29, 2025 18:46
@nickva nickva reopened this Jun 29, 2025
Previously, in issue bellard#418 and pr bellard#417 I misread the spec that in `subarray` we
should be able to access the `byteOffset` from a detached buffer. Thinking more
about it, something didn't seem right and I started a discussion in the TC39
group [1].

It turns out we shouldn't be able to read the ``byteOffset`` from detched
buffers. Instead, the spec says we should just read the `byteOffset` value
before we access `start` and `finish`. In the test262 test [2] the buffer is
detached when accessing the `end` inside the `valueOf()` conversion, and the
test expects to see the `byteOffset` before it was detached.

So to fix it, ensure we access and save the `byteOffset` value first, then get
the `start` and `finish`.

[1]
https://es.discourse.group/t/typedarray-subarray-byteoffset-with-detached-buffers/2381

[2] https://github.com/tc39/test262/blob/main/test/built-ins/TypedArray/prototype/subarray/byteoffset-with-detached-buffer.js
@nickva nickva force-pushed the fix-typedarray-byteoffset branch from 735d16e to 1ab4972 Compare June 29, 2025 18:47
@nickva
Copy link
Contributor Author

nickva commented Jun 29, 2025

Ran in CI with test262 https://github.com/nickva/quickjs/actions/runs/15958281778/job/45007403622 on top of the "Run test262 tests in CI" PR #408

Screenshot 2025-06-29 at 2 54 42 PM

@bellard
Copy link
Owner

bellard commented Aug 25, 2025

There is still a small problem in your fix: you read ByteOffset before converting 'start' to integer as in the spec. I made another patch instead.

@nickva
Copy link
Contributor Author

nickva commented Aug 25, 2025

There is still a small problem in your fix: you read ByteOffset before converting 'start' to integer as in the spec. I made another patch instead.

You're right, I agree. I see the steps are ToIntegerOrInfinity(start) then srcByteOffset = O.[[ByteOffset]] then ToIntegerOrInfinity(end). If the buffer is detached during start parsing with the valueOf() {$DETACHBUFFER(..); return ...} trick, then ByteOffset will read a detached buffer's offset. Seems like a spec omission but your fix matches the spec. I'll close this PR then. Thank you for noticing the issue.

For reference if others see this: https://tc39.es/ecma262/#sec-%25typedarray%25.prototype.subarray

...
8. Let relativeStart be ? ToIntegerOrInfinity(start).
...
13. Let srcByteOffset be O.[[ByteOffset]].
...
16. Else,
    a. If end is undefined, let relativeEnd be srcLength; else let relativeEnd be ? ToIntegerOrInfinity(end).
...

@nickva nickva closed this Aug 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants