code review 11666046: [release-branch.go1.1] time: prevent a panic from leavi...

[release-branch.go1.1] time: prevent a panic from leaving the timer mutex held

««« CL 10373047 / 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>
»»»

Update issue 5928

[adg, 2013-07-22 08:36:36]

Hello golang-dev@googlegroups.com,

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

[dfc, 2013-07-22 08:40:01]

LGTM.



On 22/07/2013, at 18:36, adg@golang.org wrote:

> Reviewers: golang-dev1,
> 
> Message:
> Hello golang-dev@googlegroups.com,
> 
> I'd like you to review this change to
> https://code.google.com/p/go
> 
> 
> Description:
> [release-branch.go1.1] time: prevent a panic from leaving the timer
> mutex held
> 
> ««« CL 10373047 / 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>
> »»»
> 
> Update issue 5928
> 
> Please review this at https://codereview.appspot.com/11666046/
> 
> Affected files:
>  M src/pkg/runtime/time.goc
>  M src/pkg/time/sleep_test.go
> 
> 
> Index: src/pkg/runtime/time.goc
> ===================================================================
> --- a/src/pkg/runtime/time.goc
> +++ b/src/pkg/runtime/time.goc
> @@ -131,6 +131,11 @@
> {
>    int32 i;
> 
> +    // Dereference t so that any panic happens before the lock is held.
> +    // Discard result, because t might be moving in the heap.
> +    i = t->i;
> +    USED(i);
> +
>    runtime·lock(&timers);
> 
>    // t may not be registered anymore and may have
> Index: src/pkg/time/sleep_test.go
> ===================================================================
> --- a/src/pkg/time/sleep_test.go
> +++ b/src/pkg/time/sleep_test.go
> @@ -314,3 +314,23 @@
>        t.Fatalf("negative timeout didn't fire")
>    }
> }
> +
> +// Test that a panic while deleting a timer does not leave
> +// the timers mutex held, deadlocking a ticker.Stop in a defer.
> +func TestIssue5745(t *testing.T) {
> +    ticker := NewTicker(Hour)
> +    defer func() {
> +        // would deadlock here before the fix due to
> +        // lock taken before the segfault.
> +        ticker.Stop()
> +
> +        if r := recover(); r == nil {
> +            t.Error("Expected panic, but none happened.")
> +        }
> +    }()
> +
> +    // cause a panic due to a segfault
> +    var timer *Timer
> +    timer.Stop()
> +    t.Error("Should be unreachable.")
> +}
> 
> 
> -- 
> 
> ---You received this message because you are subscribed to the Google Groups
"golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email to golang-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
> 
> 

[adg, 2013-07-22 08:41:36]

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

[release-branch.go1.1] time: prevent a panic from leaving the timer mutex held

««« CL 10373047 / 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>
»»»

Update issue 5928

R=golang-dev, dave
CC=golang-dev
https://codereview.appspot.com/11666046