runtime: race in getitab (itab.init)

[cherrymui]

What version of Go are you using (go version)?

tip (3cb92fc)

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

darwin/amd64, also linux/amd64

What did you do?

https://play.golang.org/p/3QdnC62q1Vb

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
}

What did you expect to see?

Not panic.

We do conversion of e (containing &t) to I concurrently, one with ",ok" form (in f1), one without (in main). T doesn't implement I, so the conversion should fail, and the program should not panic.

What did you see instead?

Panics.

$ go run i.go
iteration 2778 i = (0x1161008,0x10ddc98) &t = 0x10ddc98
panic: conversion succeeded

goroutine 1 [running]:
main.main()
	/tmp/i.go:27 +0x1a2
exit status 2

$ go run i.go
iteration 1681 i = (0x1161008,0x10ddc98) &t = 0x10ddc98
panic: conversion succeeded

goroutine 1 [running]:
main.main()
	/tmp/i.go:27 +0x1a2
exit status 2

After some iterations, the conversion in main succeeded, with an I interface containing an &t, which is bad.

I think what happens here is:

• In the first conversion (early in main), we populate the itab cache with a false result.
• In f1, we get the same itab from the cache. As this is not ",ok" form, getitab calls m.init to find out what method is missing for the panic message. m.init writes m.fun[0] to T.M's code pointer as it sees the M method. Before return it sets m.fun[0] = 0 as it couldn't find the M2 method. There is a small window that m.fun[0] is not 0.
• In the loop in main, we get the same itab from the cache, check its m.fun[0] field in getitab. If this happens in the middle of the window that m.fun[0] is not 0, it thinks the conversion succeeded.

I think we can fix this by not setting m.fun[0] immediately when we find the first method. Instead, store the code pointer in a variable, and set m.fun[0] only after we know the conversion succeeds. Will send a fix.

Found this while I'm porting itab caching to gccgo.

cc @aclements @randall77

[gopherbot]

Change https://golang.org/cl/171759 mentions this issue: runtime: set itab.fun[0] only on successful conversion