Closed
Bug 903180
Opened 11 years ago
Closed 11 years ago
Assertion failure: wrapper->is<WrapperObject>(), at js/src/jswrapper.cpp:56
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla26
Tracking | Status | |
---|---|---|
firefox23 | --- | unaffected |
firefox24 | + | fixed |
firefox25 | + | fixed |
firefox26 | + | fixed |
firefox-esr17 | --- | unaffected |
b2g18 | --- | unaffected |
People
(Reporter: anba, Assigned: till)
References
Details
(Keywords: regression, sec-high, Whiteboard: [adv-main24-])
Attachments
(2 files, 2 obsolete files)
913 bytes,
patch
|
till
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
785 bytes,
patch
|
till
:
review+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
The following test case asserts with: Assertion failure: wrapper->is<WrapperObject>(), at js/src/jswrapper.cpp:56 --- JSON.stringify(new Proxy(new Boolean(false),{})) ---
Comment 2•11 years ago
|
||
I'm betting till introduced this when he did Boolean refactoring to this stuff in bug 882468. *Not* setting the dependency as I suspect knowledge that there's a security bug depending on this would be enough to tell a semi-aware baddie how to trigger it. (Although, whether there's an actual problem deriving from this, I dunno yet.)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 3•11 years ago
|
||
Setting needinfo now that till's CCed because Bugzilla is slightly dumb and won't let me do both at once...
Flags: needinfo?(till)
Assignee | ||
Comment 4•11 years ago
|
||
It was supposed to be so easy. Also, I swear I tested this very scenario (but only ad hoc, not with anything in the test suite, *sigh*). Hence, I'm fairly, though not absolutely, convinced that this regressed via bug 887558, which landed just a few days later. In any case, the attached one-line fix makes much more sense.
Attachment #788093 -
Flags: review?(jwalden+bmo)
Assignee | ||
Updated•11 years ago
|
Assignee: general → till
Status: NEW → ASSIGNED
Comment 5•11 years ago
|
||
Which branches do we need this on?
Assignee | ||
Comment 6•11 years ago
|
||
All up to beta, sadly. Waiting for an r+ before requesting the uplift approvals.
Flags: needinfo?(till)
Updated•11 years ago
|
tracking-firefox24:
--- → ?
tracking-firefox25:
--- → ?
tracking-firefox26:
--- → ?
Keywords: regression
Comment 7•11 years ago
|
||
Per comment #4, if bug 887558 is the regression which landed in Fx25, unclear why we would need to track this on Firefox 24 ?
Flags: needinfo?(till)
Assignee | ||
Comment 8•11 years ago
|
||
(In reply to bhavana bajaj [:bajaj] (on vacation until 08/14/2013) from comment #7) > Per comment #4, if bug 887558 is the regression which landed in Fx25, > unclear why we would need to track this on Firefox 24 ? Because bug 882468 is the change that did, in fact, introduce an arguably wrong change, here. I'm almost entirely certain that the change in bug 887558 is what causes it to have visible symptoms, but nevertheless, it's clearly wrong even without that - and a very simple change. Backporting it to the 24 branch requires a slight change, but that still is very much straight-forward.
Flags: needinfo?(till)
Comment 9•11 years ago
|
||
Comment on attachment 788093 [details] [diff] [review] Simple fix Review of attachment 788093 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsbool.cpp @@ +197,5 @@ > */ > bool > js::BooleanGetPrimitiveValueSlow(HandleObject wrappedBool, JSContext *cx) > { > JS_ASSERT(wrappedBool->is<ProxyObject>()); No need for the assertion any more.
Attachment #788093 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Comment 10•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #788093 -
Attachment is obsolete: true
Assignee | ||
Comment 11•11 years ago
|
||
Hrmpf, forgot to remove the assert
Assignee | ||
Updated•11 years ago
|
Attachment #788570 -
Attachment is obsolete: true
Assignee | ||
Comment 12•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/64afa50a8042
Assignee | ||
Comment 13•11 years ago
|
||
Comment on attachment 788572 [details] [diff] [review] Simple fix. [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 882468, probably in combination with bug 887558 User impact if declined: execution of unexpected code, potentially exploitable Testing completed (on m-c, etc.): no, just landed on inbound. I'm on pto next week, though, so pre-emptively requesting approval assuming it sticks. (which it should) Risk to taking this patch (and alternatives if risky): very low String or IDL/UUID changes made by this patch: none
Attachment #788572 -
Flags: review+
Attachment #788572 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 14•11 years ago
|
||
[Approval Request Comment] Bug caused by (feature/regressing bug #): bug 882468 User impact if declined: execution of unexpected code, potentially exploitable Testing completed (on m-c, etc.): no, just landed on inbound. I'm on pto next week, though, so pre-emptively requesting approval assuming it sticks. (which it should) Risk to taking this patch (and alternatives if risky): very low String or IDL/UUID changes made by this patch: none Adapted to beta, verified that it fixes the issue there, too. (And that a fix was required, so bug 887558 isn't part of the cause, after all.
Attachment #788577 -
Flags: review+
Attachment #788577 -
Flags: approval-mozilla-beta?
Comment 15•11 years ago
|
||
might be worth also a testcase or ? :) fixed in mozilla-central https://hg.mozilla.org/mozilla-central/rev/64afa50a8042
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Updated•11 years ago
|
Flags: in-testsuite?
Updated•11 years ago
|
Updated•11 years ago
|
Updated•11 years ago
|
Attachment #788572 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•11 years ago
|
Attachment #788577 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 16•11 years ago
|
||
(In reply to Till Schneidereit [:till] (Mountain hiking, back on Friday) from comment #12) > https://hg.mozilla.org/integration/mozilla-inbound/rev/64afa50a8042 Uh, you're not supposed to push security patches anywhere without sec-approval, the idea being that we can think about scheduling of exposing the fact of an issue, wrt releases that fix it. See here: https://wiki.mozilla.org/Security/Bug_Approval_Process Too late to do anything this time, here, but keep it in mind in the future.
Comment 17•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/d303977a2b43 https://hg.mozilla.org/releases/mozilla-beta/rev/027478bc86e2
Updated•11 years ago
|
Whiteboard: [adv-main24+]
Comment 18•11 years ago
|
||
Going ahead and adding the dependency we decided not to do in comment 2: the fix has landed and will get pushed to beta users soon enough.
Blocks: 882468
status-b2g18:
--- → unaffected
status-firefox23:
--- → unaffected
status-firefox-esr17:
--- → unaffected
Keywords: sec-high
Updated•11 years ago
|
Whiteboard: [adv-main24+] → [adv-main24-]
Updated•10 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•