1079853 - Throw on indexing into detached ArrayBuffers instead of returning `undefined`

Status: RESOLVED INVALID

(Core :: JavaScript Engine, defect)

Description

[Till Schneidereit [:till]]

Section 9.4.5.8, step 5 now specifies that indexing should throw instead of failing silently on detached buffers.

I have my doubts regarding the web compatibility of this change. Ideally we should get telemetry data on this first.

Comment 1

[Tom S. (please needinfo tschuster)]

Edge actually throws for at least [[Get]], [[Set]] and [[DefineProperty] on detached arrays. So it seems like this is something we could actually implement.

Comment 2

[Tom S. (please needinfo tschuster)]

Attachment: patch

So, this was basically my first idea, throwing for typed arrays lookups in LookupOwnPropertyInline. I think this would basically work, but is so far from how the spec works, so I am not sure if this will always be correct. I feel like it would be better to do this more explicitly. André what do you think?

In IonMonkey LoadTypedArrayElementHole and StoreTypedArrayElementHole need code on the OOB to fail when the array is detached.

In CacheIR at least StoreTypedElement, LoadTypedElementExistsResult.

Comment 3

[Tom S. (please needinfo tschuster)]

This patch would violate the execution order defined in the spec, so we need to instead manually patch all the different operations.

Comment 4

[Tom S. (please needinfo tschuster)]

Attachment: Patch v2

This is a new approach. I added special handling for typed arrays to Get, Set, Has, GetOwnPropertyDescriptor and disallowed typed array indexes in LookupOwnPropertyInline.

While this handles probably handles like 90% of the code, there is _a lot_ of code that ends up calling LookupOwnPropertyInline. For example direct callers of  NativeLookupOwnProperty, like HasOwnProperty.

I am really not sure what the best way forward is here.

Comment 5

[Tom S. (please needinfo tschuster)]

Comment on attachment 8997606 [details] [diff] [review]
Patch v2

Afte

Comment 6

[Tom S. (please needinfo tschuster)]

After sleeping on this for a bit I realized we should actually be okay with throwing for detached buffers in LookupOwnProperty descriptor! Only [[Set]] and [[DefineOwnProperty]] don't check for IsDetachedBuffer in their first observable step. So in theory it should be ok to keep the custom implementation for those two and remove all the other ones and just start throwing.

Comment 7

[Tom S. (please needinfo tschuster)]

I think I found another interesting difference between our implementation and the spec: Because we set [[ArrayLength]] to zero, stuff like Reflect.ownKeys() will return an empty array, but from my understanding the Spec wants those to still be returned.

Comment 8

[André Bargull [:anba]]

Correct.

There are actually bad people who know and abuse this difference (bug 1381474, bug 1381469). :-) *goes away whistling*

Comment 9

[Tom S. (please needinfo tschuster)]

Realistically I am not going to fix this soon, especially the zero length makes me extremely anxious about introducing security issues.

Comment 10

[Anne (:annevk)]

Note that fixing this also requires updating all IDL APIs that take a buffer or a view. Currently a detached buffer means that there's nothing to read (empty sequence) or write (no place), but with this those places would need to start throwing. WebSocket's send(), fetch(), XMLHttpRequest, WebGL, TextEncoder's decodeInto(), Web Audio, etc.

Talking with Henri it's pretty clear most of our code is not robust against this and doesn't see the need to throw when passed a view or buffer (no [Throws] annotation in IDL and no relevant code to handle this issue).

Comment 11

[Tom S. (please needinfo tschuster)]

Seems like the spec was changed to allow this and bug 1672862 implemented most of the required related changes.