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

Is IntegerIndexedElementSet returning false for OOB access web-compatible? #2208

Closed
shvaikalesh opened this issue Oct 20, 2020 · 4 comments · Fixed by #2210
Closed

Is IntegerIndexedElementSet returning false for OOB access web-compatible? #2208

shvaikalesh opened this issue Oct 20, 2020 · 4 comments · Fixed by #2210

Comments

@shvaikalesh
Copy link
Member

shvaikalesh commented Oct 20, 2020

Please consider the following code:

"use strict";
const tA = new Int8Array([0, 1, 2]);
tA[42] = 1; // normal completion

Step 6 of IntegerIndexedElementSet returns false for out-of-bounds indices, which Set should throw a TypeError for.
However, none of the tested runtimes (V8, SM, JSC) throws.

const tA = new Int8Array([0, 1, 2]);
Reflect.set(tA, 42, 1);
// V8: true
// SM: false
// JSC: false

If throwing isn't web-compatible, should true be returned instead to maintain strict-mode assignment semantics?

Note: this issue wasn't affected by / isn't related to recent detached buffers change.

cc @rkirsling @syg

@anba
Copy link
Contributor

anba commented Oct 20, 2020

No, it's not web-compatible.

I've modified Firefox to throw on OOB access (or more precisely to throw when IsValidIntegerIndex returns false). Based on past experience, I tried https://wasmboy.app out. When trying to load a game (Choose "Open > Open Source ROMs" and click on one of the games for example "tobu tobu girl".), a "Game Load Error" message was displayed and devtools showed a "RangeError: invalid or out-of-range index" error.

Edit: https://godbolt.org/ is also affected.
Edit 2: And street view on https://maps.google.com, too. Later: Hmm, could be just sloppy-mode OOB access, in which case it's irrelevant for this issue.

@rkirsling
Copy link
Member

Thanks for the investigation, @anba! If so, then I think we need to align the spec with the foregoing behavior for V8 and SM—namely, we need to have IntegerIndexedElementSet early out with true instead of false. Even with that though, I wonder how we should deal with Table 6's suggestion that [[Set]] "returns true if the property value was set or false if it could not be set". Does a note suffice?

@rkirsling
Copy link
Member

Er, hmm. The Reflect.set case is still pretty interesting though, in that it seems like SM and JSC effectively override PutValue in its entirety for typed arrays. This makes a certain amount of sense, since it still allows us to convey that we couldn't perform the operation, but I'm not sure that there's a straightforward way to do such a thing spec-side—after all, [[Set]] does need to have a coherent interface as an EIM. 😓

@syg syg added the editor call to be discussed in the next editor call label Oct 21, 2020
@rkirsling rkirsling removed the editor call to be discussed in the next editor call label Oct 21, 2020
@anba
Copy link
Contributor

anba commented Oct 21, 2020

The SpiderMonkey behaviour for Reflect.set just tries to follow the spec as close as possible (i.e. return false for invalid indices). It was implemented in https://bugzilla.mozilla.org/show_bug.cgi?id=1308735 and the bug report doesn't indicate any web site compatibility issues, but rather just a spec violation.

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.

4 participants