Skip to content
Permalink
Browse files

runtime: never call into race detector with retaken P

cgocall could previously invoke the race detector on an M whose P had
been retaken. The race detector would attempt to use the P-local state
from this stale P, racing with the thread that was actually wired to
that P. The result was memory corruption of ThreadSanitizer's internal
data structures that presented as hard-to-understand assertion failures
and segfaults.

Reorder cgocall so that it always acquires a P before invoking the race
detector, and add a test that stresses the interaction between cgo and
the race detector to protect against future bugs of this kind.

Fixes golang#27660.

Change-Id: Ide93f96a23490314d6647547140e0a412a97f0d4
Reviewed-on: https://go-review.googlesource.com/c/148717
Run-TryBot: Dmitry Vyukov <dvyukov@google.com>
Reviewed-by: Dmitry Vyukov <dvyukov@google.com>
  • Loading branch information...
benesch authored and dvyukov committed Nov 9, 2018
1 parent 06be7cb commit e496e612b7f45a09209f8f4e1c7c1d0db378dc18
Showing with 68 additions and 15 deletions.
  1. +1 −0 misc/cgo/test/cgo_test.go
  2. +54 −0 misc/cgo/test/test27660.go
  3. +13 −15 src/runtime/cgocall.go
@@ -92,6 +92,7 @@ func Test25143(t *testing.T) { test25143(t) }
func Test23356(t *testing.T) { test23356(t) }
func Test26066(t *testing.T) { test26066(t) }
func Test26213(t *testing.T) { test26213(t) }
func Test27660(t *testing.T) { test27660(t) }

func BenchmarkCgoCall(b *testing.B) { benchCgoCall(b) }
func BenchmarkGoString(b *testing.B) { benchGoString(b) }
@@ -0,0 +1,54 @@
// Copyright 2018 The Go Authors. All rights reserved.
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file.

// Stress the interaction between the race detector and cgo in an
// attempt to reproduce the memory corruption described in #27660.
// The bug was very timing sensitive; at the time of writing this
// test would only trigger the bug about once out of every five runs.

package cgotest

// #include <unistd.h>
import "C"

import (
"context"
"math/rand"
"runtime"
"sync"
"testing"
"time"
)

func test27660(t *testing.T) {
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
ints := make([]int, 100)
locks := make([]sync.Mutex, 100)
// Slowly create threads so that ThreadSanitizer is forced to
// frequently resize its SyncClocks.
for i := 0; i < 100; i++ {
go func() {
for ctx.Err() == nil {
// Sleep in C for long enough that it is likely that the runtime
// will retake this goroutine's currently wired P.
C.usleep(1000 /* 1ms */)
runtime.Gosched() // avoid starvation (see #28701)
}
}()
go func() {
// Trigger lots of synchronization and memory reads/writes to
// increase the likelihood that the race described in #27660
// results in corruption of ThreadSanitizer's internal state
// and thus an assertion failure or segfault.
for ctx.Err() == nil {
j := rand.Intn(100)
locks[j].Lock()
ints[j]++
locks[j].Unlock()
}
}()
time.Sleep(time.Millisecond)
}
}
@@ -130,12 +130,19 @@ func cgocall(fn, arg unsafe.Pointer) int32 {
mp.incgo = true
errno := asmcgocall(fn, arg)

// Call endcgo before exitsyscall because exitsyscall may
// Update accounting before exitsyscall because exitsyscall may
// reschedule us on to a different M.
endcgo(mp)
mp.incgo = false
mp.ncgo--

exitsyscall()

// Note that raceacquire must be called only after exitsyscall has
// wired this M to a P.
if raceenabled {
raceacquire(unsafe.Pointer(&racecgosync))
}

// From the garbage collector's perspective, time can move
// backwards in the sequence above. If there's a callback into
// Go code, GC will see this function at the call to
@@ -153,16 +160,6 @@ func cgocall(fn, arg unsafe.Pointer) int32 {
return errno
}

//go:nosplit
func endcgo(mp *m) {
mp.incgo = false
mp.ncgo--

if raceenabled {
raceacquire(unsafe.Pointer(&racecgosync))
}
}

// Call from C back to Go.
//go:nosplit
func cgocallbackg(ctxt uintptr) {
@@ -347,13 +344,14 @@ func unwindm(restore *bool) {
sched.sp = *(*uintptr)(unsafe.Pointer(sched.sp + 16))
}

// Call endcgo to do the accounting that cgocall will not have a
// chance to do during an unwind.
// Do the accounting that cgocall will not have a chance to do
// during an unwind.
//
// In the case where a Go call originates from C, ncgo is 0
// and there is no matching cgocall to end.
if mp.ncgo > 0 {
endcgo(mp)
mp.incgo = false
mp.ncgo--
}

releasem(mp)

0 comments on commit e496e61

Please sign in to comment.
You can’t perform that action at this time.