Missing coverage: calling Array.prototype funcs with a TypedArray with a resizable / growable backing

[marjakh]

Example:

__v_1 = new Uint8Array();
Object.defineProperty(__v_1.__proto__, 'length', {value: 42});
Array.prototype.includes.call(new Uint8Array(), 2);

-> if we pass in a TA with resizable backing, weird things might happen. Especially, there are two different kinds of lengths going around, one from the length getter and one from the backing store, and the latter might change in surprising places.

Getting this right is also essential for security, so would be great if test262 had tests for this.

cc @jugglinmike

[jugglinmike]

Thanks for the support!

In your example, is it important to retrieve a reference to the TypedArrayPrototype through a Typed Array instance? Would it also be acceptable to write, e.g. Uint8Array.prototype?

Object.defineProperty(Uint8Array.prototype, 'length', {value: 42});

Or would that constitute a different test?

I may be getting too hung up with the details in your example, though. Since I'm not familiar with V8's internals, I might be able to understand the intent better if we reframe the discussion. Instead of talking in terms of what could go wrong, could we focus on what the specification states must happen? That has benefits beyond coordinating test writing: it will help determine how to organize/document the tests, and it will keep the scope tractable.

(By the way: GitHub.com interprets all discussion text as Markdown, and that can sometimes interfere with the rendering of code examples. Surrounding code in triple-backtick characters (e.g. ```) will ensure that references like __proto__ aren't inappropriately displayed as "proto". @rwaldron kindly added them for you above.)

[marjakh]

Oops, thanks for fixing the formatting!

In this particular example, __v_1.__proto__ is the same object as Uint8Array.prototype, so they're interchangable. Once we have a reference to the object and call Object.defineProperty on it, I don't think it's observable how we got that reference (unless, getters or something, which this example doesn't have). Maybe I don't fully understand what you had in mind.

Re: what should happen: In this example, the spec for Array.prototype.includes uses the length from the length getter to determine how many items to process, and then for each item, it calls Get. Get returns undefined if the index is out of bounds. So that's relatively straightforward.

However, implementations might have all kinds of fast paths, and e.g., and an implementation might have the following logic: "If the receiver is a TypedArray, check its backing ArrayBuffer's length against the length from the getter and if the backing ArrayBuffer is big enough, loop over N elements and access raw memory." That would now be a security bug now that the backing ArrayBuffer's length can change (potentially during the iteration, for functions like every, some, filter etc).

It's basically the same thing that happens for TypedArray.prototype functions. The purpose of this report was to point out we also need tests for Array.prototype functions where we pass a resizable TypedArray as the receiver.

[jugglinmike]

Thanks for the explanation!

In this particular example, __v_1.__proto__ is the same object as Uint8Array.prototype, so they're interchangable. Once we have a reference to the object and call Object.defineProperty on it, I don't think it's observable how we got that reference (unless, getters or something, which this example doesn't have). Maybe I don't fully understand what you had in mind.

Got it--that all matches my understanding. (When I write tests, I try to avoid creating values that aren't strictly necessary, mostly to better highlight the behavior under test. By the same token, it's easy to overlook subtleties in the more particular tests, so I don't attempt simplification lightly.)

Re: what should happen: In this example, the spec for Array.prototype.includes uses the length from the length getter to determine how many items to process, and then for each item, it calls Get. Get returns undefined if the index is out of bounds. So that's relatively straightforward.

However, implementations might have all kinds of fast paths, and e.g., and an implementation might have the following logic: "If the receiver is a TypedArray, check its backing ArrayBuffer's length against the length from the getter and if the backing ArrayBuffer is big enough, loop over N elements and access raw memory." That would now be a security bug now that the backing ArrayBuffer's length can change (potentially during the iteration, for functions like every, some, filter etc).

It's basically the same thing that happens for TypedArray.prototype functions. The purpose of this report was to point out we also need tests for Array.prototype functions where we pass a resizable TypedArray as the receiver.

I think I understand halfway :) Test262 doesn't currently have any tests for resizing an ArrayBuffer during traversal with the iterative Array methods like every (the same goes for the corresponding iterative TypeArray methods, actually). I can appreciate all the ways this can go wrong, and those hazards are probably why Test262 does have tests for resizing Arrays under such circumstances. I'll write up some new tests.

However, the relationship between the length getter and the ArrayBuffer's length is not so clear to me. One the one hand, it sounds like the internal optimization concerns cases where the length property is inside the boundary created by the ArrayBuffer length and the TypedArray's BYTES_PER_ELEMENT. On the other hand, the example code is setting the length property to a value that falls ouside that boundary. Are those two different cases of interest? And are either related to resizing during iteration?

[marjakh]

Hi, thanks for looking into this.

Clarification about the length getter and the ArrayBuffer's length. I think the interesting things to test are: 1) the implementation uses the correct one, the length getter, when deciding e.g., the size of the array to return or how many times to call the callback (for funcs which take a callback, like every etc) 2) correctly handles the case when the ArrayBuffer's length is less than what the length getter returned 3) correctly handles the case when the ArrayBuffer's length is greater than what the length getter returned (although, this is less likely to go wrong) and 4) doesn't get confused if the ArrayBuffer's size changes mid-operation (I guess this case can further be sub-divided into shrinking and growing).

So yea the amount of different cases to test suffers from slight combinatorial explosion but this is how this feature is :)

[jugglinmike]

Thanks again for your help. I've submitted some tests via gh-3299.