1979072 - Crash in [@ mozilla::DataChannel::AddRef]

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

35 crash reports from 33 installs of Firefox 141.0 on various operating systems. Address points to use-after-free in the content process, often after longer uptime (15+ minutes)

Crash report: https://crash-stats.mozilla.org/report/index/bce516b4-0538-40da-9452-8d0ae0250724

Reason:

EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames:

0  xul.dll  std::_Atomic_integral<unsigned long long, 8>::fetch_add(const unsigned long l...  /builds/worker/fetches/vs/VC/Tools/MSVC/14.39.33519/include/atomic:1619
0  xul.dll  mozilla::ThreadSafeAutoRefCnt::operator++()  xpcom/base/nsISupportsImpl.h:388
0  xul.dll  mozilla::DataChannel::AddRef()  netwerk/sctp/datachannel/DataChannel.h:400
0  xul.dll  mozilla::RefPtrTraits<mozilla::DataChannel>::AddRef(mozilla::DataChannel*)  mfbt/RefPtr.h:48
0  xul.dll  RefPtr<mozilla::DataChannel>::ConstRemovingRefPtrTraits<mozilla::DataChannel>...  mfbt/RefPtr.h:408
0  xul.dll  RefPtr<mozilla::DataChannel>::RefPtr(mozilla::DataChannel*)  mfbt/RefPtr.h:108
0  xul.dll  mozilla::DataChannelConnection::FinishClose_s(mozilla::DataChannel*)  netwerk/sctp/datachannel/DataChannel.cpp:1322
1  xul.dll  mozilla::DataChannelConnection::CloseAll::<lambda_12>::operator()() const  netwerk/sctp/datachannel/DataChannel.cpp:1376
1  xul.dll  mozilla::detail::RunnableFunction<`lambda at /builds/worker/checkouts/gecko/n...  xpcom/threads/nsThreadUtils.h:548
2  xul.dll  nsThread::ProcessNextEvent(bool, bool*)  xpcom/threads/nsThread.cpp:1153

Comment 1

[Byron Campen [:bwc]]

I think I see a possibility here. I'll roll a fix into some work I'm already doing in this area.

Comment 2

[Andrew McCreight [:mccr8]]

Nice find.

Comment 3

[Byron Campen [:bwc]]

Bug 1979329 confirms my suspicion.

Comment 4

[Byron Campen [:bwc]]

Attachment: (secure)

Comment 5

[Byron Campen [:bwc]]

Comment on attachment 9503645 [details]
(secure)

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Depends on how closely they read it. It probably would not be particularly hard to trigger the crash.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?: beta and release
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?:
• How likely is this patch to cause regressions; how much testing does it need?: Very unlikely
• Is the patch ready to land after security approval is given?: Yes
• Is Android affected?: Yes

Comment 6

[Tom Ritter [:tjr]]

Comment on attachment 9503645 [details]
(secure)

Approved to land and request uplift

Comment 7

[Byron Campen [:bwc]]

Comment on attachment 9503645 [details]
(secure)

Beta/Release Uplift Approval Request

• User impact if declined/Reason for urgency: Sec bug
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Very simple and safe.
• String changes made/needed: None
• Is Android affected?: Yes

Comment 8

[Byron Campen [:bwc]]

I isolated this patch from bug 1974443 as a possible fix; not 100% sure this was the cause, but it makes sense.

Comment 9

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/5d6fa04d1cad
https://hg.mozilla.org/releases/mozilla-beta/rev/d04567ce551d

Comment 10

[Byron Campen [:bwc]]

(In reply to Byron Campen [:bwc] from comment #8)

I isolated this patch from bug 1974443 as a possible fix; not 100% sure this was the cause, but it makes sense.

The signature has definitely stopped showing up in crash-stats. I do not see the signatures for bugs 1979329 and 1979081 either. Looks like that was the fix.

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

Copying crash signatures from duplicate bugs.

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

Copying crash signatures from duplicate bugs.