Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

runtime: race in getitab (itab.init) #31419

Closed
cherrymui opened this issue Apr 11, 2019 · 1 comment
Closed

runtime: race in getitab (itab.init) #31419

cherrymui opened this issue Apr 11, 2019 · 1 comment

Comments

@cherrymui
Copy link
Member

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
Copy link

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

@golang golang locked and limited conversation to collaborators Apr 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants