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)
Core
JavaScript: Standard Library
Tracking
()
RESOLVED
FIXED
mozilla54
People
(Reporter: anba, Assigned: anba)
Details
Attachments
(1 file, 2 obsolete files)
9.61 KB,
patch
|
anba
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•7 years ago
|
||
The patch uses the same approach as in TypedArrayObjectTemplate<T>::fromTypedArray() to detect and handle wrapped typed arrays. Applies on top of bug 1317384.
Comment 2•7 years ago
|
||
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+
Assignee | ||
Comment 3•7 years ago
|
||
(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 4•7 years ago
|
||
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+
Updated•7 years ago
|
Flags: needinfo?(jwalden+bmo)
Assignee | ||
Comment 5•7 years ago
|
||
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 6•7 years ago
|
||
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+
Assignee | ||
Comment 7•7 years ago
|
||
Comma splices delendae erant.
Attachment #8842039 -
Attachment is obsolete: true
Attachment #8842792 -
Flags: review+
Assignee | ||
Comment 8•7 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b70fed770700851f21011819c0d21c0ab1558b2d
Keywords: checkin-needed
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
Comment 10•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9b990c5890a8
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 11•7 years ago
|
||
Do you think this should be uplifted to 53?
Comment 12•7 years ago
|
||
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)
Updated•7 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•