1080312 - Crash in DataChannels/sctp timer loop when far-end is being debugged with GDB

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Description

[Randell Jesup [:jesup] (needinfo me)]

When debugging DataChannels, if one side is stalled (in gdb) for a long time, the other side may crash in a sctp-timer-processing routing accessing freed memory.

Crashes in user_sctp_timer_iterate.c:81:
if (c->c_time <= ticks)
with c = 0xa5a5a5a5a5a5a5a5

Suspicion is that in processing one timer (and it drops to lock to process the timer) it freed another item on the list.  Michael Tuexen thinks he knows the actual cause.  Several people have seen this (and I'm fairly sure I've hit it before but never got a useful stack from it).

sec-crit because it uses the freed-memory-pointer to load a function pointer and call it.

Likely affects multiple releases as we haven't changed the SCTP library recently.

Comment 1

[Loic]

For the record, CR with FF34:
https://crash-stats.mozilla.com/report/index/e6def404-4f1b-43bc-b4d0-301192141008

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

This code hasn't changed pretty much since initial landing.

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

There's a fix in upstream (likely should update the library on m-c), and I can cherry-pick the fix for uplifts.

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

Sounds good.

Comment 5

[Andrew McCreight [:mccr8]]

(In reply to Randell Jesup [:jesup] from comment #3)
> There's a fix in upstream (likely should update the library on m-c), and I
> can cherry-pick the fix for uplifts.

Has this upstream fix been uplifted to our repo yet?  We're nearing the end of a cycle so that would be good to have.

Comment 6

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

Michael, any updates on this before our cycle ends?

Comment 7

[Michael Tüxen]

Not sure what you are asking for. I fixed the bug upstream. Integrating that fix
is normally done by Randell.

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

Al: sorry, I wanted to build a more-upliftable patch than a full library import.  I started working on that again last night when I realized it had fallen off the radar.  I hope to land one today (and import a new lib to 36)

Comment 9

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

Michael, the bug is assigned to you, which is why I was asking you the question. We saw this during critsmash and I assumed the assignee was the person to ask for status.

Randell, thanks.

Comment 10

[Michael Tüxen]

Ahh, OK. So maybe the assignment is suboptimal...

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

Attachment: update iteration code from upstream

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8521747 [details] [diff] [review]
update iteration code from upstream

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

imported rev 9038 from upstream, updated our moz.build file to match and remove the user_sctp_timer_iterate.c
Imported as a single patch, it applies cleanly

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8521747 [details] [diff] [review]
update iteration code from upstream

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Very unlikely

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 older supported branches are affected by this flaw? Pretty much all

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?  Should apply directly

How likely is this patch to cause regressions; how much testing does it need? fairly unlikely to regress, as this makes it use the existing Mac kernel timer loop, and this upstream patch is basically a clean import.

I've done local testing where I freeze one side using the testcase in See Also here and two browsers.  no problems in multiple tests.

Comment 14

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

Comment on attachment 8521747 [details] [diff] [review]
update iteration code from upstream

Many approvals given! Go!

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

Actually: Tuexen wrote the upstream patch; I imported it, so I really should be the reviewer.

Comment 16

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/153148514f22

https://tbpl.mozilla.org/?tree=Try&rev=191b906741a0

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/199744bc7044
https://hg.mozilla.org/releases/mozilla-beta/rev/4bb1c6116c39

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/c12120e10be6
https://hg.mozilla.org/releases/mozilla-esr31/rev/65eb5e688209
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/1eff6446d7fd

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/153148514f22

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/4bb1c6116c39
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/c12120e10be6

Comment 22

[Kamil Jozwiak [:kjozwiak]]

Going to mark this as qe‑verify- as this was tested via bug # 1099388 comment # 7. I tried reproducing the issue with the test case from bug # 1099388 using the instructions from bug # 1099388 comment # 4 without any luck. I let the test case run for more than 2 hours on my asan build without any crashes.

Using the information from comment #0, I wasn't sure how to proceed with testing as the issue was discovered while debugging DataChannels.

If there's anything else that QE can do here, please let me know and I'll gladly take another look!