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

cmd/vet: crash of go vet involving cgo #18389

Closed
rfielding opened this issue Dec 20, 2016 · 9 comments

Comments

Projects
None yet
4 participants
@rfielding
Copy link

commented Dec 20, 2016

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

go version go1.7.4 darwin/amd64

Robs-MBP:object-drive-server rfielding$ go env
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/rfielding/gocode"
GORACE=""
GOROOT="/Users/rfielding/go1.7.4"
GOTOOLDIR="/Users/rfielding/go1.7.4/pkg/tool/darwin_amd64"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/nk/ftxlzcmx5yg0v6bnkm940zlh0000gn/T/go-build124531480=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"

go vet crashes with an infinite recursion in cgo.go:

go vet ./...
...

runtime: goroutine stack exceeds 1000000000-byte limit
fatal error: stack overflow

runtime stack:
runtime.throw(0x2d130d, 0xe)
	/usr/local/go/src/runtime/panic.go:566 +0x95
runtime.newstack()
	/usr/local/go/src/runtime/stack.go:1061 +0x416
runtime.morestack()
	/usr/local/go/src/runtime/asm_amd64.s:366 +0x7f

goroutine 1 [running]:
main.typeOKForCgoCall(0x3f0b20, 0xc4204b8560, 0xc4203ee660)
	/usr/local/go/src/cmd/vet/cgo.go:111 fp=0xc4407002b8 sp=0xc4407002b0
main.typeOKForCgoCall(0x3f0ae0, 0xc42049c360, 0xc4204b8590)
	/usr/local/go/src/cmd/vet/cgo.go:124 +0x104 fp=0xc4407002f8 sp=0xc4407002b8
main.typeOKForCgoCall(0x3f0b20, 0xc4204b8590, 0xc4203ee601)
	/usr/local/go/src/cmd/vet/cgo.go:119 +0x160 fp=0xc440700338 sp=0xc4407002f8
...
main.typeOKForCgoCall(0x3f0ae0, 0xc42049c360, 0xc4204b8590)
	/usr/local/go/src/cmd/vet/cgo.go:124 +0x104 fp=0xc440701b78 sp=0xc440701b38
...additional frames elided...
exit status 2
vendor/gopkg.in/yaml.v2/decode.go:123: unreachable code
vendor/gopkg.in/yaml.v2/emitterc.go:669: unreachable code
vendor/gopkg.in/yaml.v2/parserc.go:169: unreachable code
exit status 1

This code uses the spacemonkey/openssl library. The error happens during vet in cgo.go:

	// typeOKForCgoCall returns true if the type of arg is OK to pass to a
   109	// C function using cgo. This is not true for Go types with embedded
   110	// pointers.
   111	func typeOKForCgoCall(t types.Type) bool {
   112		if t == nil {
   113			return true
   114		}
   115		switch t := t.Underlying().(type) {
   116		case *types.Chan, *types.Map, *types.Signature, *types.Slice:
   117			return false
   118		case *types.Pointer:
   119			return typeOKForCgoCall(t.Elem())
   120		case *types.Array:
   121			return typeOKForCgoCall(t.Elem())
   122		case *types.Struct:
   123			for i := 0; i < t.NumFields(); i++ {
   124				if !typeOKForCgoCall(t.Field(i).Type()) {
   125					return false
   126				}
   127			}
   128		}
   129		return true
   130	}
   131	

This seems plausible if t has a t.Field(i).Type() reaches the same type. In my case, it reaches the same type with period 2.

main.typeOKForCgoCall(0x3f0b20, 0xc420493f30, 0xc42046d001)
	/usr/local/go/src/cmd/vet/cgo.go:119 +0x160 fp=0xc440701a38 sp=0xc4407019f8
main.typeOKForCgoCall(0x3f0ae0, 0xc42049cc60, 0xc420493f30)
	/usr/local/go/src/cmd/vet/cgo.go:124 +0x104 fp=0xc440701a78 sp=0xc440701a38
main.typeOKForCgoCall(0x3f0b20, 0xc420493f30, 0xc42046d001)
	/usr/local/go/src/cmd/vet/cgo.go:119 +0x160 fp=0xc440701ab8 sp=0xc440701a78
@bradfitz

This comment has been minimized.

Copy link
Member

commented Dec 20, 2016

With Go 1.8 coming out soon, we're no longer making changes to the Go 1.7 codebase. Can you check whether this bug is present in Go 1.8?

$ go get golang.org/x/build/version/go1.8beta2
$ go1.8beta2 download
$ go1.8beta2 vet ./...
@rfielding

This comment has been minimized.

Copy link
Author

commented Dec 20, 2016

yes. i am getting 1.8 to build from source. this looks like an easy change.

@rfielding

This comment has been minimized.

Copy link
Author

commented Dec 20, 2016

I'm editing origin https://go.googlesource.com/go (which I need to verify is the absolute latest code). It is alternating between (names) openssl.Certificate and *openssl.Certificate (with types *type.Pointer and *type.Struct) in the infinite recursion on the type being passed in. right now, trying to come up with an appropriate pull request.

@rfielding

This comment has been minimized.

Copy link
Author

commented Dec 20, 2016

and I tried it with the go1.8beta2 binary as well:

Robs-MacBook-Pro:object-drive-server rfielding$ go1.8beta2 vet ./...
vendor/github.com/c9s/goprocinfo/linux/netstat.go:98: struct field TCPOFODrop repeats json tag "tcp_ofo_drop" also at vendor/github.com/c9s/goprocinfo/linux/netstat.go:97
exit status 1
runtime: goroutine stack exceeds 1000000000-byte limit
fatal error: stack overflow

runtime stack:
runtime.throw(0x12d37e9, 0xe)
	/usr/local/go/src/runtime/panic.go:596 +0x95
runtime.newstack(0x0)
	/usr/local/go/src/runtime/stack.go:1089 +0x3f2
runtime.morestack()
	/usr/local/go/src/runtime/asm_amd64.s:385 +0x86

goroutine 1 [running]:
main.typeOKForCgoCall(0x13efb20, 0xc4203cb920, 0xc420156c30)
	/usr/local/go/src/cmd/vet/cgo.go:116 +0x2d2 fp=0xc440800360 sp=0xc440800358
main.typeOKForCgoCall(0x13efae0, 0xc420156b10, 0xc4203cb950)
	/usr/local/go/src/cmd/vet/cgo.go:129 +0xf4 fp=0xc440800398 sp=0xc440800360
main.typeOKForCgoCall(0x13efb20, 0xc4203cb950, 0xc420156c01)
	/usr/local/go/src/cmd/vet/cgo.go:124 +0x158 fp=0xc4408003d0 sp=0xc440800398

@bradfitz bradfitz changed the title crash of go vet involving cgo cmd/vet: crash of go vet involving cgo Dec 20, 2016

@bradfitz bradfitz added NeedsFix and removed WaitingForInfo labels Dec 20, 2016

@bradfitz bradfitz added this to the Go1.8Maybe milestone Dec 20, 2016

@bradfitz

This comment has been minimized.

Copy link
Member

commented Dec 20, 2016

I'll let @ianlancetaylor triage this for Go1.8 vs Go1.9.

rfielding pushed a commit to rfielding/go that referenced this issue Dec 21, 2016

Rob Fielding
go vet crashes when invoked on cgo code using spacemonkey openssl
When running go vet on a code base using cgo, we get infinite recursion during the check:
```
go vet ./...
```

The same type parameter goes in (0x3f0b20), and infinite recursion ensues:

```
main.typeOKForCgoCall(0x3f0b20, 0xc420493f30, 0xc42046d001)
	/usr/local/go/src/cmd/vet/cgo.go:119 +0x160 fp=0xc440701a38 sp=0xc4407019f8
main.typeOKForCgoCall(0x3f0ae0, 0xc42049cc60, 0xc420493f30)
	/usr/local/go/src/cmd/vet/cgo.go:124 +0x104 fp=0xc440701a78 sp=0xc440701a38
main.typeOKForCgoCall(0x3f0b20, 0xc420493f30, 0xc42046d001)
	/usr/local/go/src/cmd/vet/cgo.go:119 +0x160 fp=0xc440701ab8 sp=0xc440701a78
```

These pointers in this case happen to be a `openssl.Certificate` and a `*openssl.Certificate`,
with Underlying() of `*type.Pointer` and `*type.Struct`.

This function either returns immediately when the answer is yes,
or immediately return no, or recurse when the answer is possibly no.
So if we see the same type twice in the list of parents from which
the recursion began, then the answer must be no - because it will never
change to a yes.

Fixes golang#18389
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2016

@rfielding Thanks, but please don't send us patches except through Gerrit, since Gerrit ensures that we have the right to use the patch.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2016

@bradfitz Looks like CL 32754 may have caused cmd/vet to stop running the cgo tests. Hmmm.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2016

I won't insist on it, but I think it would be better to fix this for 1.8 since it's fairly annoying to have vet crash on valid code and there is no sensible workaround.

@gopherbot

This comment has been minimized.

Copy link

commented Dec 21, 2016

CL https://golang.org/cl/34660 mentions this issue.

@gopherbot gopherbot closed this in 27fb26c Dec 21, 2016

@golang golang locked and limited conversation to collaborators Dec 21, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.