Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(176)

Issue 10373047: code review 10373047: runtime: prevent a panic from leaving the timer mutex held (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 3 months ago by jeff.allen
Modified:
9 years, 2 months ago
Reviewers:
DMorsing, rsc
CC:
golang-dev, DMorsing, dvyukov, rsc, iant
Visibility:
Public.

Description

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.

Patch Set 1 #

Patch Set 2 : diff -r cbcc3e864275 http://code.google.com/p/go #

Patch Set 3 : diff -r cbcc3e864275 http://code.google.com/p/go #

Patch Set 4 : diff -r cbcc3e864275 http://code.google.com/p/go #

Patch Set 5 : diff -r cbcc3e864275 http://code.google.com/p/go #

Total comments: 8

Patch Set 6 : diff -r 29818ed2e3f7 http://code.google.com/p/go #

Total comments: 2

Patch Set 7 : diff -r 29818ed2e3f7 http://code.google.com/p/go #

Patch Set 8 : diff -r 166d946fa77f http://code.google.com/p/go #

Unified diffs Side-by-side diffs Delta from patch set Stats (+25 lines, -0 lines) Patch
M src/pkg/runtime/time.goc View 1 2 3 4 5 6 7 1 chunk +5 lines, -0 lines 0 comments Download
M src/pkg/time/sleep_test.go View 1 2 3 4 5 6 7 1 chunk +20 lines, -0 lines 0 comments Download

Messages

Total messages: 20
jeff.allen
Here is a proposed fix for "Issue 5745: Lockup in time.stopTimer". From inspection, it seems ...
9 years, 3 months ago (2013-06-21 08:31:11 UTC) #1
DMorsing
NOT LGTM. This introduces a race when deleting a timer that's currently being moved on ...
9 years, 3 months ago (2013-06-21 08:57:32 UTC) #2
dvyukov
Yeah, I would check for nil timer and return false.
9 years, 3 months ago (2013-06-21 10:35:59 UTC) #3
jeff.allen
I don't see the race. Assuming that mutex "timer" protects the integrity of the timer ...
9 years, 3 months ago (2013-06-21 11:46:36 UTC) #4
dvyukov
On 2013/06/21 11:46:36, jeff.allen wrote: > I don't see the race. Assuming that mutex "timer" ...
9 years, 3 months ago (2013-06-21 11:55:41 UTC) #5
jeff.allen
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
9 years, 3 months ago (2013-06-21 13:30:51 UTC) #6
dvyukov
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#newcode57 src/pkg/time/sleep.go:57: if t == nil { it is not necessary ...
9 years, 3 months ago (2013-06-21 15:07:57 UTC) #7
rsc
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#newcode57 src/pkg/time/sleep.go:57: if t == nil { On 2013/06/21 15:07:57, dvyukov ...
9 years, 3 months ago (2013-06-21 18:39:32 UTC) #8
dvyukov
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#newcode57 > ...
9 years, 3 months ago (2013-06-22 11:07:45 UTC) #9
iant
On Sat, Jun 22, 2013 at 4:07 AM, <dvyukov@google.com> wrote: > > t must be ...
9 years, 3 months ago (2013-06-22 14:41:03 UTC) #10
jeff.allen
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#newcode321 src/pkg/time/sleep_test.go:321: ticker := NewTicker(Duration(10000) * Millisecond) On 2013/06/21 15:07:57, ...
9 years, 3 months ago (2013-06-25 09:26:01 UTC) #11
dvyukov
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#newcode83 src/pkg/time/sleep.go:83: if t == nil { I can imagine how ...
9 years, 3 months ago (2013-06-25 10:28:43 UTC) #12
jeff.allen
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#newcode83 src/pkg/time/sleep.go:83: if t == nil { On 2013/06/25 10:28:43, dvyukov ...
9 years, 3 months ago (2013-06-25 14:58:37 UTC) #13
dvyukov
looks good, but wait for Russ or Rob
9 years, 3 months ago (2013-06-25 15:01:21 UTC) #14
rsc
I don't think we should be defining the semantics of a nil *Timer. It's fine ...
9 years, 3 months ago (2013-06-27 16:30:45 UTC) #15
dvyukov
On 2013/06/27 16:30:45, rsc wrote: > I don't think we should be defining the semantics ...
9 years, 3 months ago (2013-06-27 16:40:37 UTC) #16
rsc
Then do // Dereference t so that any panic happens here. // Discard result, because ...
9 years, 3 months ago (2013-06-27 17:10:31 UTC) #17
jeff.allen
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.
9 years, 3 months ago (2013-06-28 08:47:05 UTC) #18
rsc
LGTM
9 years, 3 months ago (2013-07-02 01:22:58 UTC) #19
rsc
9 years, 3 months ago (2013-07-02 01:42:36 UTC) #20
*** 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>
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b