Assertion failure: index < obj.length(), at js/src/vm/TypedArrayObject.cpp:2163

VERIFIED FIXED in Firefox -esr45

Status

()

Core
JavaScript Engine
VERIFIED FIXED
a year ago
a month ago

People

(Reporter: anba, Assigned: Waldo)

Tracking

({csectype-bounds, sec-critical})

Trunk
mozilla53
csectype-bounds, sec-critical
Points:
---
Bug Flags:
in-testsuite -
qe-verify +

Firefox Tracking Flags

(firefox-esr4551+ verified, firefox50 wontfix, firefox51+ verified, firefox52+ verified, firefox53+ verified)

Details

(Whiteboard: [post-critsmash-triage][adv-main51+][adv-esr45.7+])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

a year ago
m-c 679118259e91

Marked as s-s because it involves detached array buffers.


Assertion failure:
---
Assertion failure: index < obj.length(), at /home/andre/git/mozilla-central/js/src/vm/TypedArrayObject.cpp:2163
---


Test case:
---
var ta = new Int8Array(1);
Object.defineProperty(ta, 0, {
    value: {
        valueOf() {
            detachArrayBuffer(ta.buffer, "same-data");
            return 0;
        }
    }
});
---


Back trace:
---
#0  0x0000000000e2c372 in js::TypedArrayObject::setElement (obj=..., index=0, d=0)
    at /home/andre/git/mozilla-central/js/src/vm/TypedArrayObject.cpp:2163
#1  0x0000000000e2e630 in js::DefineTypedArrayElement (cx=0x7ffff6960000, obj=..., index=0, desc=..., result=...)
    at /home/andre/git/mozilla-central/js/src/vm/TypedArrayObject.cpp:2695
#2  0x0000000000d4123f in js::NativeDefineProperty (cx=0x7ffff6960000, obj=..., id=..., desc_=..., result=...)
    at /home/andre/git/mozilla-central/js/src/vm/NativeObject.cpp:1336
#3  0x0000000000b16374 in js::DefineProperty (cx=0x7ffff6960000, obj=..., id=..., desc=..., result=...)
    at /home/andre/git/mozilla-central/js/src/jsobj.cpp:2688
#4  0x0000000000b1629f in js::DefineProperty (cx=0x7ffff6960000, obj=..., id=..., desc=...)
    at /home/andre/git/mozilla-central/js/src/jsobj.cpp:2677
#5  0x0000000000ed35a4 in js::obj_defineProperty (cx=0x7ffff6960000, argc=3, vp=0x7ffff3e3f090)
    at /home/andre/git/mozilla-central/js/src/builtin/Object.cpp:911
#6  0x0000000000d0fdc8 in js::CallJSNative (cx=0x7ffff6960000, native=0xed338c <js::obj_defineProperty(JSContext*, unsigned int, JS::Value*)>, 
    args=...) at /home/andre/git/mozilla-central/js/src/jscntxtinlines.h:232
#7  0x0000000000ce7a1a in js::InternalCallOrConstruct (cx=0x7ffff6960000, args=..., construct=js::NO_CONSTRUCT)
    at /home/andre/git/mozilla-central/js/src/vm/Interpreter.cpp:453
#8  0x0000000000ce7d5b in InternalCall (cx=0x7ffff6960000, args=...) at /home/andre/git/mozilla-central/js/src/vm/Interpreter.cpp:498
#9  0x0000000000ce7d99 in js::CallFromStack (cx=0x7ffff6960000, args=...) at /home/andre/git/mozilla-central/js/src/vm/Interpreter.cpp:504
#10 0x0000000000cf5309 in Interpret (cx=0x7ffff6960000, state=...) at /home/andre/git/mozilla-central/js/src/vm/Interpreter.cpp:2873
#11 0x0000000000ce7673 in js::RunScript (cx=0x7ffff6960000, state=...) at /home/andre/git/mozilla-central/js/src/vm/Interpreter.cpp:399
#12 0x0000000000ce8c00 in js::ExecuteKernel (cx=0x7ffff6960000, script=..., scopeChainArg=..., newTargetValue=..., evalInFrame=..., result=0x0)
    at /home/andre/git/mozilla-central/js/src/vm/Interpreter.cpp:679
#13 0x0000000000ce8ee9 in js::Execute (cx=0x7ffff6960000, script=..., scopeChainArg=..., rval=0x0)
    at /home/andre/git/mozilla-central/js/src/vm/Interpreter.cpp:712
#14 0x0000000000a42a8c in ExecuteScript (cx=0x7ffff6960000, scope=..., script=..., rval=0x0)
    at /home/andre/git/mozilla-central/js/src/jsapi.cpp:4313
#15 0x0000000000a42e96 in JS_ExecuteScript (cx=0x7ffff6960000, scriptArg=...) at /home/andre/git/mozilla-central/js/src/jsapi.cpp:4346
#16 0x0000000000436910 in RunFile (cx=0x7ffff6960000, filename=0x7fffffffdfd1 "/tmp/t.js", file=0x7ffff3e3b000, compileOnly=false)
    at /home/andre/git/mozilla-central/js/src/shell/js.cpp:539
#17 0x0000000000437cd6 in Process (cx=0x7ffff6960000, filename=0x7fffffffdfd1 "/tmp/t.js", forceTTY=false, kind=FileScript)
    at /home/andre/git/mozilla-central/js/src/shell/js.cpp:860
#18 0x000000000044e941 in ProcessArgs (cx=0x7ffff6960000, op=0x7fffffffd9a0) at /home/andre/git/mozilla-central/js/src/shell/js.cpp:6788
#19 0x000000000044ff27 in Shell (cx=0x7ffff6960000, op=0x7fffffffd9a0, envp=0x7fffffffdbb0)
    at /home/andre/git/mozilla-central/js/src/shell/js.cpp:7137
#20 0x00000000004514ae in main (argc=2, argv=0x7fffffffdb98, envp=0x7fffffffdbb0) at /home/andre/git/mozilla-central/js/src/shell/js.cpp:7518
---
(Reporter)

Comment 1

a year ago
Also reproducible with:
---
var ta = new Int8Array(1);
var ta2 = new Int8Array(1);

Reflect.set(ta, 0, {
    valueOf() {
        detachArrayBuffer(ta2.buffer, "same-data");
        return 2;
    }
}, ta2);
---


Back trace:
---
0x0000000000e2c372 in js::TypedArrayObject::setElement (obj=..., index=0, d=2) at /home/andre/git/mozilla-central/js/src/vm/TypedArrayObject.cpp:2163
2163	    MOZ_ASSERT(index < obj.length());
(gdb) bt
#0  0x0000000000e2c372 in js::TypedArrayObject::setElement (obj=..., index=0, d=2) at /home/andre/git/mozilla-central/js/src/vm/TypedArrayObject.cpp:2163
#1  0x0000000000e2e630 in js::DefineTypedArrayElement (cx=0x7ffff6960000, obj=..., index=0, desc=..., result=...) at /home/andre/git/mozilla-central/js/src/vm/TypedArrayObject.cpp:2695
#2  0x0000000000d4123f in js::NativeDefineProperty (cx=0x7ffff6960000, obj=..., id=..., desc_=..., result=...) at /home/andre/git/mozilla-central/js/src/vm/NativeObject.cpp:1336
#3  0x0000000000b16524 in js::DefineProperty (cx=0x7ffff6960000, obj=..., id=..., value=..., getter=0x0, setter=0x0, attrs=114688, result=...) at /home/andre/git/mozilla-central/js/src/jsobj.cpp:2705
#4  0x0000000000d44634 in js::SetPropertyByDefining (cx=0x7ffff6960000, id=..., v=..., receiverValue=..., result=...) at /home/andre/git/mozilla-central/js/src/vm/NativeObject.cpp:2197
#5  0x0000000000d44dd4 in SetExistingProperty (cx=0x7ffff6960000, obj=..., id=..., v=..., receiver=..., pobj=..., shape=..., result=...) at /home/andre/git/mozilla-central/js/src/vm/NativeObject.cpp:2316
#6  0x0000000000d45555 in js::NativeSetProperty (cx=0x7ffff6960000, obj=..., id=..., value=..., receiver=..., qualified=js::Qualified, result=...)
    at /home/andre/git/mozilla-central/js/src/vm/NativeObject.cpp:2399
#7  0x000000000052fede in js::SetProperty (cx=0x7ffff6960000, obj=..., id=..., v=..., receiver=..., result=...) at /home/andre/git/mozilla-central/js/src/vm/NativeObject.h:1495
#8  0x0000000000eda69f in Reflect_set (cx=0x7ffff6960000, argc=4, vp=0x7ffff3e3f090) at /home/andre/git/mozilla-central/js/src/builtin/Reflect.cpp:307
#9  0x0000000000d0fdc8 in js::CallJSNative (cx=0x7ffff6960000, native=0xeda3df <Reflect_set(JSContext*, unsigned int, JS::Value*)>, args=...)
    at /home/andre/git/mozilla-central/js/src/jscntxtinlines.h:232
#10 0x0000000000ce7a1a in js::InternalCallOrConstruct (cx=0x7ffff6960000, args=..., construct=js::NO_CONSTRUCT) at /home/andre/git/mozilla-central/js/src/vm/Interpreter.cpp:453
#11 0x0000000000ce7d5b in InternalCall (cx=0x7ffff6960000, args=...) at /home/andre/git/mozilla-central/js/src/vm/Interpreter.cpp:498
#12 0x0000000000ce7d99 in js::CallFromStack (cx=0x7ffff6960000, args=...) at /home/andre/git/mozilla-central/js/src/vm/Interpreter.cpp:504
#13 0x0000000000cf5309 in Interpret (cx=0x7ffff6960000, state=...) at /home/andre/git/mozilla-central/js/src/vm/Interpreter.cpp:2873
#14 0x0000000000ce7673 in js::RunScript (cx=0x7ffff6960000, state=...) at /home/andre/git/mozilla-central/js/src/vm/Interpreter.cpp:399
#15 0x0000000000ce8c00 in js::ExecuteKernel (cx=0x7ffff6960000, script=..., scopeChainArg=..., newTargetValue=..., evalInFrame=..., result=0x0)
    at /home/andre/git/mozilla-central/js/src/vm/Interpreter.cpp:679
#16 0x0000000000ce8ee9 in js::Execute (cx=0x7ffff6960000, script=..., scopeChainArg=..., rval=0x0) at /home/andre/git/mozilla-central/js/src/vm/Interpreter.cpp:712
#17 0x0000000000a42a8c in ExecuteScript (cx=0x7ffff6960000, scope=..., script=..., rval=0x0) at /home/andre/git/mozilla-central/js/src/jsapi.cpp:4313
#18 0x0000000000a42e96 in JS_ExecuteScript (cx=0x7ffff6960000, scriptArg=...) at /home/andre/git/mozilla-central/js/src/jsapi.cpp:4346
#19 0x0000000000436910 in RunFile (cx=0x7ffff6960000, filename=0x7fffffffdfd1 "/tmp/t.js", file=0x7ffff3e3b000, compileOnly=false) at /home/andre/git/mozilla-central/js/src/shell/js.cpp:539
#20 0x0000000000437cd6 in Process (cx=0x7ffff6960000, filename=0x7fffffffdfd1 "/tmp/t.js", forceTTY=false, kind=FileScript) at /home/andre/git/mozilla-central/js/src/shell/js.cpp:860
#21 0x000000000044e941 in ProcessArgs (cx=0x7ffff6960000, op=0x7fffffffd9a0) at /home/andre/git/mozilla-central/js/src/shell/js.cpp:6788
#22 0x000000000044ff27 in Shell (cx=0x7ffff6960000, op=0x7fffffffd9a0, envp=0x7fffffffdbb0) at /home/andre/git/mozilla-central/js/src/shell/js.cpp:7137
#23 0x00000000004514ae in main (argc=2, argv=0x7fffffffdb98, envp=0x7fffffffdbb0) at /home/andre/git/mozilla-central/js/src/shell/js.cpp:7518
---
Keywords: csectype-bounds, sec-high
Group: core-security → javascript-core-security
Created attachment 8817103 [details] [diff] [review]
Test
Attachment #8817103 - Flags: review?(sphink)
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED
Created attachment 8817104 [details] [diff] [review]
Patch
Attachment #8817104 - Flags: review?(sphink)
I'm not sure why this was only sec-high when it's peas in a pod with bug 991981 and bug 982974, the latter of which had exploits in hand and the former which was kin to it.  Out-of-bounds writing like this is always critical.
Keywords: sec-high → sec-critical
Comment on attachment 8817104 [details] [diff] [review]
Patch

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

This was touched in bug 1148652, but that just moved code around; it was already broken. It looks like the bug was introduced in Fx38 in bug 1125628 (though it looks like that was probably just following the spec, so it was sort of a spec bug back then?)

::: js/src/vm/TypedArrayObject.cpp
@@ +3008,5 @@
>              return false;
>  
> +        // Steps 4-5, 8-9.
> +        if (obj->as<TypedArrayObject>().hasDetachedBuffer())
> +            return result.fail(JSMSG_TYPED_ARRAY_DETACHED);

I see steps 4-5 (the IsDetachedBuffer check). I do not see 8-9:

    Let length be O.[[ArrayLength]].
    If index < 0 or index ≥ length, return false.

Steps 4-5 seem to be covered earlier, since the only way the length could be changed is via detaching. And TypedArrayObject::setElement starts with

    MOZ_ASSERT(index < obj.length())

so it seems like we know we're assuming that.
Attachment #8817104 - Flags: review?(sphink) → review+
Comment on attachment 8817104 [details] [diff] [review]
Patch

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?

As easily as for bug 982974, which is to say pwn2own can do it, so attackers can.

> Do comments in the patch, the check-in comment, or tests included
> in the patch paint a bulls-eye on the security problem?

Not wholly obviously, because they're spec step numbers (and test is separate patch).  But the is-detached check is pretty obvious in its current absence and clear protection of an indexed write -- that's pretty glaring as an out-of-range buffer access.

> Which older supported branches are affected by this flaw?
> If not all supported branches, which bug introduced the flaw?

All (see prior comment for regressor).

> Do you have backports for the affected branches? If not, how
> different, hard to create, and risky will they be?

Not sure about *exact* backport-simplicity -- but just adding the is-detached check is super-trivial and easy if any backport issues are encountered.

> How likely is this patch to cause regressions; how much testing
> does it need?

Basically zero likelihood -- anyone hitting this is basically trying to exploit it.
Attachment #8817104 - Flags: sec-approval?
status-firefox50: affected → wontfix
status-firefox51: --- → affected
status-firefox52: --- → affected
status-firefox53: --- → affected
status-firefox-esr45: --- → affected
tracking-firefox51: --- → +
tracking-firefox52: --- → +
tracking-firefox53: --- → +
tracking-firefox-esr45: --- → 51+
Comment on attachment 8817104 [details] [diff] [review]
Patch

sec-approval=dveditz

Is the patch the same on aurora/beta/esr, or will you need modified versions?
Attachment #8817104 - Flags: sec-approval? → sec-approval+
Created attachment 8819430 [details] [diff] [review]
ESR45 backport

Only trivial changes were required for this backport.
Attachment #8817104 - Attachment is obsolete: true
Comment on attachment 8817104 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/Bug causing the regression]: allegedly bug 1125628

[User impact if declined]: sec-critical

[Is this code covered by automated tests?]: no *landed* tests because they'd give the game away; test attached to this bug awaiting review, to land whenever we think it's safe to do so.  (Note that the test here may require trivial changes for backporting to branches -- namely, the "same-data"/"change-data" arguments to detachArrayBuffer, present in comment 0, are not present on trunk -- not sure exactly when we removed those, offhand.)

[Has the fix been verified in Nightly?]: test fails w/o patch, passes with -- but not verified by an actual *landing* yet, because landing this too early gives the game away

[Needs manual test from QE? If yes, steps to reproduce]: no -- automated is good enough (once it lands -- before it lands, we have to be careful, and maybe manually running the test here is desirable)

[List of other uplifts needed for the feature/fix]: N/A

[Is the change risky?]: [Why is the change risky/not risky?]: No -- comment-numbering changes aside, this is just adding an early exit in case of error, and that early exit is dead simple.

[String changes made/needed]: N/A
Attachment #8817104 - Attachment is obsolete: false
Attachment #8817104 - Flags: approval-mozilla-beta?
Attachment #8817104 - Flags: approval-mozilla-aurora?
Comment on attachment 8819430 [details] [diff] [review]
ESR45 backport

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-critical
User impact if declined: pwn2own Fail v2.0.
Fix Landed on Version: will land this cycle -- just want to minimize exposure window by landing on all branches relatively simultaneously.
Risk to taking this patch (and alternatives if risky): Minimal, see comment 9.
String or UUID changes made by this patch: N/A
Attachment #8819430 - Flags: review+
Attachment #8819430 - Flags: approval-mozilla-esr45?
Created attachment 8819443 [details] [diff] [review]
Spot-fix for beta
Attachment #8817104 - Attachment is obsolete: true
Attachment #8817104 - Flags: approval-mozilla-beta?
Attachment #8817104 - Flags: approval-mozilla-aurora?
Attachment #8819430 - Attachment is obsolete: true
Attachment #8819430 - Flags: approval-mozilla-esr45?
Comment on attachment 8819443 [details] [diff] [review]
Spot-fix for beta

Bah, bzex'd with wrong patch applied.  :-(  Sorry for bugspam.
Attachment #8819443 - Attachment is obsolete: true
Attachment #8817104 - Attachment is obsolete: false
Attachment #8817104 - Flags: approval-mozilla-beta?
Attachment #8817104 - Flags: approval-mozilla-aurora?
Attachment #8819430 - Attachment is obsolete: false
Attachment #8819430 - Flags: approval-mozilla-esr45?
(In reply to Steve Fink [:sfink] [:s:] from comment #5)
> ::: js/src/vm/TypedArrayObject.cpp
> @@ +3008,5 @@
> >              return false;
> >  
> > +        // Steps 4-5, 8-9.
> > +        if (obj->as<TypedArrayObject>().hasDetachedBuffer())
> > +            return result.fail(JSMSG_TYPED_ARRAY_DETACHED);
> 
> I see steps 4-5 (the IsDetachedBuffer check). I do not see 8-9:
> 
>     Let length be O.[[ArrayLength]].
>     If index < 0 or index ≥ length, return false.
> 
> Steps 4-5 seem to be covered earlier, since the only way the length could be
> changed is via detaching. And TypedArrayObject::setElement starts with
> 
>     MOZ_ASSERT(index < obj.length())
> 
> so it seems like we know we're assuming that.

Yes.  We checked the length already, and the only way the length stuff changes/is invalidated, is if the underlying buffer gets detached.  So a detachment check covers steps 4-5 *and* 8-9, as the comment says.
Comment on attachment 8817104 [details] [diff] [review]
Patch

Giving branch approvals for this security issue.
Attachment #8817104 - Flags: approval-mozilla-beta?
Attachment #8817104 - Flags: approval-mozilla-beta+
Attachment #8817104 - Flags: approval-mozilla-aurora?
Attachment #8817104 - Flags: approval-mozilla-aurora+
Attachment #8819430 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
can this land first on inbound/central so we can uplift this
Flags: needinfo?(jwalden+bmo)
https://hg.mozilla.org/mozilla-central/rev/0728c4ae020f
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox53: affected → fixed
Flags: needinfo?(jwalden+bmo) → in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

Comment 17

a year ago
uplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/907b98f8c6fa
https://hg.mozilla.org/releases/mozilla-beta/rev/45f796204c54
https://hg.mozilla.org/releases/mozilla-esr45/rev/122f5fbfc563
status-firefox51: affected → fixed
status-firefox52: affected → fixed
status-firefox-esr45: affected → fixed

Updated

11 months ago
Attachment #8817103 - Flags: review?(sphink) → review+
Flags: qe-verify+
Whiteboard: [post-critsmash-triage]
I reproduced the original issue using the poc from comment#0 and comment#1 using 679118259e91:
* Assertion failure: index < obj.length(), at /home/kjozwiak/mozcode/mozilla-central/js/src/vm/TypedArrayObject.cpp:2163

Once I reproduced the above issue, I went through verification using the test case from comment#0 and comment#1 with the following builds:

* fx53, rev: e68cbc3b5b3d
* fx52, rev: f3f71b2a6293
* fx51, rev: 95d20a617288
* fx45esr, rev: 0521b0e4707c

All three of the above builds produced the following error:

/home/kjozwiak/Desktop/poc1.js:5:13 Error: detachArrayBuffer() requires a single argument
Stack:
  valueOf@/home/kjozwiak/Desktop/poc1.js:5:13
  @/home/kjozwiak/Desktop/poc1.js:2:1

fx45esr produced a slightly different error:

/home/kjozwiak/Desktop/comment0.js:5:13 ReferenceError: detachArrayBuffer is not defined
Stack:
  valueOf@/home/kjozwiak/Desktop/comment0.js:5:13
  @/home/kjozwiak/Desktop/comment0.js:2:1

Jeff, mind taking a quick look at the above errors? I can't reproduce the original assertion with the new builds, but are the above errors expected behaviour?
Flags: needinfo?(jwalden+bmo)
(Reporter)

Comment 19

11 months ago
(In reply to Kamil Jozwiak [:kjozwiak] from comment #18)
> All three of the above builds produced the following error:
> 
> /home/kjozwiak/Desktop/poc1.js:5:13 Error: detachArrayBuffer() requires a
> single argument
> Stack:
>   valueOf@/home/kjozwiak/Desktop/poc1.js:5:13
>   @/home/kjozwiak/Desktop/poc1.js:2:1

Try |detachArrayBuffer(ta.buffer)| without "same-data" as the second argument (the second argument to detachArrayBuffer was removed in bug 1240984).


> 
> fx45esr produced a slightly different error:
> 
> /home/kjozwiak/Desktop/comment0.js:5:13 ReferenceError: detachArrayBuffer is
> not defined
> Stack:
>   valueOf@/home/kjozwiak/Desktop/comment0.js:5:13
>   @/home/kjozwiak/Desktop/comment0.js:2:1
> 

Try |neuter(ta.buffer, "same-data")| for older builds (the function was renamed in bug 1079844).
Flags: needinfo?(jwalden+bmo)
(In reply to André Bargull from comment #19)
> Try |detachArrayBuffer(ta.buffer)| without "same-data" as the second
> argument (the second argument to detachArrayBuffer was removed in bug
> 1240984).

Removing the second argument will display the following error when running through the test case from comment#0 using fx53, fx52 and fx51. Taking a quick look at the patch, it looks like we're returning the correct error message.

/home/kjozwiak/Desktop/testCase1.js:2:1 TypeError: attempting to access detached ArrayBuffer
Stack:
  @/home/kjozwiak/Desktop/testCase1.js:2:1

The test case from comment#1 doesn't display any errors.

> Try |neuter(ta.buffer, "same-data")| for older builds (the function was
> renamed in bug 1079844).

Changing the name of the function as suggested above will display the following error when running through the test case from comment#0 using fx45esr:

/home/kjozwiak/Desktop/asanTestCase1.js:2:1 TypeError: attempting to access detached ArrayBuffer
Stack:
  @/home/kjozwiak/Desktop/asanTestCase1.js:2:1

The test case from comment#1 doesn't display any errors.

Thanks for the help André.
Status: RESOLVED → VERIFIED
status-firefox51: fixed → verified
status-firefox52: fixed → verified
status-firefox53: fixed → verified
status-firefox-esr45: fixed → verified

Updated

11 months ago
Group: javascript-core-security → core-security-release

Updated

11 months ago
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main51+][adv-esr45.7+]
Group: core-security-release
Comment on attachment 8817103 [details] [diff] [review]
Test

This is actually kinda incredi-sad: both tests here (that I was about to land, to clear out an old mq) are sorta buggy.  (They do, however, detect this bug's having been fixed in its worst manifestations.)

The Reflect.set-test *shouldn't* throw, because in current spec the receiver in the [[Set]] operation is ignored when an index is being set.  Our looking at the receiver at all is bug 1413794, I think.

The Object.defineProperty-test is correct as written -- but only because Object.defineProperty interprets the detached-buffer case as a not-succeed case (not as a throw-no-matter-how-invoked case) and chooses to throw because of that.  If I were to s/Object/Reflect/ to not implicate Object.defineProperty's treatment of not-success as throw, rather just returning false, to transparently pass along what [[Set]] does, the test would start failing.

Both issues are covered in test262 versions of these tests, appropriately modified, that I'm upstreaming here:

https://github.com/tc39/test262/pull/1336

Given that, I'm not going to bother landing this tests patch.
Attachment #8817103 - Attachment is obsolete: true
Flags: in-testsuite? → in-testsuite-
(Reporter)

Comment 22

a month ago
(In reply to Jeff Walden [:Waldo] (I'm baaaaaaack...) from comment #21)
> The Object.defineProperty-test is correct as written -- but only because
> Object.defineProperty interprets the detached-buffer case as a not-succeed
> case (not as a throw-no-matter-how-invoked case) and chooses to throw
> because of that.  If I were to s/Object/Reflect/ to not implicate
> Object.defineProperty's treatment of not-success as throw, rather just
> returning false, to transparently pass along what [[Set]] does, the test
> would start failing.

But that looks like a bug in SM, in the spec a TypeError should be thrown for Object.defineProperty and Reflect.defineProperty: 
9.4.5.3 [[DefineOwnProperty]], step 3.b.x.2 calls 9.4.5.9 IntegerIndexedElementSet, and in 9.4.5.9, step 5 a TypeError is thrown.

Hmm, why is this [1] using |result.fail(JSMSG_TYPED_ARRAY_DETACHED)| instead of throwing a TypeError? That looks wrong...

[1] https://searchfox.org/mozilla-central/rev/423b2522c48e1d654e30ffc337164d677f934ec3/js/src/vm/TypedArrayObject.cpp#2275-2276
You need to log in before you can comment on or make changes to this bug.