Check for detached array buffers in TypedArray.prototype.slice

RESOLVED FIXED in Firefox 54

Status

()

Core
JavaScript: Standard Library
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: anba, Assigned: anba)

Tracking

(Blocks: 1 bug)

Trunk
mozilla54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

a year ago
Add an explicit check for detached array buffers because we don't throw for detached array buffers in typed array's [[Get]] method.
(Assignee)

Comment 1

a year ago
Created attachment 8841191 [details] [diff] [review]
bug1342663.patch

This adds an explicit detached array buffer check for step 14.b.ii, in the spec this check is implicitly performed through the detached check in typed array's [[Get]] method.
Attachment #8841191 - Flags: review?(arai.unmht)
Comment on attachment 8841191 [details] [diff] [review]
bug1342663.patch

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

Great testcase :)

::: js/src/builtin/TypedArray.js
@@ +1022,5 @@
> +        // Steps 14.b.ii, 15.b.
> +        if (buffer === null)
> +            buffer = GetAttachedArrayBuffer(O);
> +
> +        if (IsDetachedBuffer(buffer))

GetAttachedArrayBuffer checks IsDetachedBuffer internally, so it's redundant to check it here for `buffer === null` case above.

how about this?

  if (buffer === null)
    GetAttachedArrayBuffer(O);
  else if (IsDetachedBuffer(buffer))
    ThrowTypeError(JSMSG_TYPED_ARRAY_DETACHED);
Attachment #8841191 - Flags: review?(arai.unmht) → review+
(Assignee)

Comment 3

a year ago
(In reply to Tooru Fujisawa [:arai] from comment #2)
> ::: js/src/builtin/TypedArray.js
> @@ +1022,5 @@
> > +        // Steps 14.b.ii, 15.b.
> > +        if (buffer === null)
> > +            buffer = GetAttachedArrayBuffer(O);
> > +
> > +        if (IsDetachedBuffer(buffer))
> 
> GetAttachedArrayBuffer checks IsDetachedBuffer internally, so it's redundant
> to check it here for `buffer === null` case above.
> 
> how about this?
> 
>   if (buffer === null)
>     GetAttachedArrayBuffer(O);
>   else if (IsDetachedBuffer(buffer))
>     ThrowTypeError(JSMSG_TYPED_ARRAY_DETACHED);

Oops, you're totally right. Thanks for spotting that! I had planned to copy the pattern from [1], so the second call to GetAttachedArrayBuffer should instead be a call to ViewedArrayBufferIfReified. Are you okay with that alternative? (I should probably also copy the comments from [1] to slice(), so it's clearer why it's necessary to recheck the buffer slot.)

[1] http://searchfox.org/mozilla-central/rev/93f1641e394cfbdfbeed44e81f7dab0f2aff7b6f/js/src/builtin/TypedArray.js#892-898
(In reply to André Bargull from comment #3)
> (In reply to Tooru Fujisawa [:arai] from comment #2)
> > ::: js/src/builtin/TypedArray.js
> > @@ +1022,5 @@
> > > +        // Steps 14.b.ii, 15.b.
> > > +        if (buffer === null)
> > > +            buffer = GetAttachedArrayBuffer(O);
> > > +
> > > +        if (IsDetachedBuffer(buffer))
> > 
> > GetAttachedArrayBuffer checks IsDetachedBuffer internally, so it's redundant
> > to check it here for `buffer === null` case above.
> > 
> > how about this?
> > 
> >   if (buffer === null)
> >     GetAttachedArrayBuffer(O);
> >   else if (IsDetachedBuffer(buffer))
> >     ThrowTypeError(JSMSG_TYPED_ARRAY_DETACHED);
> 
> Oops, you're totally right. Thanks for spotting that! I had planned to copy
> the pattern from [1], so the second call to GetAttachedArrayBuffer should
> instead be a call to ViewedArrayBufferIfReified. Are you okay with that
> alternative?

Yes :)
(Assignee)

Comment 5

a year ago
Created attachment 8841697 [details] [diff] [review]
bug1342663.patch

Updated per review comments, carrying r+ from arai.
Attachment #8841191 - Attachment is obsolete: true
Attachment #8841697 - Flags: review+

Comment 7

a year ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2358cbc6056
Check for detached source buffer in TypedArray.prototype.slice. r=arai
Keywords: checkin-needed

Comment 8

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f2358cbc6056
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox54: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.