runtime: set itab.fun[0] only on successful conversion

For a failed interface conversion not in ",ok" form, getitab
calls itab.init to get the name of the missing method for the
panic message. itab.init will try to find the methods, populate
the method table as it goes. When some method is missing, it sets
itab.fun[0] to 0 before return. There is a small window that
itab.fun[0] could be non-zero.

If concurrently, another goroutine tries to do the same interface
conversion, it will read the same itab's fun[0]. If this happens
in the small window, it sees a non-zero fun[0] and thinks the
conversion succeeded, which is bad.

Fix the race by setting fun[0] to non-zero only when we know the
conversion succeeds. While here, also simplify the syntax
slightly.

Fixes #31419.

Change-Id: Ied34d3043079eb933e330c5877b85e13f98f1916
Reviewed-on: https://go-review.googlesource.com/c/go/+/171759
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>

diff --git a/src/runtime/iface.go b/src/runtime/iface.go
index 246b63b8971968383b6aaadc3e73f219eafd00f7..bb4eccc9bd22f3c2bdcf02fc9ad9d53dd6dcf4dd 100644 (file)
--- a/src/runtime/iface.go
+++ b/src/runtime/iface.go
@@ -195,6 +195,8 @@ func (m *itab) init() string {
        nt := int(x.mcount)
        xmhdr := (*[1 << 16]method)(add(unsafe.Pointer(x), uintptr(x.moff)))[:nt:nt]
        j := 0
+       methods := (*[1 << 16]unsafe.Pointer)(unsafe.Pointer(&m.fun[0]))[:ni:ni]
+       var fun0 unsafe.Pointer
 imethods:
        for k := 0; k < ni; k++ {
                i := &inter.mhdr[k]
@@ -216,7 +218,11 @@ imethods:
                                if tname.isExported() || pkgPath == ipkg {
                                        if m != nil {
                                                ifn := typ.textOff(t.ifn)
-                                               *(*unsafe.Pointer)(add(unsafe.Pointer(&m.fun[0]), uintptr(k)*sys.PtrSize)) = ifn
+                                               if k == 0 {
+                                                       fun0 = ifn // we'll set m.fun[0] at the end
+                                               } else {
+                                                       methods[k] = ifn
+                                               }
                                        }
                                        continue imethods
                                }
@@ -226,6 +232,7 @@ imethods:
                m.fun[0] = 0
                return iname
        }
+       m.fun[0] = uintptr(fun0)
        m.hash = typ.hash
        return ""
 }

diff --git a/test/fixedbugs/issue31419.go b/test/fixedbugs/issue31419.go
new file mode 100644 (file)
index 0000000..233111a
--- /dev/null
+++ b/test/fixedbugs/issue31419.go
@@ -0,0 +1,58 @@
+// run
+
+// Copyright 2019 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.
+
+// Issue 31419: race in getitab when two goroutines try
+// to do the same failed interface conversion.
+
+package main
+
+type T int
+
+func (t T) M() {}
+
+type I interface {
+       M()
+       M2()
+}
+
+var t T
+var e interface{} = &t
+var ok = false
+var ch = make(chan int)
+
+func main() {
+       _, ok = e.(I) // populate itab cache with a false result
+
+       go f() // get itab in a loop
+
+       var i I
+       for k := 0; k < 10000; k++ {
+               i, ok = e.(I) // read the cached itab
+               if ok {
+                       println("iteration", k, "i =", i, "&t =", &t)
+                       panic("conversion succeeded")
+               }
+       }
+       <-ch
+}
+
+func f() {
+       for i := 0; i < 10000; i++ {
+               f1()
+       }
+       ch <- 1
+}
+
+func f1() {
+       defer func() {
+               err := recover()
+               if err == nil {
+                       panic("did not panic")
+               }
+       }()
+       i := e.(I) // triggers itab.init, for getting the panic string
+       _ = i
+}