Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

%TypedArray%.prototype.slice does not completely account for the original TA's size changing #3248

Closed
trflynn89 opened this issue Dec 24, 2023 · 3 comments · Fixed by #3255
Closed
Assignees

Comments

@trflynn89
Copy link

In %TypedArray%.prototype.slice, it is possible for the source TA's size to change, for example during the 13. Let A be ? TypedArraySpeciesCreate(O, « 𝔽(count) »). step. The spec partially handles this case by updating a few locals before later writing to the new TA:

14. If count > 0, then
    a. Set taRecord to MakeTypedArrayWithBufferWitnessRecord(O, seq-cst).
    b. If IsTypedArrayOutOfBounds(taRecord) is true, throw a TypeError exception.
    c. Set len to TypedArrayLength(taRecord).
    d. Set final to min(final, len).

However, these steps do not update the count value, which was previously initialized as:

12. Let count be max(final - k, 0).

This causes an issue later while reading values from the source TA:

viii. Let limit be targetByteIndex + min(count, len) × elementSize.
ix. Repeat, while targetByteIndex < limit,
    1. Let value be GetValueFromBuffer(srcBuffer, srcByteIndex, uint8, true, unordered).
    2. Perform SetValueInBuffer(targetBuffer, targetByteIndex, uint8, value, true, unordered).
    3. Set srcByteIndex to srcByteIndex + 1.
    4. Set targetByteIndex to targetByteIndex + 1.

In the call to GetValueFromBuffer, because count was not updated, it is possible to read from an index that is past-the-end of the srcBuffer.

This is observable with the following JS, which I reduced from the test/staging/ArrayBuffer/resizable/slice-species-create-resizes.js test262 test case:

const rab = new ArrayBuffer(4 * Uint8Array.BYTES_PER_ELEMENT, {
    maxByteLength: 8 * Uint8Array.BYTES_PER_ELEMENT,
});

let resizeWhenConstructorCalled = false;

class MyArray extends Uint8Array {
    constructor(...params) {
        super(...params);

        if (resizeWhenConstructorCalled) {
            rab.resize(2 * Uint8Array.BYTES_PER_ELEMENT);
        }
    }
}

const lengthTracking = new MyArray(rab);
resizeWhenConstructorCalled = true;

lengthTracking.slice(-3, -1);

In the call to slice, the size of the source TA is reduced from 4 to 2 during the 13. TypedArraySpeciesCreate step. Before that step, we have k=1, final=3, len=4, count=2. After step 14d, we have k=1, final=2, len=2. Because count is not updated, it is still count=2. This leads to reading one past-the-end of the source TA buffer.

@michaelficarra
Copy link
Member

But as you say, len is updated, and limit (used for bounding the loop) is computed from the smaller of count and len, so I don't see how srcByteIndex would go past the end of the source TA.

@trflynn89
Copy link
Author

trflynn89 commented Jan 2, 2024

In the example, we read past the end of the source TA because the starting offset (k) is 1. Since len becomes 2, we only have 1 slot left to read from. So limit should be 1.

But because count wasn't updated, when we set limit, we have limit = min(count, len) = min(2, 2), thus we think we have 2 slots available to read from.

@michaelficarra michaelficarra added the editor call to be discussed in the next editor call label Jan 3, 2024
@syg
Copy link
Contributor

syg commented Jan 11, 2024

Great catch, @trflynn89. Will have a PR up shortly.

syg added a commit to syg/ecma262 that referenced this issue Jan 11, 2024
Closes tc39#3248.

The current algorithm has a bug that can result in OOB reads in the
source TA, because _count_ is not correctly recomputed when the source
TA is resized during evaluation of the species constructor.

(It is currently bounded by _len_, which is recomputed, but this is
incorrect because the bounds of the copy loop is not on the length, but
instead on how many bytes need to be copied.)
@michaelficarra michaelficarra removed the editor call to be discussed in the next editor call label Jan 11, 2024
@ljharb ljharb closed this as completed in 22de374 Jan 11, 2024
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 a pull request may close this issue.

3 participants