1320771 - Crash in MessageLoop::PostTask_Helper (no Vsync involvement)

Status: RESOLVED WORKSFORME

(Core :: IPC, defect, P1)

Description

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

+++ This bug was initially created as a clone of Bug #1283019 +++

Lots of UAF signatures in PostTask().  Note, this is spun off as it doesn't involve vsync.

The UAF signature for this is much stronger than for ones with Vsync in the stack, though some of those are UAFs also

Comment 1

[Frederik Braun [:freddy]]

Adding David [:dvander], because did some analysis in bug 1283019.

Comment 2

[Frederik Braun [:freddy]]

Please excuse the double-bugspam.
But I wanted to make sure this is on your radar, David, so we get the number of security bugs down :)

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

I don't really have anything to add here.

Comment 4

[Andrew McCreight [:mccr8]]

I took a look at the crashes with a particular poison value, and all 67 of these crashes were happening in xpcom-shutdown, so this might be some bug in tearing down IPC stuff.

Comment 5

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

(In reply to Andrew McCreight [:mccr8] from comment #4)
> I took a look at the crashes with a particular poison value, and all 67 of
> these crashes were happening in xpcom-shutdown, so this might be some bug in
> tearing down IPC stuff.

So this actually makes me worry more.  The failures are in the same location (looking at the 51 crashes), and the non-shutdown ones all have random real-ish values in them, not poison.  This smells strongly like reallocated memory, and in shutdown memory pressure is vastly less, and so it stays unallocated (and poisoned).

I doubt do_GetMainThread() has a problem.  And GetXPCOMThread will only indirect off 'this' aka the pump to read mThread.  In theory the pumps should be safe since they're stored in RefPtr<>s, so either something is whomping things, or a pump isn't refcounting-threadsafe in some manner, or someone played games with the refcounts/objects and freed one directly (delete pump_ or equivalent)

Bill, any ideas?

Comment 6

[Gabor Krizsanits (INACTIVE)]

For the record, crashes seemed to stop on 54 (last on Feb 4th) and only a few days later on 53 too (last on Feb 9th), so it seems to be fixed and the fix uplifted...

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: assertion patch

I would like to land some assertions to figure out what might be happening. This patch asserts if you shut down a MessageLoop (i.e., a thread) when there's still a MessageChannel bound to it.

I tested the patch by removing the Close() call from the ProcessHangMonitor code. We crash on shutdown, as desired.

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: assertion patch v2

Here's the right patch. Sorry.

Comment 9

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

Can we request sec-approval and land this?  does it make sense to uplift this diagnostic patch?  (given we seem to have fixed it on 54 and 53 now).  also please mark the "affecting" as appropriate

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I moved the patch to bug 1349699 since it's not sensitive.

Comment 11

[Frederik Braun [:freddy]]

Looking at bug 1349699 comment 20 to 21 it seems like uplifting is not trivial :/
In essence, we depend on bug 1356365, which is active.

Comment 12

[Frederik Braun [:freddy]]

(In reply to Bill McCloskey (:billm) from comment #10)
> I moved the patch to bug 1349699 since it's not sensitive.


The bug is marked as fixed, but the crash rate has not declined. Can you elaborate what's going on?

Comment 13

[Daniel Veditz [:dveditz]]

(In reply to Frederik Braun [:freddyb] from comment #12)
> (In reply to Bill McCloskey (:billm) from comment #10)
> > I moved the patch to bug 1349699 since it's not sensitive.
> 
> [That] bug is marked as fixed,

That's a diagnostic assert, not a "fix".

> but the crash rate has not declined. Can you elaborate what's going on?

It has, a little. Large jump in March because WinXP folks were moved to the ESR branch that doesn't do throttling, but a steady decline since then (minus huge spike when 53.0 released--do some people get an unrecoverable startup crash and give up?). Still high though: https://ashughes1.github.io/bugzilla-socorro-lens/chart.htm?s=MessageLoop::PostTask_Helper

Signatures contain a rather large number of Mac users crashing in 48.0.2 (and some on 47.x) on a null deref. The UAF signatures are all Windows machines with the bulk being Win7 (despite the lack of throttling of WinXP/Vista crashes). Those crashes continue through 53.0.3 and 54.0b9 -- I think Randell was wrong in comment 9. 

I don't see any crashes on Nightly (55) but we might not have enough Windows users on Nightly, or just not enough users to have hit whatever conditions trigger this crash. Or maybe it really is fixed, but if so that won't help us identify a fix to back-port to ESR 52

Comment 14

[Andrew McCreight [:mccr8]]

Bug 1349699 has been turning up crashes, so maybe that will help once fixes for those get out to branches.

Comment 15

[Stephanie Ouillon [:arroway] (needinfo me)]

The crash rate for for this signature on Fennec has significatively risen over the past weeks, any idea what could be the reason?

Comment 16

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I think the Fennec problem has been fixed, as has the IPC problem. I'm going to close this. It doesn't make any sense to have a security sesnsitive bug without any actionable information in it, especially given how generic a signature this is.