TypedArray constructor with ArrayBuffer from other global throws exception from wrong global

RESOLVED FIXED in Firefox 55

Status

()

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

People

(Reporter: anba, Assigned: anba)

Tracking

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Assignee)

Description

a year ago
Test case:
---
var global = newGlobal();
var ab = new global.ArrayBuffer(0);
try {
  new Int8Array(ab, 4);
} catch (e) {
  print(e instanceof RangeError);
  print(e instanceof global.RangeError);
}
---

Expected: Prints "true" and then "false"
Actual: Prints "false" and then "true"
(Assignee)

Comment 1

a year ago
Replacing the NonGenericMethodGuard approach in TypedArrayObjectTemplate::fromBufferWithProto() should fix this bug. And replacing it is also needed to get the various bounds and detachment checks in the correct (= spec compliant) order, which should be done as part of bug 1317383. Therefore I'm setting this bug as a blocker for bug 1317383.
Blocks: 1317383
(Assignee)

Updated

a year ago
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
(Assignee)

Comment 2

a year ago
Created attachment 8844934 [details] [diff] [review]
bug1345115.patch

The patch uses the same approach to handle cross-compartment ArrayBuffers like the DataView constructor. This ensures we throw exceptions from the correct global and allows us to reorder the steps to follow the spec more closely.

TypedArrayObjectTemplate::getConstructorArgsForBuffer(...)
- This new helper method implements ES2017 22.2.4.5, steps 6-9 and 11.a.
- It's similar to DataViewObject::getAndCheckConstructorArgs(), but I'm not quite happy about having three out-params, because it feels a bit too much. And the part where ArrayBuffer's byteLength is retrieved in getConstructorArgsForBuffer(), passed back to create(), and then used in fromBufferSameCompartment()/fromBufferWrapped() seems a bit confusing. Maybe it's easier to understand the code when we keep steps 6-9 and 11.a inlined in create() and instead use a |bool isWrapped| to decide if we need to call fromBufferSameCompartment() or fromBufferWrapped(). WDYT?

TypedArrayObjectTemplate::computeAndCheckLength(...)
- This method implements ES2017 22.2.4.5, steps 7, 10.a-c, 11.b-c.
- We could reorder the if-statements so they match the spec steps, but I'd like to defer this for a follow-up bug.
- Step 7 is checked again because computeAndCheckLength() is also called from TypedArrayObjectTemplate::fromBuffer() (called from JSAPI functions).
- I've changed |lengthInt == -1| to |lengthInt < 0| to ensure JSAPI users can't do bad things when a negative number other than -1 is passed to TypedArrayObjectTemplate::fromBuffer().

TypedArrayObjectTemplate::makeInstance
- Removed unused variant without prototype argument.
- Added assertion that the ArrayBuffer must not be detached.

Class.h, GlobalObject.h, ArrayBufferObject.h:
- Removed no longer used createTypedArrayFromBuffer functions.

TypedArrayObject.h:
- Removed no longer used IsAnyArrayBuffer functions.

DataViewObject:
- Removed stale comment.
Attachment #8844934 - Flags: review?(jwalden+bmo)
Comment on attachment 8844934 [details] [diff] [review]
bug1345115.patch

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

*gulp*  So hairy.

::: js/src/tests/ecma_6/TypedArray/constructor-buffer-sequence-nonstandard.js
@@ +5,5 @@
> +// be incorrect.
> +
> +const otherGlobal = newGlobal();
> +
> +function* createBuffers(lengths = [0, 8]) {

Why not have

  var lengths = [0, 8];

inside the function instead?

::: js/src/tests/ecma_6/TypedArray/constructor-buffer-sequence.js
@@ +18,5 @@
> +
> +const poisonedValue = {
> +    valueOf() {
> +        throw new Error("Poisoned Value");
> +    }

Ideally you'd use that Proxy gimmick that throws if you so much as squint at it here.

@@ +42,5 @@
> +    let constructor = Object.defineProperty(function(){}.bind(null), "prototype", {
> +        get() {
> +            throw new ExpectedError();
> +        }
> +    });

Maybe abstract all this into

function ConstructorWithThrowingPrototype(detach)
{
  return Object.defineProperty(function(){}.bind(null), "prototype", {
                                 get() {
                                   if (detach)
                                     detach();
                                   throw new ExpectedError();
                                 }
                               });
}

rather than copypastaing it and variations throughout?

::: js/src/vm/TypedArrayObject.cpp
@@ +713,5 @@
>          MOZ_ASSERT(args.isConstructing());
>          RootedObject newTarget(cx, &args.newTarget().toObject());
>  
>          /* () or (number) */
>          uint32_t len = 0;

22.2.4.2TypedArray ( length )
This description applies only if the TypedArray function is called with at least one argument and the Type of the first argument is not Object.

suggests that we're not doing the right thing in cases like

js> new Uint8Array("2", { hi: 'bye' });
typein:1:1 TypeError: invalid arguments
Stack:
  @typein:1:1

Something to fix in a separate bug.  (And, uh, have spec semantics always been this way?)

@@ +832,2 @@
>              }
>          }

Add a comment to the effect that step 11c is subsequently performed by the callers, in computeAndCheckLength.  So gnarl.

@@ +854,5 @@
>  
>          uint32_t len;
> +        if (lengthInt < 0) {
> +            // Step 10.b.
> +            len = (bufferByteLength - byteOffset) / sizeof(NativeType);

Kinda prefer

  uint32_t newByteLength = bufferByteLength - byteOffset;

  // Step 10.b.
  len = newByteLength / sizeof(NativeType);

  // Steps 10.a, 10.c.
  if (len * sizeof(NativeType) != newByteLength)

(and note that step 10c is now cited).
Attachment #8844934 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 4

a year ago
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #3)
> Comment on attachment 8844934 [details] [diff] [review]
> bug1345115.patch
> 
> Review of attachment 8844934 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> *gulp*  So hairy.
> 

Agreed! :-)

Fortunately it gets a bit better with the latest changes from littledan (https://github.com/tc39/ecma262/pull/852), but it also means the current patch no longer matches the current spec draft, so I'll need to prepare an additional patch. 


> 
> ::: js/src/vm/TypedArrayObject.cpp
> @@ +713,5 @@
> >          MOZ_ASSERT(args.isConstructing());
> >          RootedObject newTarget(cx, &args.newTarget().toObject());
> >  
> >          /* () or (number) */
> >          uint32_t len = 0;
> 
> 22.2.4.2TypedArray ( length )
> This description applies only if the TypedArray function is called with at
> least one argument and the Type of the first argument is not Object.
> 
> suggests that we're not doing the right thing in cases like
> 
> js> new Uint8Array("2", { hi: 'bye' });
> typein:1:1 TypeError: invalid arguments
> Stack:
>   @typein:1:1
> 
> Something to fix in a separate bug.  (And, uh, have spec semantics always
> been this way?)

This will be fixed in bug 1317383. 


> @@ +854,5 @@
> >  
> >          uint32_t len;
> > +        if (lengthInt < 0) {
> > +            // Step 10.b.
> > +            len = (bufferByteLength - byteOffset) / sizeof(NativeType);
> 
> Kinda prefer
> 
>   uint32_t newByteLength = bufferByteLength - byteOffset;
> 
>   // Step 10.b.
>   len = newByteLength / sizeof(NativeType);
> 
>   // Steps 10.a, 10.c.
>   if (len * sizeof(NativeType) != newByteLength)
> 
> (and note that step 10c is now cited).

But citing step 10.c isn't correct, is it?
(Assignee)

Comment 5

a year ago
Created attachment 8854998 [details] [diff] [review]
bug1345115-part1-before-spec-update.patch

Update patch per review comments, carrying r+ from Waldo.
Attachment #8844934 - Attachment is obsolete: true
Attachment #8854998 - Flags: review+
(Assignee)

Comment 6

a year ago
Created attachment 8855008 [details] [diff] [review]
bug1345115-part2-reduce-computations.patch

This moves the byteOffset alignment check to fromBuffer(), so we don't need to duplicate the check in computeAndCheckLength(). And it reorders the checks in computeAndCheckLength to remove unnecessary checks.

For 22.2.4.5, step 10 we only need the |(len >= INT32_MAX / sizeof(NativeType))| check.

We can remove the |arrayByteLength + byteOffset > bufferByteLength| check, because we know |arrayByteLength = len * sizeof(NativeType) = newByteLength = bufferByteLength - byteOffset|.

That means
|arrayByteLength + byteOffset > bufferByteLength|
=> |bufferByteLength - byteOffset + byteOffset > bufferByteLength|
=> |bufferByteLength > bufferByteLength|
=> |false|

We can also remove the |byteOffset >= INT32_MAX - arrayByteLength| check, because
|byteOffset >= INT32_MAX - arrayByteLength|
=> |byteOffset >= INT32_MAX - bufferByteLength + byteOffset|
=> |bufferByteLength >= INT32_MAX|

And given that we also check for
|len >= INT32_MAX / sizeof(NativeType)|
=> |newByteLength / sizeof(NativeType) >= INT32_MAX / sizeof(NativeType)|
=> |newByteLength >= INT32_MAX|
=> |bufferByteLength - byteOffset >= INT32_MAX|

And since |byteOffset >= 0|, the |bufferByteLength >= INT32_MAX| test is included in |bufferByteLength - byteOffset >= INT32_MAX|.


And I've also merged |byteOffset >= INT32_MAX - arrayByteLength| and |arrayByteLength + byteOffset > bufferByteLength| in 22.2.4.5, step 11, to just test for |arrayByteLength > bufferByteLength - byteOffset|. But this will change again in bug 1317383, when I'll update the method to use uint64_t for the byteOffset and length parameters.
Attachment #8855008 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 7

a year ago
Created attachment 8855011 [details] [diff] [review]
bug1345115-part2-reduce-computations.patch

Missed a |return false| -> |return nullptr| update.
Attachment #8855008 - Attachment is obsolete: true
Attachment #8855008 - Flags: review?(jwalden+bmo)
Attachment #8855011 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 8

a year ago
Created attachment 8855014 [details] [diff] [review]
bug1345115-part3-latest-spec-changes.patch

This is so much nicer than the first patch! :-)
Attachment #8855014 - Flags: review?(jwalden+bmo)
Comment on attachment 8855011 [details] [diff] [review]
bug1345115-part2-reduce-computations.patch

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

Changing a false->nullptr would clearly not merit re-review if it were my patch and I were running it through the review process.  Just sayin'.
Attachment #8855011 - Flags: review?(jwalden+bmo) → review+

Updated

a year ago
Attachment #8855014 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 10

a year ago
Created attachment 8859618 [details] [diff] [review]
bug1345115-part1-before-spec-update.patch

Update part 1 to apply cleanly on inbound, carrying r+ from Waldo.
Attachment #8854998 - Attachment is obsolete: true
Attachment #8859618 - Flags: review+

Comment 12

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4126ffda9153
Part 1: Don't switch compartment when typed array constructor is called with arraybuffer from other compartment. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f44dcf0d340
Part 2: Reduce duplicated byte length/offset computations in TypedArray constructor. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4bdc406fc1d
Part 3: Update TypedArray constructor with ArrayBuffer argument to follow latest spec. r=Waldo
Keywords: checkin-needed

Comment 13

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