code review 7981043: sync, sync/atomic: do not corrupt race detector after a...

sync, sync/atomic: do not corrupt race detector after a nil dereference.

The race detector uses a global lock to analyze atomic
operations. A panic in the middle of the code leaves the
lock acquired.

Similarly, the sync package may leave the race detectro
inconsistent when methods are called on nil pointers.

[remyoudompheng, 2013-03-22 23:14:46]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[r, 2013-03-23 00:27:30]

They might in-line already, but should it be done by hand anyway? You'd could
have more accurate messages with less overhead

[minux1, 2013-03-23 07:31:17]

check for <4096 doesn't solve the general
peoblem of a bad pointer passed in.

why not just use defer to release the mtx?

[remyoudompheng, 2013-03-23 08:18:07]

On 2013/03/23 07:31:17, minux wrote:
> check for <4096 doesn't solve the general
> peoblem of a bad pointer passed in.
> 
> why not just use defer to release the mtx?

It seems a naive approach usingdefers is not enough to leave the race detector
in a consistent state. Maybe dmitry can shed a light on this.

[dvyukov, 2013-03-25 14:00:48]

On 2013/03/23 08:18:07, remyoudompheng wrote:
> On 2013/03/23 07:31:17, minux wrote:
> > check for <4096 doesn't solve the general
> > peoblem of a bad pointer passed in.
> > 
> > why not just use defer to release the mtx?
> 
> It seems a naive approach usingdefers is not enough to leave the race detector
> in a consistent state. Maybe dmitry can shed a light on this.

Yes, it's quite possible that SIGSEGV leaves internal race detector state in an
inconsistent state.

[dvyukov, 2013-03-25 14:00:54]

https://codereview.appspot.com/7981043/diff/5001/src/pkg/sync/atomic/race.go
File src/pkg/sync/atomic/race.go (right):

https://codereview.appspot.com/7981043/diff/5001/src/pkg/sync/atomic/race.go#...
src/pkg/sync/atomic/race.go:25: panic("nil pointer dereference in atomic
operation")
throw the same "invalid memory address or nil pointer dereference", somebody can
be checking for this particular string

[dvyukov, 2013-03-25 14:01:05]

LGTM

[dvyukov, 2013-03-25 14:02:34]

On 2013/03/23 00:27:30, r wrote:
> They might in-line already, but should it be done by hand anyway? You'd could
> have more accurate messages with less overhead

I would prefer exactly the same message as runtime throws on nil deref --
"invalid memory address or nil pointer dereference". The race detector should
try hard to preserve visible program behavior.

[r, 2013-03-25 15:55:46]

On Mon, Mar 25, 2013 at 7:02 AM,  <dvyukov@google.com> wrote:
> On 2013/03/23 00:27:30, r wrote:
>>
>> They might in-line already, but should it be done by hand anyway?
>
> You'd could
>>
>> have more accurate messages with less overhead
>
>
> I would prefer exactly the same message as runtime throws on nil deref
> -- "invalid memory address or nil pointer dereference". The race
> detector should try hard to preserve visible program behavior.

That's a fine counterargument.

-rob

[minux1, 2013-03-25 19:49:19]

On 2013/03/25 14:00:48, dvyukov wrote:
> Yes, it's quite possible that SIGSEGV leaves internal race detector state in
an
> inconsistent state.
can we fix that instead? Go program is unlike C where the SIGSEGV handler could
do arbitrary things.
I still feel unsure about the check for 4096 here, for example, people might
actually map a page at address 0 for compatibility reasons.

[rsc, 2013-03-25 19:58:22]

It would be better to dereference the pointer, so that the runtime can
trigger the panic itself. The panic string may match but the panic value
differs here.

_ = *addr
or
x := *addr
_ = x

seems like it should be enough.

[dvyukov, 2013-03-26 06:34:48]

On 2013/03/25 19:58:22, rsc wrote:
> It would be better to dereference the pointer, so that the runtime can
> trigger the panic itself. The panic string may match but the panic value
> differs here.
> 
> _ = *addr
> or
> x := *addr
> _ = x
> 
> seems like it should be enough.

Agree

[dvyukov, 2013-03-26 06:40:40]

On 2013/03/25 19:49:19, minux wrote:
> On 2013/03/25 14:00:48, dvyukov wrote:
> > Yes, it's quite possible that SIGSEGV leaves internal race detector state in
> an
> > inconsistent state.
> can we fix that instead? Go program is unlike C where the SIGSEGV handler
could
> do arbitrary things.

There are 2 reasons I do not want to do this:
1. Importing new race runtime at this stage is risky (similar to rewriting
goroutine scheduler).
2. I will do the same in race runtime, but with higher chances of being
inconsistent with Go runtime. We don't have any kind of exceptions/unwind/signal
handler/etc.


> I still feel unsure about the check for 4096 here, for example, people might
> actually map a page at address 0 for compatibility reasons.

This should be fine. First, it won't trigger paging fault then. Second, this
check matches runtime check, so it is not worse.

But Russ' suggestion to actually trigger SIGSEGV is better. Note that it
actually introduces the data race, but this file is not instrumented, so it
won't be reported.

[remyoudompheng, 2013-03-28 19:46:17]

Hello golang-dev@googlegroups.com, r@golang.org, minux.ma@gmail.com,
dvyukov@google.com, rsc@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[remyoudompheng, 2013-03-28 19:47:09]

Similar fixes were needed in the sync package to avoid corruptions.

I was not aware that the sync packages were not instrumented. It simplifies
things a lot and the simple dereference at top of functions is OK.

[dvyukov, 2013-03-29 08:08:48]

https://codereview.appspot.com/7981043/diff/22001/src/pkg/sync/mutex.go
File src/pkg/sync/mutex.go (right):

https://codereview.appspot.com/7981043/diff/22001/src/pkg/sync/mutex.go#newco...
src/pkg/sync/mutex.go:84: _ = m.state
Is it actually necessary?

I would expect that this trick for atomics is enough.

Currently mutex.Lock() crashes in atomic operation:

Program received signal SIGSEGV, Segmentation fault.
sync/atomic.CompareAndSwapUint32 (fp=void, val=0x0, old=0, new=1, swapped=false)
at 4/src/pkg/sync/atomic/race.go:32
32		if *val == old {
(gdb) bt
#0  sync/atomic.CompareAndSwapUint32 (fp=void, val=0x0, old=0, new=1,
swapped=false) at 4/src/pkg/sync/atomic/race.go:32
#1  0x000000000041b8f3 in sync/atomic.CompareAndSwapInt32 (fp=void, val=0x0,
old=0, new=1, ~anon3=223) at 4/src/pkg/sync/atomic/race.go:24
#2  0x000000000041b3e3 in sync.(*Mutex).Lock (fp=void, m=0x0) at
4/src/pkg/sync/mutex.go:43
#3  0x0000000000400cf2 in main.main (fp=void) at /tmp/mtc.go:18

[remyoudompheng, 2013-03-29 08:22:23]

It seems necessary because of the racedisable/raceenable thing. The
sequence must be correct even if a segfault happens in between.
Maybe there is something I don't understand , but otherwise the race
tests in sync-test.go fail in totally strange ways.


2013/3/29, dvyukov@google.com <dvyukov@google.com>:
>
> https://codereview.appspot.com/7981043/diff/22001/src/pkg/sync/mutex.go
> File src/pkg/sync/mutex.go (right):
>
>
https://codereview.appspot.com/7981043/diff/22001/src/pkg/sync/mutex.go#newco...
> src/pkg/sync/mutex.go:84: _ = m.state
> Is it actually necessary?
>
> I would expect that this trick for atomics is enough.
>
> Currently mutex.Lock() crashes in atomic operation:
>
> Program received signal SIGSEGV, Segmentation fault.
> sync/atomic.CompareAndSwapUint32 (fp=void, val=0x0, old=0, new=1,
> swapped=false) at 4/src/pkg/sync/atomic/race.go:32
> 32		if *val == old {
> (gdb) bt
> #0  sync/atomic.CompareAndSwapUint32 (fp=void, val=0x0, old=0, new=1,
> swapped=false) at 4/src/pkg/sync/atomic/race.go:32
> #1  0x000000000041b8f3 in sync/atomic.CompareAndSwapInt32 (fp=void,
> val=0x0, old=0, new=1, ~anon3=223) at 4/src/pkg/sync/atomic/race.go:24
> #2  0x000000000041b3e3 in sync.(*Mutex).Lock (fp=void, m=0x0) at
> 4/src/pkg/sync/mutex.go:43
> #3  0x0000000000400cf2 in main.main (fp=void) at /tmp/mtc.go:18
>
> https://codereview.appspot.com/7981043/
>

[dvyukov, 2013-03-29 08:39:45]

On Fri, Mar 29, 2013 at 12:22 PM, Rémy Oudompheng
<remyoudompheng@gmail.com> wrote:
> It seems necessary because of the racedisable/raceenable thing. The

Isn't it always called with defer? Defers should be called on SIGSEGV, right?
I would like understand why we are doing this.


> sequence must be correct even if a segfault happens in between.
> Maybe there is something I don't understand , but otherwise the race
> tests in sync-test.go fail in totally strange ways.
>
>
> 2013/3/29, dvyukov@google.com <dvyukov@google.com>:
>>
>> https://codereview.appspot.com/7981043/diff/22001/src/pkg/sync/mutex.go
>> File src/pkg/sync/mutex.go (right):
>>
>>
https://codereview.appspot.com/7981043/diff/22001/src/pkg/sync/mutex.go#newco...
>> src/pkg/sync/mutex.go:84: _ = m.state
>> Is it actually necessary?
>>
>> I would expect that this trick for atomics is enough.
>>
>> Currently mutex.Lock() crashes in atomic operation:
>>
>> Program received signal SIGSEGV, Segmentation fault.
>> sync/atomic.CompareAndSwapUint32 (fp=void, val=0x0, old=0, new=1,
>> swapped=false) at 4/src/pkg/sync/atomic/race.go:32
>> 32            if *val == old {
>> (gdb) bt
>> #0  sync/atomic.CompareAndSwapUint32 (fp=void, val=0x0, old=0, new=1,
>> swapped=false) at 4/src/pkg/sync/atomic/race.go:32
>> #1  0x000000000041b8f3 in sync/atomic.CompareAndSwapInt32 (fp=void,
>> val=0x0, old=0, new=1, ~anon3=223) at 4/src/pkg/sync/atomic/race.go:24
>> #2  0x000000000041b3e3 in sync.(*Mutex).Lock (fp=void, m=0x0) at
>> 4/src/pkg/sync/mutex.go:43
>> #3  0x0000000000400cf2 in main.main (fp=void) at /tmp/mtc.go:18
>>
>> https://codereview.appspot.com/7981043/
>>

[remyoudompheng, 2013-04-02 06:42:51]

On 2013/03/29 08:39:45, dvyukov wrote:
> On Fri, Mar 29, 2013 at 12:22 PM, Rémy Oudompheng
> <mailto:remyoudompheng@gmail.com> wrote:
> > It seems necessary because of the racedisable/raceenable thing. The
> 
> Isn't it always called with defer? Defers should be called on SIGSEGV, right?
> I would like understand why we are doing this.

I haven't seen defers in the sync code. We could use defer, but some operations
seem to do complex bookkeeping and the pointer dereference makes it simple and
uniform.

What do you think? Maybe I missed something obvious? I'm totally in favour of
better ideas.

[dvyukov, 2013-04-07 21:32:29]

Yes, now I see the problem. Defer is used only in WaitGroup. Panic in most
primitives can lead to disabled race detection or something else.

LGTM

[remyoudompheng, 2013-04-08 07:05:10]

Is this okay for Go 1.1 ?

[adg, 2013-04-08 07:09:06]

I want to only include critical fixes from now on.

It doesn't seem critical to me. What do you think?

How often does the situation that this fix addresses arise?



On 8 April 2013 17:05, <remyoudompheng@gmail.com> wrote:

> Is this okay for Go 1.1 ?
>
>
>
https://codereview.appspot.**com/7981043/<https://codereview.appspot.com/7981...
>
> --
>
> ---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<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>

[remyoudompheng, 2013-04-08 07:15:53]

On 2013/4/8 Andrew Gerrand <adg@golang.org> wrote:
> I want to only include critical fixes from now on.
>
> It doesn't seem critical to me. What do you think?
>
> How often does the situation that this fix addresses arise?

In presence of a nil Mutex, the nil pointer dereference that happens
usually, is transformed under the race detector into a hang, which is
not the same behaviour.

I think it's not an exceptional situation, especially during
degugging, do you agree?

The fix is indeed only for the race detector.

[adg, 2013-04-08 07:20:54]

Okay. I am fine with this going into 1.1, seeing as the race detector is a
significant new feature in the release.


On 8 April 2013 17:15, Rémy Oudompheng <remyoudompheng@gmail.com> wrote:

> On 2013/4/8 Andrew Gerrand <adg@golang.org> wrote:
> > I want to only include critical fixes from now on.
> >
> > It doesn't seem critical to me. What do you think?
> >
> > How often does the situation that this fix addresses arise?
>
> In presence of a nil Mutex, the nil pointer dereference that happens
> usually, is transformed under the race detector into a hang, which is
> not the same behaviour.
>
> I think it's not an exceptional situation, especially during
> degugging, do you agree?
>
> The fix is indeed only for the race detector.
>

[remyoudompheng, 2013-04-08 21:46:59]

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

sync, sync/atomic: do not corrupt race detector after a nil dereference.

The race detector uses a global lock to analyze atomic
operations. A panic in the middle of the code leaves the
lock acquired.

Similarly, the sync package may leave the race detectro
inconsistent when methods are called on nil pointers.

R=golang-dev, r, minux.ma, dvyukov, rsc, adg
CC=golang-dev
https://codereview.appspot.com/7981043