1573160 - use-after-free possibility in nsThreadPool

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Nathan Froyd [:froydnj]]

While reviewing some other code, I think we have a latent use-after-free possibility in nsThreadPool. The sequence of events is something like:

T1. Dispatch an event into the thread pool.
T1. Discover that we need to spawn a new thread.
T1. Spawn a new thread.

Now, we get interrupted here:

https://searchfox.org/mozilla-central/source/xpcom/threads/nsThreadPool.cpp#117

T2. Call Shutdown() on the thread pool.
T2. Executes bits of shutdown, specifically:

https://searchfox.org/mozilla-central/source/xpcom/threads/nsThreadPool.cpp#333-347

So the thread pool is marked as shutting down, and we've completely cleared out the list of threads...except that we have one pending thread that might be added to our thread list.

T2. Start shutting down all the threads in the thread pool.
T1. Resumes, discovers that we have 0 threads in the pool, so adds itself to the list of threads.
T1. Dispatches an event into the thread pool.
T2. Releases the last reference to the thread pool.
T2. Thread pool is freed.
...new thread is scheduled to run...
NT. Tries to touch things in the now-freed thread pool object.
NT. Boom.

Can somebody check that logic?

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: Bug 1573160 - shutdown newly-spawned threads if we're shutting down; r=mccr8

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 9084692 [details]
Bug 1573160 - shutdown newly-spawned threads if we're shutting down; r=mccr8

Security Approval Request

• How easily could an exploit be constructed based on the patch?: You'd have to force thread pool construction and then time thread pool shutdown with spawning another thread. The timing issues don't feel very easy, but exploit authors gonna exploit.
• 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?: everything
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: The patch should cleanly backport; it is straightforward to create branch-specific patches if need be.
• How likely is this patch to cause regressions; how much testing does it need?: Not likely.

Comment 3

[Nathan Froyd [:froydnj]]

Asking for sec-approval since I suppose this bug is probably sec-high given the UAF possibilities.

Comment 4

[Andrew McCreight [:mccr8]]

I'll just mark it sec-high, then.

Comment 5

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

Sec-approval+ for mozilla-central and I've added tracking flags.

I'd like to get Beta, ESR68, and ESR60 patches nominated if possible so we can land them after this is on trunk.

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 9084692 [details]
Bug 1573160 - shutdown newly-spawned threads if we're shutting down; r=mccr8

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: Users subject to more exploits than normal.
• Fix Landed on Version: 70
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Low risk: this is well understood code, and the change in behavior is small.
• String or UUID changes made by this patch: None

Beta/Release Uplift Approval Request

• User impact if declined: Users subject to more exploits than normal.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• 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): Low risk: this is well understood code, and the change in behavior is small.
• String changes made/needed: None

Comment 7

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

https://hg.mozilla.org/integration/autoland/rev/486c1d9d10912074867ea4b255107007224bedf7
https://hg.mozilla.org/mozilla-central/rev/486c1d9d1091

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9084692 [details]
Bug 1573160 - shutdown newly-spawned threads if we're shutting down; r=mccr8

Fixes a possible UAF. Approved for 69.0b15, 68.1esr, and 60.9esr.

Comment 9

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

https://hg.mozilla.org/releases/mozilla-beta/rev/cbf4da0320f6
https://hg.mozilla.org/releases/mozilla-esr68/rev/9be2439e4f50
https://hg.mozilla.org/releases/mozilla-esr60/rev/38189f281389