1204669 - nsXBLService::GetBinding is still crashing

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

See https://bugzilla.mozilla.org/show_bug.cgi?id=1202844#c18

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v1

I think we can have deleted baseProto if it is from some different XBL document 
and we've cleared protocache or something.
We should keep all the relevant XBLDocumentInfos alive (strong mBaseXBLDocInfo in nsXBLPrototypeBinding?), but that is _a_lot_ more riskier fix than this one
which is basically just trying to make null checks always valid by using WeakPtr and not foo*

The change to WeakPtr is to make it possible to construct/assign it with nullptr.
The current setup there is totally broken and I'm surprised we haven't seen crashes in code using WeakPtr.

-    protoBinding = docInfo->GetPrototypeBinding(ref);
is about backing out https://hg.mozilla.org/releases/mozilla-esr38/rev/9665a2406b3c#l1.82
(Bug 1202844) since by using WeakPtr it shouldn't be needed anymore.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8663636 [details] [diff] [review]
v1

r=me, but you may want to get review from whoever owns WeakPtr.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8663636 [details] [diff] [review]
v1

Waldo, review request for the WeakPtr change

Comment 4

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking since this is sec-critical.

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 8663636 [details] [diff] [review]
v1

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

::: mfbt/WeakPtr.h
@@ +173,5 @@
>    WeakPtr& operator=(T* aOther)
>    {
> +    if (aOther) {
> +      *this = aOther->SelfReferencingWeakPtr();
> +    } else if (!mRef || mRef->get()) {

Unless I'm misreading, I don't believe it's ever the case that !mRef.  WeakPtr is constructed with this field allocated (and we're presuming it's infallible, bleh).  It's only ever assigned a |new| expression in another place, another WeakPtr::mRef that's non-null inductively, and |explicit WeakPtr(const RefPtr<WeakReference>& aOther) : mRef(aOther) {}| looks completely unused to me.

So remove the !mRef here, and please remove that extra, unused constructor while you're at it.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #5)
> Unless I'm misreading, I don't believe it's ever the case that !mRef. 
There is, when the method is called from ctor(T*)

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

(and would like to keep the changes minimum since this needs to land on branches, so no random code removals)

Comment 8

[Jeff Walden [:Waldo]]

(In reply to Olli Pettay [:smaug] from comment #6)
> There is, when the method is called from ctor(T*)

Oh, bah.  Okay.  On trunk at some point, I think I'd rather see the constructors initializing members and not having not-initialized cases to deal with in non-constructor methods.  But for branch this is fine.  (As is minimizing changes, of course.)

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8663636 [details] [diff] [review]
v1

Approval Request Comment
[Feature/regressing bug #]: This is _old_ stuff.
[User impact if declined]: crashes
[Describe test coverage new/current, TreeHerder]: NA. This is _still_ speculative fix. No one has managed to reproduce the crash. But after bug 1202844 it became more clear what the issue could be.
[Risks and why]: Should be safe, given that we make raw pointers to weak pointers. So, no touching to deleted objects anymore.
[String/UUID change made/needed]: NA

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
I haven't figured out how to crash here. The fix is speculative based on the stack traces and code.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Comment could be (indicating one part of the fix):
-u "Olli Pettay <[e-mail]>" -m "Bug 1204669 optimize out hashtable lookups caused by extra GetPrototypeBinding call, r=bz,waldo"
	
Which older supported branches are affected by this flaw?
All

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should apply easily to all branches. Just tested esr38 and applies there without any fuzz.

How likely is this patch to cause regressions; how much testing does it need?
Should be safe.

Comment 10

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8663636 [details] [diff] [review]
v1

Approvals given.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/91657db26f49

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/91657db26f49

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1c9058b0ab32

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d7c8fdff5210

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Given that this is a speculative fix and not an easy exploit, I don't feel the need to take this fix in 41, is now a wontfix.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

So far I haven't found any 42.0b2 crashes, but there were crashes still in b1. The patch should be in b2. So hopefully this ancient bug (see also Bug 720991) is now fixed. But I'll keep looking at the crash stats.

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-esr38/rev/f36e6394b07f