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

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[André Bargull [:anba]]

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"

Comment 1

[André Bargull [:anba]]

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.

Comment 2

[André Bargull [:anba]]

Attachment: 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.

Comment 3

[Jeff Walden [:Waldo]]

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).

Comment 4

[André Bargull [:anba]]

(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?

Comment 5

[André Bargull [:anba]]

Attachment: bug1345115-part1-before-spec-update.patch

Update patch per review comments, carrying r+ from Waldo.

Comment 6

[André Bargull [:anba]]

Attachment: 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.

Comment 7

[André Bargull [:anba]]

Attachment: bug1345115-part2-reduce-computations.patch

Missed a |return false| -> |return nullptr| update.

Comment 8

[André Bargull [:anba]]

Attachment: bug1345115-part3-latest-spec-changes.patch

This is so much nicer than the first patch! :-)

Comment 9

[Jeff Walden [:Waldo]]

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'.

Comment 10

[André Bargull [:anba]]

Attachment: bug1345115-part1-before-spec-update.patch

Update part 1 to apply cleanly on inbound, carrying r+ from Waldo.

Comment 11

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d57eef5067443698297688d1bcb80cae8b27bfab

Comment 12

[Pulsebot]

Pushed by [e-mail]:
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

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4126ffda9153
https://hg.mozilla.org/mozilla-central/rev/8f44dcf0d340
https://hg.mozilla.org/mozilla-central/rev/b4bdc406fc1d