code review 10373047: runtime: prevent a panic from leaving the timer mutex held

runtime: prevent a panic from leaving the timer mutex held

When deleting a timer, a panic due to nil deref
would leave a lock held, possibly leading to a deadlock
in a defer. Instead return false on a nil timer.

Fixes issue 5745.

[jeff.allen, 2013-06-21 08:31:11]

Here is a proposed fix for "Issue 5745:	Lockup in time.stopTimer".

From inspection, it seems like the same risk exists in addtimer. For example if
malloc or newproc1 cause panics, then the timers mutex will be held during the
defer and any addtimer/deltimer in the defer would deadlock. But it is a lot
harder to see how to solve this risk compared to fixing deltimer.

Ideas?

[DMorsing, 2013-06-21 08:57:32]

NOT LGTM.

This introduces a race when deleting a timer that's currently being moved on the
heap.

Either insert a check for nil and panic or make stopping a nil timer into a
no-op.

[dvyukov, 2013-06-21 10:35:59]

Yeah, I would check for nil timer and return false.

[jeff.allen, 2013-06-21 11:46:36]

I don't see the race. Assuming that mutex "timer" protects the integrity of the
timer heap. You seem to be implying that the mutex also protects all accesses to
struct Timer.i field as well. Is that the problem?

The reason I didn't fix it with a nil check is that I didn't want to make a
behavior change. It seems correct to me that this is a segv-induced panic.

[dvyukov, 2013-06-21 11:55:41]

On 2013/06/21 11:46:36, jeff.allen wrote:
> I don't see the race. Assuming that mutex "timer" protects the integrity of
the
> timer heap. You seem to be implying that the mutex also protects all accesses
to
> struct Timer.i field as well. Is that the problem?

Yes.

> The reason I didn't fix it with a nil check is that I didn't want to make a
> behavior change. It seems correct to me that this is a segv-induced panic.

If you don't want to change the behavior, then you need to leave the deadlock
there -- it is the current behavior.
Arguably stop of a non-started timer must just return false (the timer was not
stopped):

func (t *Timer) Stop() bool
    Stop prevents the Timer from firing. It returns true if the call stops
    the timer, false if the timer has already expired or been stopped. Stop
    does not close the channel, to prevent a read from the channel
    succeeding incorrectly.

[jeff.allen, 2013-06-21 13:30:51]

Hello golang-dev@googlegroups.com, daniel.morsing@gmail.com, dvyukov@google.com
(cc: golang-dev@googlegroups.com),

I'd like you to review this change to
http://code.google.com/p/go

[dvyukov, 2013-06-21 15:07:57]

https://codereview.appspot.com/10373047/diff/9002/src/pkg/time/sleep.go
File src/pkg/time/sleep.go (right):

https://codereview.appspot.com/10373047/diff/9002/src/pkg/time/sleep.go#newco...
src/pkg/time/sleep.go:57: if t == nil {
it is not necessary nil
consider:

type X struct {
  pad int
  t Timer
}

var x *X

x.t.Stop()

https://codereview.appspot.com/10373047/diff/9002/src/pkg/time/sleep_test.go
File src/pkg/time/sleep_test.go (right):

https://codereview.appspot.com/10373047/diff/9002/src/pkg/time/sleep_test.go#...
src/pkg/time/sleep_test.go:321: ticker := NewTicker(Duration(10000) *
Millisecond)
s/Duration(10000) * Millisecond/Hour/?

https://codereview.appspot.com/10373047/diff/9002/src/pkg/time/sleep_test.go#...
src/pkg/time/sleep_test.go:328: var tmr *Timer
s/tmr/t/

https://codereview.appspot.com/10373047/diff/9002/src/pkg/time/sleep_test.go#...
src/pkg/time/sleep_test.go:329: stopped := tmr.Stop()
if tmr.Stop() {
  ...
}

[rsc, 2013-06-21 18:39:32]

https://codereview.appspot.com/10373047/diff/9002/src/pkg/time/sleep.go
File src/pkg/time/sleep.go (right):

https://codereview.appspot.com/10373047/diff/9002/src/pkg/time/sleep.go#newco...
src/pkg/time/sleep.go:57: if t == nil {
On 2013/06/21 15:07:57, dvyukov wrote:
> it is not necessary nil
> consider:
> 
> type X struct {
>   pad int
>   t Timer
> }
> 
> var x *X
> 
> x.t.Stop()

Don't try to guard against that in this code.
That's a different bug that we should also fix for Go 1.2
(golang.org/issue/4238).

[dvyukov, 2013-06-22 11:07:45]

On 2013/06/21 18:39:32, rsc wrote:
> https://codereview.appspot.com/10373047/diff/9002/src/pkg/time/sleep.go
> File src/pkg/time/sleep.go (right):
> 
>
https://codereview.appspot.com/10373047/diff/9002/src/pkg/time/sleep.go#newco...
> src/pkg/time/sleep.go:57: if t == nil {
> On 2013/06/21 15:07:57, dvyukov wrote:
> > it is not necessary nil
> > consider:
> > 
> > type X struct {
> >   pad int
> >   t Timer
> > }
> > 
> > var x *X
> > 
> > x.t.Stop()
> 
> Don't try to guard against that in this code.
> That's a different bug that we should also fix for Go 1.2
> (golang.org/issue/4238).

t must be nil in the following case, right?

var x *X
t := &x.t

[iant, 2013-06-22 14:41:03]

On Sat, Jun 22, 2013 at 4:07 AM,  <dvyukov@google.com> wrote:
>
> t must be nil in the following case, right?
>
> var x *X
> t := &x.t

http://golang.org/issue/4238

[jeff.allen, 2013-06-25 09:26:01]

PTAL.

https://codereview.appspot.com/10373047/diff/9002/src/pkg/time/sleep_test.go
File src/pkg/time/sleep_test.go (right):

https://codereview.appspot.com/10373047/diff/9002/src/pkg/time/sleep_test.go#...
src/pkg/time/sleep_test.go:321: ticker := NewTicker(Duration(10000) *
Millisecond)
On 2013/06/21 15:07:57, dvyukov wrote:
> s/Duration(10000) * Millisecond/Hour/?

Done.

https://codereview.appspot.com/10373047/diff/9002/src/pkg/time/sleep_test.go#...
src/pkg/time/sleep_test.go:328: var tmr *Timer
t is a *testing.T in this scope. So I dug deep in the vowel well and changed tmr
to timer. :)

https://codereview.appspot.com/10373047/diff/9002/src/pkg/time/sleep_test.go#...
src/pkg/time/sleep_test.go:329: stopped := tmr.Stop()
On 2013/06/21 15:07:57, dvyukov wrote:
> if tmr.Stop() {
>   ...
> }

Done.

[dvyukov, 2013-06-25 10:28:43]

https://codereview.appspot.com/10373047/diff/23001/src/pkg/time/sleep.go
File src/pkg/time/sleep.go (right):

https://codereview.appspot.com/10373047/diff/23001/src/pkg/time/sleep.go#newc...
src/pkg/time/sleep.go:83: if t == nil {
I can imagine how Stop() is useful with nil Timer. But Reset() with nil Timer
seems different. It can't possibly do what user asks to do -- start a new timer.
I think it needs to panic.

[jeff.allen, 2013-06-25 14:58:37]

https://codereview.appspot.com/10373047/diff/23001/src/pkg/time/sleep.go
File src/pkg/time/sleep.go (right):

https://codereview.appspot.com/10373047/diff/23001/src/pkg/time/sleep.go#newc...
src/pkg/time/sleep.go:83: if t == nil {
On 2013/06/25 10:28:43, dvyukov wrote:
> I can imagine how Stop() is useful with nil Timer. But Reset() with nil Timer
> seems different. It can't possibly do what user asks to do -- start a new
timer.
> I think it needs to panic.

Done.

[dvyukov, 2013-06-25 15:01:21]

looks good, but wait for Russ or Rob

[rsc, 2013-06-27 16:30:45]

I don't think we should be defining the semantics of a nil *Timer. It's fine to
panic, it just can't panic while the runtime is holding the timers lock. Could
you try reverting the changes to time.go and applying this to runtime/time.goc
instead?

diff -r b836ce280918 src/pkg/runtime/time.goc
--- a/src/pkg/runtime/time.goc	Thu Jun 27 09:20:02 2013 -0700
+++ b/src/pkg/runtime/time.goc	Thu Jun 27 12:29:33 2013 -0400
@@ -131,12 +131,15 @@
 {
 	int32 i;
 
-	runtime·lock(&timers);
-
 	// t may not be registered anymore and may have
 	// a bogus i (typically 0, if generated by Go).
 	// Verify it before proceeding.
+	// Read i before acquiring lock on timers, so that
+	// if t is a bad pointer, the panic happens without
+	// the lock held.
 	i = t->i;
+
+	runtime·lock(&timers);
 	if(i < 0 || i >= timers.len || timers.t[i] != t) {
 		runtime·unlock(&timers);
 		return false;

Then the nil Stop will panic as before, but a second one will also panic (not
deadlock).

Russ

[dvyukov, 2013-06-27 16:40:37]

On 2013/06/27 16:30:45, rsc wrote:
> I don't think we should be defining the semantics of a nil *Timer. It's fine
to
> panic, it just can't panic while the runtime is holding the timers lock. Could
> you try reverting the changes to time.go and applying this to runtime/time.goc
> instead?
> 
> diff -r b836ce280918 src/pkg/runtime/time.goc
> --- a/src/pkg/runtime/time.goc	Thu Jun 27 09:20:02 2013 -0700
> +++ b/src/pkg/runtime/time.goc	Thu Jun 27 12:29:33 2013 -0400
> @@ -131,12 +131,15 @@
>  {
>  	int32 i;
>  
> -	runtime·lock(&timers);
> -
>  	// t may not be registered anymore and may have
>  	// a bogus i (typically 0, if generated by Go).
>  	// Verify it before proceeding.
> +	// Read i before acquiring lock on timers, so that
> +	// if t is a bad pointer, the panic happens without
> +	// the lock held.
>  	i = t->i;
> +
> +	runtime·lock(&timers);
>  	if(i < 0 || i >= timers.len || timers.t[i] != t) {
>  		runtime·unlock(&timers);
>  		return false;
> 
> Then the nil Stop will panic as before, but a second one will also panic (not
> deadlock).
> 
> Russ

See above:
This introduces a race when deleting a timer that's currently being moved on the
heap.

[rsc, 2013-06-27 17:10:31]

Then do

// Dereference t so that any panic happens here.
// Discard result, because t might be moving in the heap.
i = t->i;
USED(i);

runtime.lock(&timers);
i = t->i;

[jeff.allen, 2013-06-28 08:47:05]

Hello golang-dev@googlegroups.com, daniel.morsing@gmail.com, dvyukov@google.com,
rsc@golang.org, iant@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2013-07-02 01:22:58]

LGTM

[rsc, 2013-07-02 01:42:36]

*** Submitted as https://code.google.com/p/go/source/detail?r=974a69ed9fcf ***

time: prevent a panic from leaving the timer mutex held

When deleting a timer, a panic due to nil deref
would leave a lock held, possibly leading to a deadlock
in a defer. Instead return false on a nil timer.

Fixes issue 5745.

R=golang-dev, daniel.morsing, dvyukov, rsc, iant
CC=golang-dev
https://codereview.appspot.com/10373047

Committer: Russ Cox <rsc@golang.org>