TimerThread::Init can race against TimerThread::Run, and can call TimerEvent::Init more than once

RESOLVED FIXED in Firefox 54

Status

()

RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: bwc, Assigned: bwc)

Tracking

({csectype-race, sec-moderate})

54 Branch
mozilla55
csectype-race, sec-moderate
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox53 wontfix, firefox54 fixed, firefox55 fixed)

Details

(Whiteboard: [adv-main54+])

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
I noticed this problem while looking at this crash:

https://crash-stats.mozilla.com/report/index/3befdff1-702c-48b7-a2a8-3033a0170427#allthreads

Here we see TimerThread::Run beating TimerThread::Init to the lock. There's quite a racy window of opportunity around TimerThread::mInitialized; it is checked without locking, and set well after TimerEvent::Init is called, which means TimerEvent::Init could be called more than once (not sure the consequences of this).

I'm unsure why we aren't locking for the entirety of TimerThread::Init. I'll have to try it out.
(Assignee)

Comment 1

2 years ago
Ok, I've simplified TimerThread::Init considerably, and changed who is responsible for calling it so we can piggyback on a lock we were already doing, so it is more efficient/encapsulated too. Let's see how it does...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2acc41de9be7f5d3e9277a002da4504525e7e125
Assignee: nobody → docfaraday
Group: core-security → dom-core-security
Keywords: csectype-race, sec-moderate
Comment on attachment 8864911 [details] [diff] [review]
Simplify TimerThread::Init some

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

::: xpcom/threads/TimerThread.cpp
@@ +551,5 @@
>  
> +  nsresult rv = Init();
> +  if (NS_FAILED(rv)) {
> +    return rv;
> +  }

Do we need to add this same sort of code in RemoveTimer as well?  Theoretically, it shouldn't matter...?
Attachment #8864911 - Flags: review?(nfroyd) → review+
(Assignee)

Comment 4

2 years ago
(In reply to Nathan Froyd [:froydnj] from comment #3)
> Comment on attachment 8864911 [details] [diff] [review]
> Simplify TimerThread::Init some
> 
> Review of attachment 8864911 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: xpcom/threads/TimerThread.cpp
> @@ +551,5 @@
> >  
> > +  nsresult rv = Init();
> > +  if (NS_FAILED(rv)) {
> > +    return rv;
> > +  }
> 
> Do we need to add this same sort of code in RemoveTimer as well? 
> Theoretically, it shouldn't matter...?

Well, the init is just to start the thread up, so the added timer will actually pop. I don't think it is necessary to do this for removeTimer, and we weren't doing it before.
https://hg.mozilla.org/mozilla-central/rev/0f9d66b6a6d9
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Group: dom-core-security → core-security-release
Is this wontfix for older versions?
Flags: needinfo?(docfaraday)
(Assignee)

Comment 8

2 years ago
Let me see how easy it is to uplift this. It is a low-enough risk change that we should be able to uplift safely.
(Assignee)

Updated

2 years ago
status-firefox53: --- → wontfix
status-firefox54: --- → affected
Flags: needinfo?(docfaraday)
(Assignee)

Comment 9

2 years ago
Comment on attachment 8864911 [details] [diff] [review]
Simplify TimerThread::Init some

Approval Request Comment
[Feature/Bug causing the regression]:

   Present for a _long_ time.

[User impact if declined]:

   Possible crashes on startup, might be exploitable.

[Is this code covered by automated tests?]:

   Yeah, but it is a rare crash.

[Has the fix been verified in Nightly?]:

   Yes.

[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?]:

   Not very, it is pretty straightforward.

[String changes made/needed]:

   None.
Attachment #8864911 - Flags: approval-mozilla-beta?
(Assignee)

Updated

2 years ago
status-firefox-esr52: --- → wontfix
Comment on attachment 8864911 [details] [diff] [review]
Simplify TimerThread::Init some

Fix a sec-moderate. Beta54+. Should be in 54 beta 8.
Attachment #8864911 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Byron Campen [:bwc] from comment #9)
> [Is this code covered by automated tests?]:
> 
>    Yeah, but it is a rare crash.
> 
> [Has the fix been verified in Nightly?]:
> 
>    Yes.
> 
> [Needs manual test from QE? If yes, steps to reproduce]: 
> 
>    No.

Setting qe-verify- based on Byron's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
Whiteboard: [adv-main54+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.