1101256 - Implement detachment checks for %TypedArray% methods

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Morgan Phillips [:mrrrgn]]

Gonna crack at this since I've been working on TypedArray methods that want for it.

Comment 2

[Morgan Phillips [:mrrrgn]]

Herp derp, once I started implementing this, I found that someone else already had ! There are still lots of sites where the check is not being used (presumably because they didn't know the function existed either, or came before it was implemented). I'll make a patch to fix that up and put it here.

Comment 3

[Morgan Phillips [:mrrrgn]]

Attachment: detachedbuffer.diff

This adds the check and some tests on methods which were explicitly tagged with this bug number in place of where the check should go. In the spec there is an abstract method "ValidateTypedArray" which encapsulates this pattern: 

    // This function is not generic.
    if (!IsObject(this) || !IsTypedArray(this)) {
        return callFunction(CallTypedArrayMethodIfWrapped, this, target, start, end,
                            "TypedArrayCopyWithin");
    }

    var buffer = TypedArrayBuffer(this);
    if (IsDetachedBuffer(buffer))
        ThrowTypeError(JSMSG_TYPED_ARRAY_DETACHED);

Which were are using now.  

Is there any reason why we should use "
return callFunction(CallTypedArrayMethodIfWrapped, this, target, start, end,
                            "TypedArrayCopyWithin")" 

instead of just throwing an TypeError explicitly? Doing that would allow us to move all of these checks into a "ValidateTypedArray" function for simpler management and DRYer code.

Comment 4

[Morgan Phillips [:mrrrgn]]

My question above was answered by Waldo in irc:

12:20 PM <mrrrgn> The spec requires a check which throws if it fails, but instead of throwing we're using: callFunction(CallTypedArrayMethodIfWrapped, this, target, start, end,
12:20 PM <mrrrgn>                             "TypedArrayCopyWithin")" 
12:21 PM <mrrrgn> Why do that instead of just throwing with a generic error message?
12:21 PM — mrrrgn thinks we can make the functions more DRY
12:22 PM <Waldo> mrrrgn: so, if you have |var ta = new Uint8Array(); var ota = new otherWindow.Uint8Array();|
12:22 PM <Waldo> mrrrgn: ta.copyWithin is supposed to be callable with |ta| as this, *and* with |ota| as this
12:23 PM <Waldo> mrrrgn: IsTypedArray checks *only* that the value/object is a typed array from the copyWithin method's global
12:23 PM <Waldo> mrrrgn: it doesn't detect typed arrays from other globals

Comment 5

[Morgan Phillips [:mrrrgn]]

Changing reviewers just because I've recently discussed TypedArray.js with Waldo in irc, seems appropriate.

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 8699807 [details] [diff] [review]
detachedbuffer.diff

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

So this is sort of fine as far as it goes, but it's definitely not complete.  For example, %TypedArray%.prototype.every has:

"""
This function is not generic. ValidateTypedArray is applied to the this value prior to evaluating the algorithm. If its result is an abrupt completion that exception is thrown instead of evaluating the algorithm.
"""

and if you look at the algorithm, this means that |var ta = new Uint8Array(); neuter(ta.buffer, "change-data"); ta.every(() => true);| should throw rather than evaluate to true.  I expect similar issues exist for every function defined in TypedArray.js (excepting only TypedArraySet because I haven't made us use it yet).

So whatever patching happens here, it's going to need to tackle a whole bunch more functions to be a proper fix.

::: js/src/builtin/TypedArray.js
@@ +11,5 @@
>          return callFunction(CallTypedArrayMethodIfWrapped, this, target, start, end,
>                              "TypedArrayCopyWithin");
>      }
>  
> +    var buffer = TypedArrayBuffer(this);

If you look at the implementation, this will go out of its way to create an ArrayBuffer for typed arrays that (initially) stored their elements in inline storage.  But when the buffer isn't then going to be used, this is just wasted work.  You should work slightly differently/harder to avoid this.  See what SetFromNonTypedArray does, using ViewedArrayBufferIfReified, and do that instead.

@@ +87,5 @@
>          return callFunction(CallTypedArrayMethodIfWrapped, O, "TypedArrayEntries");
>      }
>  
> +    var buffer = TypedArrayBuffer(O);
> +    if (IsDetachedBuffer(buffer))

IfReified again.

@@ +453,5 @@
>          return callFunction(CallTypedArrayMethodIfWrapped, O, "TypedArrayKeys");
>      }
>  
> +    var buffer = TypedArrayBuffer(O);
> +    if (IsDetachedBuffer(buffer))

IfReified again.

@@ +965,5 @@
>      if (!IsObject(O) || !IsTypedArray(O)) {
>          return callFunction(CallTypedArrayMethodIfWrapped, O, "TypedArrayValues");
>      }
> +    var buffer = TypedArrayBuffer(O);
> +    if (IsDetachedBuffer(buffer))

IfReified

::: js/src/tests/ecma_6/TypedArray/entries.js
@@ +58,5 @@
> +      var buffer = new ArrayBuffer(10);
> +      var u8array = new Uint8Array(buffer);
> +      neuter(buffer, "change-data");
> +      u8array.entries();
> +}, TypeError, "Assert that entries fails if when ArrayBuffer is detached.");

s/if when/when/ or something

::: js/src/tests/ecma_6/TypedArray/keys.js
@@ +58,5 @@
> +      var buffer = new ArrayBuffer(10);
> +      var u8array = new Uint8Array(buffer);
> +      neuter(buffer, "change-data");
> +      u8array.keys();
> +}, TypeError, "Assert that entries fails if when ArrayBuffer is detached.");

Same if when thing as in the other.

Comment 7

[Morgan Phillips [:mrrrgn]]

Attachment: detachedbuffer.diff

Thanks for all of your reviews. This covers all of the methods I could find, though I'll give another comb over if/when a patch is accepted. https://media2.giphy.com/media/10N0qMA6FLi4BW/200_s.gif

Comment 8

[Jeff Walden [:Waldo]]

Comment on attachment 8707157 [details] [diff] [review]
detachedbuffer.diff

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

::: js/src/builtin/TypedArray.js
@@ +708,5 @@
>      var flags = UnsafeGetInt32FromReservedSlot(buffer, JS_ARRAYBUFFER_FLAGS_SLOT);
>      return (flags & JS_ARRAYBUFFER_NEUTERED_FLAG) !== 0;
>  }
>  
> +function DetachedArrayBufferCheck(tarray) {

Super-mild preference -- in a separate patch can we move this method, IsDetachedBuffer, etc. up to the top of the file above all callers, so we're not "relying" on variable hoisting to make this work?  (Set aside that in self-hosting code we don't "hoist" in the regular sense at all.)  Always better not to depend on hoisting, IMO.

As a name, GetAttachedArrayBuffer?  Seems to indicate the meaning well, for the rare cases that do want the buffer afterward.

@@ +822,5 @@
>      if (targetOffset < 0)
>          ThrowRangeError(JSMSG_TYPED_ARRAY_NEGATIVE_ARG, "2");
>  
>      // Steps 9-10.
> +    var targetBuffer = DetachedArrayBufferCheck(target);

Hmm.  If I ever get around to enabling this self-hosted version :-\ we should maybe see if we can avoid reifying the buffer, since .set doesn't actually expose the buffer directly at all.

@@ +1091,5 @@
>      // Step 3.
>      var len = TypedArrayLength(obj);
>  
>      if (comparefn === undefined) {
>          comparefn = TypedArrayCompare;

In the process of skimming through this file, I noticed that TypedArrayCompare falls off the end without returning a value.  I'm gonna guess |undefined| coerces to NaN and so things are okay, but clearly this is not so copacetic.  Please fix?  Grab me on IRC for a quick pastebin-review, the fix is almost certainly not worth a full-fledged bug for a one-liner that's probably |return 0;|.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/214b31421207

Comment 10

[Morgan Phillips [:mrrrgn]]

Attachment: detachedbuffer2.diff

I dropped the ball when finalizing the last patch and left out tests ! The good news is we were going to move up the detachment check helper functions in a separate patch anyway, so I've included them here.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/214b31421207

Comment 12

[Jeff Walden [:Waldo]]

Comment on attachment 8707192 [details] [diff] [review]
detachedbuffer2.diff

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

::: js/src/tests/ecma_6/TypedArray/detached-array-buffer-checks.js
@@ +8,5 @@
> +let array  = new Int32Array(buffer);
> +neuter(buffer, "change-data");
> +
> +assertThrowsInstanceOf(() => {
> +    array.copyWithin(0, 1, 2);

So ideally we should be checking that all the effectful steps in each method, are not taken.  Please add something along the lines of

  var POISON = (function() {
    var internalTarget = {};
    var throwForAllTraps =
      new Proxy(internalTarget, { get(target, prop, receiver) {
        assertEq(target, internalTarget);
        assertEq(receiver, throwForAllTraps);
        throw "FAIL: " + prop + " trap invoked";
      } });
    return new Proxy(throwForAllTraps, throwForAllTraps);
  });

and then pass POISON for all arguments here, and for all the other builtins that require arguments be passed, as necessary.

@@ +84,5 @@
> +    array.values();
> +}, TypeError);
> +
> +assertThrowsInstanceOf(() => {
> +    array.every(()=>true);

...that is, passing POISON here should not cause the test to fail, even tho POISON is not a callable.  (Technically both cases throw a TypeError so no difference would be observed except in the error message.  But error message-checking is a pain for anyone changing js.msg, so I don't think we should do it here just for extra precision.)

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb4198efe3cd

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fb4198efe3cd