1232265 - ArrayBuffer constructor should throw for non-integer byte sizes

Status: RESOLVED DUPLICATE of bug 1255128

(Core :: JavaScript: Standard Library, defect)

Description

[André Bargull [:anba]]

The ArrayBuffer constructor should validate the input parameter to catch misuses like [1]. 

ES2015, 24.1.2.1 ArrayBuffer( length ), steps 2-4 [2].

[1] http://hg.mozilla.org/mozilla-central/file/tip/js/src/tests/ecma_6/DataView/detach-after-construction.js#l4
[2] http://ecma-international.org/ecma-262/6.0/#sec-arraybuffer-length

Comment 1

[Greg Weng [:snowmantw][:gweng][:λ]]

I'm patching this, but from the spec I don't know if I need to throw the error inside the virtual |ToNumber| (which now is actually |ToInt32| in the |class_constructor|), or I can just check the value before send it to the converter. Because the spec only describes the |length| need to be converted by |ToNumber| first, and mention nothing about the check. However, since I don't see things like the |ToNumber| in the constructor, to change the implementation just for an error message looks not a good idea.

Another concern is that I don't know will it break lots of tests and websites use the API in the wrong way (although they shouldn't). Beside that, I noticed that Node.js also allow the misuse without throwing.

Comment 2

[Greg Weng [:snowmantw][:gweng][:λ]]

The patch passed try tests:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=325412d42276

I don't know if it's good enough, but I need a reviewer to address the possible issues first.

Comment 3

[Greg Weng [:snowmantw][:gweng][:λ]]

Attachment: Patch ver.1

Comment 4

[Greg Weng [:snowmantw][:gweng][:λ]]

Oh I need to rebase first.

Comment 5

[Greg Weng [:snowmantw][:gweng][:λ]]

Attachment: Patch ver.1

Comment 6

[Greg Weng [:snowmantw][:gweng][:λ]]

Well, I don't know who should be the reviewer.

Comment 7

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #6)
> Well, I don't know who should be the reviewer.

If you are not sure, set needinfo? from the module owners of SpiderMonkey/JIT, they will know who should take this on.

https://wiki.mozilla.org/Modules/All#JavaScript

Jason, Jan, not sure who should review this, so setting needinfo? as a start.

If you would like to request fuzzing later, feel free to set feedback? from me or :decoder on your rollup patch (if multiple patches), listing the m-c rev it applies on.

Comment 8

[Greg Weng [:snowmantw][:gweng][:λ]]

Thanks for help!

Comment 9

[Jan de Mooij [:jandem]]

Jason or Waldo are probably good reviewers for this.

Greg: a few things I noticed glancing at your patch:

* toInt32 is not a great test when testing whether a Value is int32, as integer values can also be represented as doubles (JS doesn't have an int32 type, it's just an internal optimization).

* We should follow the steps in the spec:

  http://ecma-international.org/ecma-262/6.0/#sec-arraybuffer-length

So first do ToNumber, then do ToLength, then do the SameValueZero thing :)

Comment 10

[Jan de Mooij [:jandem]]

In particular, I think the following tests will throw with your patch, but should still pass:

assertEq(new ArrayBuffer({valueOf: () => 123}).byteLength, 123);
assertEq(new ArrayBuffer(true).byteLength, 1);

Comment 11

[Greg Weng [:snowmantw][:gweng][:λ]]

(In reply to Jan de Mooij [:jandem] from comment #9)
> Jason or Waldo are probably good reviewers for this.
> 
> Greg: a few things I noticed glancing at your patch:
> 
> * toInt32 is not a great test when testing whether a Value is int32, as
> integer values can also be represented as doubles (JS doesn't have an int32
> type, it's just an internal optimization).
> 
> * We should follow the steps in the spec:
> 
>   http://ecma-international.org/ecma-262/6.0/#sec-arraybuffer-length
> 
> So first do ToNumber, then do ToLength, then do the SameValueZero thing :)

Okay I will try to implement that, although ToLength is already an issue at bug 924058 and bug 1064000, so I may try to resolve them first.

Comment 12

[Greg Weng [:snowmantw][:gweng][:λ]]

Or maybe I can use the existing API in this bug first, and take a look at those related bugs if I encountered any further issue.

Comment 13

[Greg Weng [:snowmantw][:gweng][:λ]]

Okay. I updated my patch and it works well except one issue: in the spec, the step of |ToNumber| actually cannot prevent user give an empty Array or one single-element Array, like:

    var ab0 = new ArrayBuffer([])
    var ab90 = new ArrayBuffer([90])
    var ab0x10 = new ArrayBuffer([0x10])
    var abs90 = new ArrayBuffer(["90"])
    var abs0x10 = new ArrayBuffer(["0x10"])

This is because |ToNumber| will pick up the only element and turn it as a Number, which seems legit for the |ToNumber| API and I've tested with a customed JS_FN binding function. As a result, for this bug we indeed need to rewrite tests like

    var abIncorrect = new ArrayBuffer([1,2])

But we may need to guarantee the code like

    var abCorrect   = new ArrayBuffer(["2"])

works, if to follow the spec is the most critical requirement.

Comment 14

[Jan de Mooij [:jandem]]

(In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #13)
> But we may need to guarantee the code like
> 
>     var abCorrect   = new ArrayBuffer(["2"])
>
> works,

Yup, good find. We should add tests for these cases.

> if to follow the spec is the most critical requirement.

It is :)

Comment 15

[Greg Weng [:snowmantw][:gweng][:λ]]

Attachment: Patch ver.2

Uploaded the new patch and set the review. I'm not sure if I need to check the overflow case: if there is an overflow, the |SameValue(number_length, byte_length)| won't be the same. So to check it looks redundant.

Comment 16

[Jason Orendorff [:jorendorff]]

Comment on attachment 8745181 [details] [diff] [review]
Patch ver.2

Review of attachment 8745181 [details] [diff] [review]:
-----------------------------------------------------------------

This conflicts with my patch in bug 1255128; they're pretty much the same bug, except I'm also capping ArrayBuffer allocations at 1GB.

I'm going to try merging the test changes here with my code changes and see how it goes...

Comment 17

[Jason Orendorff [:jorendorff]]

Comment on attachment 8745181 [details] [diff] [review]
Patch ver.2

Review of attachment 8745181 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing r? in favor of the patch in bug 1255128.

Note that there was also a web-platform test that expected the old behavior that I had to change.

Comment 18

[Greg Weng [:snowmantw][:gweng][:λ]]

Okay I see. Does this means you will implement the |ToNumber| and |SameValue| parts in the spec for ArrayBuffer, or would you leave that so this bug is still valid and I need to patch those steps? If you will implement that, maybe this bug should be closed as a duplicated?

Comment 19

[Greg Weng [:snowmantw][:gweng][:λ]]

Update the NI.

Comment 20

[Greg Weng [:snowmantw][:gweng][:λ]]

Okay I double-checked the patch in Bug 1255128. There is a |ToNumber|, and maybe you already check the case of |!SameValueZero(numberLength, byteLength)| in the lines of |length != byteLength| and the over-sized case. If I read it correctly, maybe this should be closed.

Comment 21

[Jason Orendorff [:jorendorff]]

Merging. Thanks for the tests, Greg.