Closed Bug 1314148 Opened 8 years ago Closed 7 years ago

TypedArray.prototype.set uses the non-typed array path for wrapped typed arrays

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: anba)

Details

Attachments

(1 file, 2 obsolete files)

Test case:
---
var ta = new Int8Array(2);
var wta = newGlobal().eval("new Int8Array([10, 20])");
Object.defineProperty(wta, "length", {value: 1});
ta.set(wta);

assertEq(ta[1], 20);
---

Expected: Passes.
Actual: Assertion failed: got 0, expected 20
Attached patch bug1314148.patch (obsolete) — Splinter Review
The patch uses the same approach as in TypedArrayObjectTemplate<T>::fromTypedArray() to detect and handle wrapped typed arrays. 

Applies on top of bug 1317384.
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
Attachment #8836002 - Flags: review?(lhansen)
Comment on attachment 8836002 [details] [diff] [review]
bug1314148.patch

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

This looks OK to my eyes but I'm not the best qualified person to judge.  If you are uncertain about anything perhaps ask arai or waldo for a second opinion.
Attachment #8836002 - Flags: review?(lhansen) → review+
(In reply to Lars T Hansen [:lth] from comment #2)
> If you are uncertain about anything perhaps ask arai or waldo for a second
> opinion.

I think it should be okay, but it probably doesn't hurt to ask Waldo for his opinion. :-)

@Waldo: Do you spot anything wrong with this patch? I've copied the approach to handle wrapped typed arrays from TypedArrayObjectTemplate<T>::fromTypedArray(), under the assumption this is the correct and safe way to unwrap them.
Flags: needinfo?(jwalden+bmo)
Comment on attachment 8836002 [details] [diff] [review]
bug1314148.patch

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

Looks broadly okay, but the approach is a little fruity-looking.

::: js/src/tests/ecma_6/TypedArray/set-wrapped.js
@@ +61,5 @@
> +            otherGlobal.detachArrayBuffer(source.buffer);
> +            assertThrowsInstanceOf(() => target.set(source), TypeError);
> +
> +            var source = new otherGlobal[TA.name](1);
> +            taintLengthProperty(source);

Shouldn't this bit really be

  var source = new TA(new otherGlobal.ArrayBuffer(1 * TA.BYTES_PER_ELEMENT));

so that it's testing the same thing as eight lines prior, just like all the other cases in this file do?

::: js/src/vm/TypedArrayObject.cpp
@@ +1555,5 @@
> +    bool isTypedArray = false, isWrapped = false;
> +    if (args.get(0).isObject()) {
> +        JSObject* source = &args[0].toObject();
> +        if (MOZ_UNLIKELY(source->is<WrapperObject>())) {
> +            source = UncheckedUnwrap(source);

I'm inclined to say the better approach to be copying than TAOT::fromTypedArray is than in intrinsic_IsPossiblyWrappedTypedArray.

Well, okay, the two functions both do it about the same way.  Just, the approach *this* patch implements seems a bit muddled, and not so clean as I would have expected it to be.

So how about something like this.  |args[0]| *must* be an object in the typed array/non-typed array cases both, so what if we recorded typed array-ness in a TypedArrayObject* and then branched on that, like so:

  // 22.2.3.23.1, step 15.  (22.2.3.23.2 only applies if args[0] is a typed array, so
  // it doesn't make a difference there to apply ToObject here.)
  RootedObject src(cx, ToObject(cx, args.get(0)));
  if (!src)
    return false;

  Rooted<TypedArrayObject*> typedArray(cx, nullptr);
  {
    JSObject* obj = CheckedUnwrap(src);
    if (!obj) {
      ReportAccessDenied(cx);
      return false;
    }

    if (obj->is<TypedArrayObject>())
      typedArray = &obj->as<TypedArrayObject>();
  }

  if (typedArray) {
    // Remaining steps of 22.2.3.23.2.

    // Steps 11-12.
    ...
  } else {
    // Remaining steps of 22.2.3.23.1.

    // Step 10.
    // We can't reorder...
    uint32_t targetLength = target->length();

    // Step 16.
    uint32_t srcLength;
    ...
  }

I'll briefly note this doesn't include a JSAutoCompartment in it, which may raise some hackles -- but IMO this is basically unavoidable.  It is unavoidable for the same reason it was unavoidable in the partially self-hosted version of %TypedArray%.prototype.set, yet to be enabled.  And while at the time I discussed that with bholley I thought maybe there was a way to structure the code to placate him, I no longer think that's the case.

One final thought: please add suitable warning/danger comments by SetFromTypedArray, TAOT::setFromTypedArray, and TAOT::setFromOverlappingTypedArray indicating that the source argument may be from another compartment and that caution is required.  It looks like all that code would be unaffected by the cross-compartmentness, and it would throw TypeErrors and such from the correct compartment, but the code there nonetheless *is* playing with fire some.
Attachment #8836002 - Flags: feedback+
Flags: needinfo?(jwalden+bmo)
Attached patch bug1314148.patch (obsolete) — Splinter Review
Fixed the copy-paste error in the test case and updated the implementation per comment #4.
Attachment #8836002 - Attachment is obsolete: true
Attachment #8842039 - Flags: review?(jwalden+bmo)
Comment on attachment 8842039 [details] [diff] [review]
bug1314148.patch

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

::: js/src/vm/TypedArrayObject-inl.h
@@ +273,5 @@
>                        Handle<TypedArrayObject*> target, Handle<TypedArrayObject*> source,
>                        uint32_t offset)
>      {
> +        // WARNING: |source| may be an unwrapped typed array from a different
> +        // compartment, proceed with caution!

s/, p/.  P/

and below as well.  Comma splices delenda est!

::: js/src/vm/TypedArrayObject.cpp
@@ +1503,5 @@
>  SetFromTypedArray(JSContext* cx, Handle<TypedArrayObject*> target,
>                    Handle<TypedArrayObject*> source, uint32_t offset)
>  {
> +    // WARNING: |source| may be an unwrapped typed array from a different
> +    // compartment, proceed with caution!

Same comma splice thing.

@@ +1578,1 @@
>  

Probably worth a similar WARNING comment in this section before the rest of the code.
Attachment #8842039 - Flags: review?(jwalden+bmo) → review+
Attached patch bug1314148.patchSplinter Review
Comma splices delendae erant.
Attachment #8842039 - Attachment is obsolete: true
Attachment #8842792 - Flags: review+
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b990c5890a8
Correctly handle wrapped typed arrays in TypedArray.prototype.set. r=lth, waldo
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9b990c5890a8
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Do you think this should be uplifted to 53?
Flags: needinfo?(andrebargull)
As with the other bug, 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?(andrebargull)
You need to log in before you can comment on or make changes to this bug.