Closed Bug 1452417 Opened 3 years ago Closed 3 years ago
Crash in ns
COMPtr<T>::ns COMPtr<T> | ns Host Resolver::Detach Callback
This bug was filed from the Socorro interface and is report bp-af95af75-2515-42fb-838b-c403a0180401. ============================================================= Top 10 frames of crashing thread: 0 xul.dll nsCOMPtr<nsIMutationObserver>::nsCOMPtr<nsIMutationObserver> mfbt/RefPtr.h:112 1 xul.dll nsHostResolver::DetachCallback netwerk/dns/nsHostResolver.cpp:1007 2 xul.dll nsDNSAsyncRequest::Cancel netwerk/dns/nsDNSService2.cpp:398 3 xul.dll mozilla::net::PACResolver::Notify netwerk/base/ProxyAutoConfig.cpp:311 4 xul.dll nsTimerImpl::Fire xpcom/threads/nsTimerImpl.cpp:704 5 xul.dll nsTimerEvent::Run xpcom/threads/TimerThread.cpp:290 6 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1096 7 xul.dll NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:519 8 xul.dll nsThread::Shutdown xpcom/threads/nsThread.cpp:814 9 xul.dll mozilla::net::WaitForThreadShutdown::Run netwerk/base/nsPACMan.cpp:160 ============================================================= this signature is newly showing up on windows since firefox 59 - probably related to bug 1424834 and just a signature change. it's a uaf in some instances though...
Valentin, do you have cycles to look at this?
Assignee: nobody → valentin.gosu
Priority: -- → P1
It seems that either PACResolver or nsDNSAsyncRequest is being released on another thread, while we call Cancel on the main thread. I think the solution here is to hold a ref to mRequest in PACResolver::Notify()
Attachment #8966717 - Flags: review?(daniel)
Attachment #8966717 - Flags: review?(daniel) → review+
Comment on attachment 8966717 [details] [diff] [review] Hold a ref to mRequest in PACResolver::Notify [Security approval request comment] How easily could an exploit be constructed based on the patch? Difficult. The race causing this issue would be quite hard for an attacker to trigger. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Kind of. The problem is obvious if you look at the code. Which older supported branches are affected by this flaw? Firefox 59 according to comment 0 If not all supported branches, which bug introduced the flaw? Unknown. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Easy to backport. How likely is this patch to cause regressions; how much testing does it need? Unlikely to cause regressions.
Clearing sec-approval as it doesn't need it since we made this a sec-moderate. That said, I'd like to see a Beta patch nominated so we can have this fixed for ESR60 as well.
Comment on attachment 8966717 [details] [diff] [review] Hold a ref to mRequest in PACResolver::Notify Approval Request Comment [Feature/Bug causing the regression]: According to comment 0 the first crashes started in Firefox 59 [User impact if declined]: Crashes may occur, most likely during shutdown. [Is this code covered by automated tests?]: No. [Has the fix been verified in Nightly?]: Just landed on Nightly. Waiting for confirmation that the crashes have stopped. [Needs manual test from QE? If yes, steps to reproduce]: No. [List of other uplifts needed for the feature/fix]: None. [Is the change risky?]: No. [Why is the change risky/not risky?]: The patch just adds an extra AddRef. [String changes made/needed]: None.
Attachment #8966717 - Flags: approval-mozilla-beta?
Attachment #8966717 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Whiteboard: [necko-triaged] → [necko-triaged][adv-main60+]
Whiteboard: [necko-triaged][adv-main60+] → [necko-triaged][adv-main60+][post-critsmash-triage]
You need to log in before you can comment on or make changes to this bug.