From 8d86ef221631757ef4d89401947db674c730f94e Mon Sep 17 00:00:00 2001
From: Cherry Zhang <cherryyz@google.com>
Date: Thu, 11 Apr 2019 14:00:07 -0400
Subject: [PATCH] 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>
---
 src/runtime/iface.go         |  9 +++++-
 test/fixedbugs/issue31419.go | 58 ++++++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+), 1 deletion(-)
 create mode 100644 test/fixedbugs/issue31419.go

diff --git a/src/runtime/iface.go b/src/runtime/iface.go
index 246b63b897..bb4eccc9bd 100644
--- 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
index 0000000000..233111ae14
--- /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
+}
-- 
2.50.0