1024765 - Refcounting of nsTimerImpl looks wrong in in TimerThread when PostTimerEvent fails

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Byron Campen [:bwc]]

Noticed this while working on bug 941751. In TimerThread, we call nsTimerImpl::PostTimerEvent (just after doing an addref/release while removing the timer from mTimers), which does the following:

http://dxr.mozilla.org/mozilla-central/source/xpcom/threads/nsTimerImpl.cpp#722

The constructor of nsTimerEvent does not addref (because we are essentially transferring a reference from mTimers to the nsTimerEvent):

http://dxr.mozilla.org/mozilla-central/source/xpcom/threads/nsTimerImpl.cpp#116

It does set itself up to perform a release once it is destroyed, which makes sense:

http://dxr.mozilla.org/mozilla-central/source/xpcom/threads/nsTimerImpl.cpp#157

If we fail for any reason (other than OOM), the nsTimerEvent is destroyed prior to the return, and the refcount is decremented. Then, we decrement the refcount _again_ here, leading to a very misleading logging statement for yours truly, and probably a dangling nsITimer pointer somewhere:

http://dxr.mozilla.org/mozilla-central/source/xpcom/threads/TimerThread.cpp#266

Comment 1

[Byron Campen [:bwc]]

Attachment: Patch that demonstrates the bug.

Comment 2

[Byron Campen [:bwc]]

Debug builds are probably safe here most of the time (since we'll usually hit the assertion), but if that assertion doesn't fire I'm not sure what will happen.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Byron, did you plan to fix this?

Comment 4

[Byron Campen [:bwc]]

I can take a crack at it I suppose.

Comment 5

[Byron Campen [:bwc]]

Ok, I see a few ways that might make sense to do this:

1) Have PostTimerEvent do a brute-force forget().take() on the timer to relinquish the reference when an error is about to be returned.

2) Refactor to have PostTimerEvent() take an already_AddRefed<nsTimerImpl>& inout param that the function may or may not take (if not, the caller will still hold the reference).

3) Make nsresult an out param and return an already_AddRefed containing the timer in the event of failure.


I kinda like option 2.

Comment 6

[Byron Campen [:bwc]]

Attachment: Part 2: Do not take a reference in PostTimerEvent in the case of failure.

Option 1. Seems like the least amount of code and the smallest change, but it could be argued that some refactoring should be done here.

Comment 7

[Byron Campen [:bwc]]

Comment on attachment 8440133 [details] [diff] [review]
Part 2: Do not take a reference in PostTimerEvent in the case of failure.

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

This seems like the most minimal fix, but would you prefer that I rewrite the code in TimerThread to stop using direct refcount manipulation?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8440133 [details] [diff] [review]
Part 2: Do not take a reference in PostTimerEvent in the case of failure.

This looks fine, but I'd just make ForgetTimer() return void and move the take() into it.  And maybe in the comments indicate that it's the destructor of nsTimerEvent that we don't want to decrement the refcount?

Comment 9

[Byron Campen [:bwc]]

(In reply to Boris Zbarsky [:bz] from comment #8)
> Comment on attachment 8440133 [details] [diff] [review]
> Part 2: Do not take a reference in PostTimerEvent in the case of failure.
> 
> This looks fine, but I'd just make ForgetTimer() return void and move the
> take() into it.  And maybe in the comments indicate that it's the destructor
> of nsTimerEvent that we don't want to decrement the refcount?

The reason I had it return already_AddRefed is to make it easier to implement the more explicit options 2 or 3, should we decide to go that route (now, or someday later). Also, the scary (void)event->ForgetTimer().take() makes it more obvious that some funky reference counting stuff is going on, and that the code in |PostTimerEvent| needs to be treated carefully. Just calling ForgetTimer() might not jump out in the same way.

I should point out that I am quite willing to implement option 2 or 3, but I'm not all that familiar with the conventions that are used when implementing an optional reference transfer, so I don't know which fits the rest of the code better (or if there's another variant I haven't thought of). I worry that option 1 leaves |PostTimerEvent| fragile, and also paints a pretty obvious bullseye on the problem, whereas options 2 and 3 leave us in a better place going forward, and just look like code cleanup.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Ah, ok.  In that case, I think the best solution here is actually a combination of 2 and 3: an already_AddRefed in param and an already_AddRefed return value, with the return value null on success, non-null on failure.  The caller ignores the actual return value anyway, so there's no point propagating out a particular nsresult.

Comment 11

[Byron Campen [:bwc]]

Alright, I'll give that a go. Thanks!

Comment 12

[Byron Campen [:bwc]]

Attachment: Part 2: Make refcounting logic around PostTimerEvent more explicit.

An alternative approach.

Comment 13

[Byron Campen [:bwc]]

Attachment: Part 2: Make refcounting logic around PostTimerEvent more explicit.

Restore some ordering.

Comment 14

[Byron Campen [:bwc]]

Attachment: Part 2: Make refcounting logic around PostTimerEvent more explicit.

Remove an outdated comment.

Comment 15

[Byron Campen [:bwc]]

Comment on attachment 8440835 [details] [diff] [review]
Part 2: Make refcounting logic around PostTimerEvent more explicit.

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

::: xpcom/threads/TimerThread.cpp
@@ +247,2 @@
>            RemoveTimerInternal(timer);
> +          timer = nullptr;

Setting this here to avoid use in the code below; should be using timerRef instead.

@@ +267,5 @@
> +            if (timerRef) {
> +              // We got our reference back due to an error.
> +              // Unhook the nsRefPtr, and release manually so we can get the
> +              // refcount.
> +              nsrefcnt rc = timerRef.forget().take()->Release();

Is there a simpler way to do this?

::: xpcom/threads/nsTimerImpl.cpp
@@ +732,2 @@
>  
> +  nsRefPtr<nsTimerEvent> event = new nsTimerEvent;

If we wanted to pass the timer as an already_AddRefed here, we'd need to keep an extra reference around so we can do the stuff below. I can do that if it is preferable, just let me know.

Comment 16

[Byron Campen [:bwc]]

CC'ing maire and jesup because this bug is related to bug 941751 (one of our oranges).

Comment 17

[Byron Campen [:bwc]]

Also CC'ing Nils and Shell by request.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8440835 [details] [diff] [review]
Part 2: Make refcounting logic around PostTimerEvent more explicit.

> Is there a simpler way to do this?

No.  :(

>+nsTimerImpl::PostTimerEvent(already_AddRefed<nsTimerImpl> timerRef)

aTimerRef?

>+  nsRefPtr<nsTimerEvent> event = new nsTimerEvent;

This can't return null.  Does that make it simpler to just go ahead and pass aTimerRef to the constructor here?  We could then use event->Timer() (which would return nsTimerImpl*) for whatever we need the timer for, and event->ForgetTimer() for error returns.  Then we don't need a SetTimer().

r=me

Comment 19

[Byron Campen [:bwc]]

(In reply to Boris Zbarsky [:bz] from comment #18)
> Comment on attachment 8440835 [details] [diff] [review]
> Part 2: Make refcounting logic around PostTimerEvent more explicit.
> 
> >+  nsRefPtr<nsTimerEvent> event = new nsTimerEvent;
> 
> This can't return null.  Does that make it simpler to just go ahead and pass
> aTimerRef to the constructor here?  We could then use event->Timer() (which
> would return nsTimerImpl*) for whatever we need the timer for, and
> event->ForgetTimer() for error returns.  Then we don't need a SetTimer().
> 

   Actually, we override operator new to alloc from a memory pool for this, so it might not be infallible:

http://dxr.mozilla.org/mozilla-central/source/xpcom/threads/nsTimerImpl.cpp?from=nsTimerEvent#138

   That in itself probably merits a comment.

Comment 20

[Maire Reavy [:mreavy]]

Hopefully this fixes our intermittent Bug 941751

Comment 21

[Boris Zbarsky [:bzbarsky]]

Oh, ouch.  Yes, that can definitely use a comment!

Comment 22

[Byron Campen [:bwc]]

Attachment: Part 1: Add test-case for timers firing when the target thread is not running.

Update commit message.

Comment 23

[Byron Campen [:bwc]]

Attachment: Part 2: Make refcounting logic around PostTimerEvent more explicit.

Incorporate feedback, and suppress an unused variable warning.

Comment 24

[Byron Campen [:bwc]]

Comment on attachment 8440939 [details] [diff] [review]
Part 1: Add test-case for timers firing when the target thread is not running.

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

Might be a good idea to land this too?

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8440939 [details] [diff] [review]
Part 1: Add test-case for timers firing when the target thread is not running.

Yes, please!

Comment 26

[Byron Campen [:bwc]]

Comment on attachment 8440941 [details] [diff] [review]
Part 2: Make refcounting logic around PostTimerEvent more explicit.

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

Carry forward r=bz

Comment 27

[Byron Campen [:bwc]]

Seems to be working, will give the test-case another scrub tomorrow morning since it was just a hacked-up copy/paste job.

https://tbpl.mozilla.org/?tree=Try&rev=677d6e237448

Comment 28

[Benjamin Smedberg]

It should be an assertable problem for a timer to stay alive when the target thread is not running. I think we should assert that there, but also consider whether we can assert it at nsThread::Shutdown also.

Comment 29

[Byron Campen [:bwc]]

I don't know about that. Cancel() does not prevent TimerThread from trying to dispatch; it only prevents the timer from popping once it hits the target thread. So, if Cancel() races the TimerThread, it is very easy for a dispatch to the target thread to fail.

Comment 30

[Benjamin Smedberg]

Why can't it prevent the dispatch? In particular I expect the following sequence should not attempt to dispatch to a dead thread:

mTimer->Cancel();
mThread->Shutdown();

This pattern should really assert if we can make it:

mThread->Shutdown(); // ASSERT: Active timer pointing at dying thread
mTimer->Cancel();

Comment 31

[Andrew McCreight [:mccr8]]

Sounds bad, so I'm going to mark this sec-high.  Please get sec-approval before landing.

Comment 32

[Byron Campen [:bwc]]

Looking at the code again, it does look like Cancel() should be removing the timer from TimerThread's set of timers, so I'll need to look into why we don't learn that our thread is shutting down until after dispatches are being denied.

Comment 33

[Byron Campen [:bwc]]

Ok, there is still a race here. Let's say the TimerThread is cruising along, and decides to fire a timer. We end up going through the motions, and end up here:

http://dxr.mozilla.org/mozilla-central/source/xpcom/threads/TimerThread.cpp#251

We have removed the timer from mTimers at this point, and we unlock. Then, on the target thread, we call Cancel(). This flips the mCancelled bit, and tries to remove from mTimers again (with no effect). Since the monitor is unlocked (see above), we return straightaway:

http://dxr.mozilla.org/mozilla-central/source/xpcom/threads/nsTimerImpl.cpp#434

Then, the target thread shuts down, and TimerThread continues execution where it left off, and attempts to post the timer.

Comment 34

[Byron Campen [:bwc]]

I should clarify; the race above is pretty much benign once the above patches are in place, since we recover from the failed dispatch. But if we add an assert, we're going to hit it occasionally even when the user of the timer is doing the right thing.

Comment 35

[Byron Campen [:bwc]]

Attachment: Part 1: Add test-case for timers firing when the target thread is not running.

Pare down the test case a bit.

Comment 36

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8441653 [details] [diff] [review]
Part 1: Add test-case for timers firing when the target thread is not running.

r=me.  Does this test actually fail without the part 2 patch?

Comment 37

[Byron Campen [:bwc]]

(In reply to Boris Zbarsky [:bz] from comment #36)
> Comment on attachment 8441653 [details] [diff] [review]
> Part 1: Add test-case for timers firing when the target thread is not
> running.
> 
> r=me.  Does this test actually fail without the part 2 patch?

Yes, we hit the rc != 0 assertion. I could do a non-debug ASAN build and verify that we double free, but that would take quite a bit of time.

Needinfo self to check back on try push.

https://tbpl.mozilla.org/?tree=Try&rev=638b6db666c1

Comment 38

[Byron Campen [:bwc]]

Oh now this is interesting; the Marionette tests are very sad on windows with these patches.

Comment 39

[Byron Campen [:bwc]]

Trying again with a more recent pull:

https://tbpl.mozilla.org/?tree=Try&rev=5d297691df6a

Comment 40

[Byron Campen [:bwc]]

Comment on attachment 8440941 [details] [diff] [review]
Part 2: Make refcounting logic around PostTimerEvent more explicit.

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

   For code that calls Cancel() on its timers before thread shutdown, it is an extremely tight race to hit this bug. If there was something content could do to cause very short lifetime timers to be scheduled, and then cause thread shutdown, it might be able to trigger the race in comment 33 with some frequency, but it is hard to say (this is my current hypothesis as to the cause of bug 941751). For code that does not call Cancel() when it is supposed to, it becomes much easier, but would require content to cause the target thread in question to shut down in some way (possibly by inducing the user to close the browser, for example). However, it seems doubtful that we have code that does this, or else we'd see this assertion being hit very frequently. 

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

   The test-case might provide a hint, but the bugfix looks like code cleanup.

Which older supported branches are affected by this flaw?

   Probably all; the patch on bug 744836 introduced this.

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

   I do not have backports, but given how little the code has changed, I do not think they will be particularly hard to construct.

How likely is this patch to cause regressions; how much testing does it need?

   Well, this patch is a change in refcounting logic, on a widely-used feature. I think I've gotten it right, but there is some risk. At least we're making use of the safer nsRefPtr and already_AddRefed (the only departure from this is where we check the refcount when PostTimerEvent fails, but we know for sure the existing code would do something nasty here).

Comment 41

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

Comment on attachment 8440941 [details] [diff] [review]
Part 2: Make refcounting logic around PostTimerEvent more explicit.

sec-approval+ for trunk.

We should get Aurora, Beta, and ESR24 patches for this.

Comment 42

[Byron Campen [:bwc]]

I'll go ahead and cook those up.

Comment 43

[Byron Campen [:bwc]]

Patches seem to apply cleanly to aurora. Verifying that they build and then running some tests.

Comment 44

[Byron Campen [:bwc]]

Patches apply cleanly to beta. Checking build-tests there now.

Comment 45

[Byron Campen [:bwc]]

Comment on attachment 8441653 [details] [diff] [review]
Part 1: Add test-case for timers firing when the target thread is not running.

aurora and beta seem to do what I'd expect with the patches as they are. Not sure if we want to land the test case too, but here's the request.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 

   bug 744836

User impact if declined:

   No unit test in CI for the bugfix.

Testing completed (on m-c, etc.):

   This test is being run in CI.
   
Risk to taking this patch (and alternatives if risky): 

   None apart from test failures.

String or IDL/UUID changes made by this patch:

   None.

Comment 46

[Byron Campen [:bwc]]

Comment on attachment 8440941 [details] [diff] [review]
Part 2: Make refcounting logic around PostTimerEvent more explicit.

aurora and beta seem to work fine with the patches as-is

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 

   bug 744836

User impact if declined: 

   Rare UAF when shutting down a thread that uses timers.

Testing completed (on m-c, etc.):

   CI testing on try, plus a new test-case in TestTimers.

Risk to taking this patch (and alternatives if risky):

   There is some risk here, since we're messing with refcounts on a widely-used feature. I suppose an alternative would be to abort() if the dispatch fails, so we have a controlled crash.

String or IDL/UUID changes made by this patch:

   None.

Comment 47

[Byron Campen [:bwc]]

Attachment: Part 1: (patch for esr24) Add test-case for timers firing when the target thread is not running.

test case for esr24

Comment 48

[Byron Campen [:bwc]]

Comment on attachment 8443829 [details] [diff] [review]
Part 1: (patch for esr24) Add test-case for timers firing when the target thread is not running.

[Approval Request Comment]

See separate comment for aurora/beta approval.

Comment 49

[Byron Campen [:bwc]]

Attachment: Part 2: (patch for esr24) Make refcounting logic around PostTimerEvent more explicit.

bugfix for esr24

Comment 50

[Byron Campen [:bwc]]

Comment on attachment 8443830 [details] [diff] [review]
Part 2: (patch for esr24) Make refcounting logic around PostTimerEvent more explicit.

[Approval Request Comment]

See comment for aurora/beta approval.

Comment 51

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8441653 [details] [diff] [review]
Part 1: Add test-case for timers firing when the target thread is not running.

Taking the 2 patches for aurora. If no issues are found, I will accept them for beta 5 (end of the week)

Comment 52

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2210c3fbcf63
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c09da2ef546

Comment 53

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2210c3fbcf63
https://hg.mozilla.org/mozilla-central/rev/0c09da2ef546

Comment 54

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b1873b86994c
https://hg.mozilla.org/releases/mozilla-aurora/rev/358992828820

https://hg.mozilla.org/releases/mozilla-beta/rev/7ba3110d484c
https://hg.mozilla.org/releases/mozilla-beta/rev/e3522dec2b84

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/14bfee369c2c
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/9117bdce9e48

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/541fb0359578
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/9e79ec1dd96f

https://hg.mozilla.org/releases/mozilla-esr24/rev/a55d20b0230a
https://hg.mozilla.org/releases/mozilla-esr24/rev/1ba65e0ee8b1

Comment 56

[Matt Wobensmith [:mwobensmith][:matt:]]

Marking [qa-] for verification purposes, based on comments and lack of testable media / STR. Please feel free to provide if you'd like help in testing, thank you.