Closed Bug 1202844 Opened 6 years ago Closed 6 years ago
crash in ns
Based on https://bugzilla.mozilla.org/show_bug.cgi?id=1197321#c5 I think baseBindingURI (which is nIURI*) is deleted object in nsXBLService::GetBinding. If we get nested GetBinding calls, LoadBindingDocumentInfo may spin event loop, as far as I see, and that may do, well, whatever. See Bug 1197321
Kairo, does the crash happen often also on Nightlies?
protoBinding = docInfo->GetPrototypeBinding(ref); is a bit annoying, but protobindings aren't refcounted, so in theory the object might be dead. I don't understand how this all might happen though. Starting to shutdown while we're still in process to start FF perhaps?
Attachment #8658381 - Flags: review?(bzbarsky)
Comment on attachment 8658381 [details] [diff] [review] pure guess fix > protoBinding = docInfo->GetPrototypeBinding(ref); is a bit annoying, but > protobindings aren't refcounted, so in theory the object might be dead. This is worth a comment... r=me
Attachment #8658381 - Flags: review?(bzbarsky) → review+
Well, I explicitly didn't want to add a comment at this point - to not point so explicitly to the problem. I'll ni? myself to add a comment some time later.
Comment on attachment 8658381 [details] [diff] [review] pure guess fix Approval Request Comment [Feature/regressing bug #]: This is _old_ stuff. [User impact if declined]: crashes [Describe test coverage new/current, TreeHerder]: NA. This is speculative fix. [Risks and why]: Should be safe, given that we ensure keeping certain objects alive [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 what the guess fix probably is about): -u "Olli Pettay <Olli.Pettay@helsinki.fi>" -m "Bug 1202844, make nsXBLService::GetBinding deal with shutting down during binding loading, r=bz" 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.
How bad is the crash here? We need a security rating on this before we can check in.
This is about Bug 119732, which seems to be a topcrasher. The crash is, based on crash-stat, about accessing deleted object. By default that would be sec-critical, but I have no idea how to trigger this crash.
[Tracking Requested - why for this release]:
Comment on attachment 8658381 [details] [diff] [review] pure guess fix sec-approval+ for trunk.
Attachment #8658381 - Flags: sec-approval? → sec-approval+
Comment on attachment 8658381 [details] [diff] [review] pure guess fix sec-critical fix, taking it, Beta41+, Aurora42+
(In reply to Olli Pettay [:smaug] from comment #1) > Kairo, does the crash happen often also on Nightlies? Not in a high frequency. I see 9 crashes on Firefox 43.0a1 in the last week when I look at the link in Crash Signatures of the other bug and expand the (huge) list under "Products" in the Signature Summary.
Comment on attachment 8658381 [details] [diff] [review] pure guess fix sec-critical fix so let's take it for esr too. Though "pure guess fix" sounds a bit unnerving.
Attachment #8658381 - Flags: approval-mozilla-esr38? → approval-mozilla-esr38+
I'm seeing another ::GetBinding crash on crash-stats/Nightly. Not about url object being bogus, but baseBindingURI = baseProto->BindingURI(); So, apparently baseProto isn't valid, but is it protobinding or baseProto. dmajor was very right https://bugzilla.mozilla.org/show_bug.cgi?id=1197321#c8 Investigating and I'll file a followup.
Whiteboard: [adv-main41+][adv-esr38.3+] → [adv-main41+][adv-esr38.3+][post-critsmash-triage]
Based on comment 17, this was fixed in esr38.3.0. Updating the tracking flag to reflect that.
You need to log in before you can comment on or make changes to this bug.