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)

defect
Not set
critical

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)

The following test case asserts with:

Assertion failure: wrapper->is<WrapperObject>(), at js/src/jswrapper.cpp:56

---
JSON.stringify(new Proxy(new Boolean(false),{}))
---
This sounds bad...
Group: core-security
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
Setting needinfo now that till's CCed because Bugzilla is slightly dumb and won't let me do both at once...
Flags: needinfo?(till)
Attached patch Simple fix (obsolete) — Splinter Review
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: general → till
Status: NEW → ASSIGNED
Which branches do we need this on?
All up to beta, sadly. Waiting for an r+ before requesting the uplift approvals.
Flags: needinfo?(till)
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)
(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 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+
Attached patch Simple fix (obsolete) — Splinter Review
Attachment #788093 - Attachment is obsolete: true
Attached patch Simple fix.Splinter Review
Hrmpf, forgot to remove the assert
Attachment #788570 - Attachment is obsolete: true
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?
Attached patch Fix for betaSplinter Review
[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?
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
Flags: in-testsuite?
Attachment #788572 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #788577 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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.
Whiteboard: [adv-main24+]
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.
Whiteboard: [adv-main24+] → [adv-main24-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.