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
Closed

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

rfielding opened this issue Dec 20, 2016 · 9 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@rfielding
Copy link

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

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 ./...

@bradfitz bradfitz added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Dec 20, 2016
@rfielding
Copy link
Author

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

@rfielding
Copy link
Author

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

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 The path to resolution is known, but the work has not been done. and removed WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Dec 20, 2016
@bradfitz bradfitz added this to the Go1.8Maybe milestone Dec 20, 2016
@bradfitz
Copy link
Contributor

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

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

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

@ianlancetaylor
Copy link
Contributor

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

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

@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.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

4 participants