Closed Bug 1317382 Opened 7 years ago Closed 7 years ago

DataView constructor uses ToIndex in ES2017

Categories

(Core :: JavaScript: Standard Library, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox52 --- wontfix
firefox53 --- wontfix
firefox54 --- fixed

People

(Reporter: anba, Assigned: evilpie)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file)

For example `new DataView(new ArrayBuffer(10), -Infinity)` is now expected to throw a RangeError.


ES2017 spec:
https://tc39.github.io/ecma262/#sec-dataview-buffer-byteoffset-bytelength
https://tc39.github.io/ecma262/#sec-toindex


The following test262 tests expect the new ES2017 semantics:
built-ins/DataView/byteoffset-is-negative-throws.js
built-ins/DataView/excessive-byteoffset-throws.js
built-ins/DataView/excessive-bytelength-throws.js
built-ins/DataView/negative-byteoffset-throws.js
built-ins/DataView/negative-bytelength-throws.js
Depends on: 1255128
I seem to have incidentally fixed this in other work, in my tree, so I'll take this.
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED
Attachment #8836469 - Flags: review?(jwalden+bmo)
Comment on attachment 8836469 [details] [diff] [review]
Use ToIndex in DataView constructor

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

::: js/src/builtin/DataViewObject.cpp
@@ +173,5 @@
> +        return false;
> +    }
> +
> +    MOZ_ASSERT(offset <= INT32_MAX);
> +    *byteOffsetPtr = uint32_t(offset);

Please put the |*byteOffsetPtr| and |*byteLengthPtr| assignments at the end of the function, and assign into locals in earlier code.  Makes clearer that we've verified *both* are consistent at one instant, IMO, than assigning earlier.

@@ +178,5 @@
> +
> +    // Step 8.
> +    if (!args.hasDefined(2)) {
> +        // Step 8.a.
> +        *byteLengthPtr = bufferByteLength - uint32_t(offset);

Put a |uint64_t viewByteLength;| above the if-else and set it here.

@@ +186,5 @@
> +        if (!ToIndex(cx, args.get(2), &viewByteLength))
> +            return false;
> +
> +        // Step 9.b.
> +        if (offset + viewByteLength > bufferByteLength) {

Before this, add

MOZ_ASSERT(offset + viewByteLength >= offset,
           "can't overflow: both numbers are less than DOUBLE_INTEGRAL_PRECISION_LIMIT");
Attachment #8836469 - Flags: review?(jwalden+bmo) → review+
Assignee: jwalden+bmo → evilpies
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a394ec7d209
DataView constructor uses ToIndex in ES2017. r=Waldo
https://hg.mozilla.org/mozilla-central/rev/5a394ec7d209
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Is this a potential candidate for 53, or should it ride the trains?
Flags: needinfo?(evilpies)
Changes to web-visible semantics that aren't necessitated by a security fix usually aren't backported, so I'm not sure why we'd consider this.
Flags: needinfo?(evilpies)
You need to log in before you can comment on or make changes to this bug.