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

x/vuln/cmd/govulncheck: stack overflow #51560

Closed
hyangah opened this issue Mar 9, 2022 · 11 comments
Closed

x/vuln/cmd/govulncheck: stack overflow #51560

hyangah opened this issue Mar 9, 2022 · 11 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. vulncheck or vulndb Issues for the x/vuln or x/vulndb repo
Milestone

Comments

@hyangah
Copy link
Contributor

hyangah commented Mar 9, 2022

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

$ go version
go version devel go1.19-c6d9b38dd8 Wed Mar 9 06:51:37 2022 +0000 darwin/amd64

$ go version -m ~/go/bin/govulncheck
/Users/hakim/go/bin/govulncheck: devel go1.19-c6d9b38dd8 Wed Mar 9 06:51:37 2022 +0000
	path	golang.org/x/exp/vulndb/govulncheck
	mod	golang.org/x/exp/vulndb	v0.0.0-20220307200941-a1099baf94bf	h1:mCsuZcXtJ3XKXuc7Gmi7dstBeY5lzL+Ys4uZUvOFd/c=
	dep	golang.org/x/mod	v0.5.0	h1:UG21uOlmZabA4fW5i7ZX6bjw1xELEGg/ZLgZq9auk/Q=
	dep	golang.org/x/sys	v0.0.0-20210823070655-63515b42dcdf	h1:2ucpDCmfkl8Bd/FsLtiD653Wf96cW37s+iGx93zsu4k=
	dep	golang.org/x/tools	v0.0.0-20210910171127-a568412ca0e6	h1:O8E8PvYQ1HtZae5hPC9lyLiVuGlqL50yMQW7Y6oRaXQ=
	dep	golang.org/x/vuln	v0.0.0-20211109030331-63d5d8171d01	h1:aohfMrFbzRmVFllsufGifzVZzGJHuCrdmXA88J2FX9Q=
	dep	golang.org/x/xerrors	v0.0.0-20200804184101-5ec99f83aff1	h1:go1bK/D/BFZV2I8cIQd1NKEZ+0owSTG1fDTci4IqFcE=
	build	-compiler=gc
	build	CGO_ENABLED=1
	build	CGO_CFLAGS=
	build	CGO_CPPFLAGS=
	build	CGO_CXXFLAGS=
	build	CGO_LDFLAGS=
	build	GOARCH=amd64
	build	GOOS=darwin
	build	GOAMD64=v1

The problem exists with the govulncheck built with upgraded dependency for golang.org/x/tools

$ go version -m ~/go/bin/govulncheck
/Users/hakim/go/bin/govulncheck: devel go1.19-c6d9b38dd8 Wed Mar 9 06:51:37 2022 +0000
	path	golang.org/x/exp/vulndb/govulncheck
	mod	golang.org/x/exp/vulndb	(devel)	
	dep	golang.org/x/mod	v0.6.0-dev.0.20220106191415-9b9b3d81d5e3	h1:kQgndtyPBW/JIYERgdxfwMYh3AVStj88WQTlNDi2a+o=
	dep	golang.org/x/sys	v0.0.0-20211019181941-9d821ace8654	h1:id054HUawV2/6IGm2IV8KZQjqtwAOo2CYlOToYqa0d0=
	dep	golang.org/x/tools	v0.1.10-0.20220307161149-b59c441bdb9d	h1:8ZBB2K3kY243OYGD5lDI5iC32QRQgW7/bvZOsh4SX2U=
	dep	golang.org/x/vuln	v0.0.0-20211109030331-63d5d8171d01	h1:aohfMrFbzRmVFllsufGifzVZzGJHuCrdmXA88J2FX9Q=
	dep	golang.org/x/xerrors	v0.0.0-20200804184101-5ec99f83aff1	h1:go1bK/D/BFZV2I8cIQd1NKEZ+0owSTG1fDTci4IqFcE=
...

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/hakim/Library/Caches/go-build"
GOENV="/Users/hakim/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/hakim/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/hakim/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/Users/hakim/sdk/gotip"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/Users/hakim/sdk/gotip/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="devel go1.19-c6d9b38dd8 Wed Mar 9 06:51:37 2022 +0000"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/hakim/projects/editor/tools/go.mod"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/bw/6r6k9d113sv1_vvzk_1kfxbm001py5/T/go-build2866605994=/tmp/go-build -gno-record-gcc-switches -fno-common

What did you do?

$ git clone https://go.googlesource.com/tools
$ cd tools
$ git checkout gopls/v0.6.0
$ govulncheck -tests all

I am not yet sure what exactly triggers this recursion in go/callgraph/vta.
My repro case is a pathological case - running govulncheck with all can be unusual.
Anyhow, I think it's not a bad idea to limit the recursion.

What did you expect to see?

Succeed, or fail with user-friendly message.

What did you see instead?

$ govulncheck -tests all            
runtime: goroutine stack exceeds 1000000000-byte limit
runtime: sp=0xc115c00398 stack=[0xc115c00000, 0xc135c00000]
fatal error: stack overflow

runtime stack:
runtime.throw({0x1457d88?, 0x1711060?})
	/Users/hakim/sdk/gotip/src/runtime/panic.go:992 +0x71
runtime.newstack()
	/Users/hakim/sdk/gotip/src/runtime/stack.go:1101 +0x5cc
runtime.morestack()
	/Users/hakim/sdk/gotip/src/runtime/asm_amd64.s:547 +0x8b

goroutine 1 [running]:
go/types.(*Named).Underlying(0xc03ca36460?)
	/Users/hakim/sdk/gotip/src/go/types/named.go:222 +0x3a fp=0xc115c003a8 sp=0xc115c003a0 pc=0x12e181a
golang.org/x/tools/go/callgraph/vta.interfaceUnderPtr({0x1501cf8?, 0xc03ca36460?})
	/Users/hakim/go/pkg/mod/golang.org/x/[email protected]/go/callgraph/vta/utils.go:88 +0x2b fp=0xc115c003d0 sp=0xc115c003a8 pc=0x137cf4b
golang.org/x/tools/go/callgraph/vta.interfaceUnderPtr({0x1501cf8?, 0xc03ca36460?})
	/Users/hakim/go/pkg/mod/golang.org/x/[email protected]/go/callgraph/vta/utils.go:97 +0x7e fp=0xc115c003f8 sp=0xc115c003d0 pc=0x137cf9e
golang.org/x/tools/go/callgraph/vta.interfaceUnderPtr({0x1501cf8?, 0xc03ca36460?})
...
golang.org/x/tools/go/callgraph/vta.interfaceUnderPtr({0x1501cf8?, 0xc03ca36460?})
	/Users/hakim/go/pkg/mod/golang.org/x/[email protected]/go/callgraph/vta/utils.go:97 +0x7e fp=0xc115c012f8 sp=0xc115c012d0 pc=0x137cf9e
golang.org/x/tools/go/callgraph/vta.interfaceUnderPtr({0x1501cf8?, 0xc03ca36460?})
	/Users/hakim/go/pkg/mod/golang.org/x/[email protected]/go/callgraph/vta/utils.go:97 +0x7e fp=0xc115c01320 sp=0xc115c012f8 pc=0x137cf9e
...additional frames elided...
...

cc @zpavlinovic

@gopherbot gopherbot added this to the Unreleased milestone Mar 9, 2022
@zpavlinovic zpavlinovic self-assigned this Mar 9, 2022
@zpavlinovic zpavlinovic added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 9, 2022
@zpavlinovic
Copy link
Contributor

Does gopls/v0.6.0 perhaps use generics?

@hyangah
Copy link
Contributor Author

hyangah commented Mar 10, 2022

Based on https://github.com/golang/tools/blob/gopls/v0.6.0/go.mod
I guess x/tools didn't use generics back then. (gopls/v0.6.0 tag corresponds to 2037813)

But it's possible some dependencies brought in generics code (note: I used all target. govulncheck with ./... was ok)

@zpavlinovic
Copy link
Contributor

Ok, will look into this next week. This is definitely unexpected, but likely has a simple fix. Let me know if this is should be a priority.

@zpavlinovic
Copy link
Contributor

Tried to reproduce this today. I got a different error.

panic: T

goroutine 31473 [running]:
golang.org/x/tools/go/types/typeutil.Hasher.hashFor({0x9665a0?}, {0xa8d790?, 0xc0704d6f30?})
        /usr/local/google/home/zpavlinovic/go/pkg/mod/golang.org/x/[email protected]/go/types/typeutil/map.go:302 +0x41f
golang.org/x/tools/go/types/typeutil.Hasher.Hash({0xa8d6a0?}, {0xa8d790, 0xc0704d6f30})
        /usr/local/google/home/zpavlinovic/go/pkg/mod/golang.org/x/[email protected]/go/types/typeutil/map.go:226 +0x65
golang.org/x/tools/go/types/typeutil.Hasher.hashFor({0x9665a0?}, {0xa8d740?, 0xc0704d6f90?})
        /usr/local/google/home/zpavlinovic/go/pkg/mod/golang.org/x/[email protected]/go/types/typeutil/map.go:264 +0x539
golang.org/x/tools/go/types/typeutil.Hasher.Hash({0x968be0?}, {0xa8d740, 0xc0704d6f90})
        /usr/local/google/home/zpavlinovic/go/pkg/mod/golang.org/x/[email protected]/go/types/typeutil/map.go:226 +0x65
golang.org/x/tools/go/types/typeutil.(*Map).At(0xc0348d6b98, {0xa8d740, 0xc0704d6f90})
        /usr/local/google/home/zpavlinovic/go/pkg/mod/golang.org/x/[email protected]/go/types/typeutil/map.go:88 +0x48
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc0348d6b40, {0xa8d740?, 0xc0704d6f90?}, 0x1)
        /usr/local/google/home/zpavlinovic/go/pkg/mod/golang.org/x/[email protected]/go/ssa/methods.go:156 +0x66
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc0348d6b40, {0xa8d6a0?, 0xc0704c9300?}, 0x0)
        /usr/local/google/home/zpavlinovic/go/pkg/mod/golang.org/x/[email protected]/go/ssa/methods.go:221 +0x588
golang.org/x/tools/go/ssa.(*Program).needMethodsOf(0xc0348d6b40, {0xa8d6a0?, 0xc0704c9300?})
        /usr/local/google/home/zpavlinovic/go/pkg/mod/golang.org/x/[email protected]/go/ssa/methods.go:145 +0x70
golang.org/x/tools/go/ssa.(*Package).build(0xc031a4e1e0)
        /usr/local/google/home/zpavlinovic/go/pkg/mod/golang.org/x/[email protected]/go/ssa/builder.go:2281 +0x111
sync.(*Once).doSlow(0xc06ce66c70?, 0xc06ced09f0?)
        /usr/lib/google-golang/src/sync/once.go:68 +0xc2
sync.(*Once).Do(...)
        /usr/lib/google-golang/src/sync/once.go:59
golang.org/x/tools/go/ssa.(*Package).Build(...)
        /usr/local/google/home/zpavlinovic/go/pkg/mod/golang.org/x/[email protected]/go/ssa/builder.go:2269
golang.org/x/tools/go/ssa.(*Program).Build.func1(0x0?)
        /usr/local/google/home/zpavlinovic/go/pkg/mod/golang.org/x/[email protected]/go/ssa/builder.go:2253 +0x4c
created by golang.org/x/tools/go/ssa.(*Program).Build
        /usr/local/google/home/zpavlinovic/go/pkg/mod/golang.org/x/[email protected]/go/ssa/builder.go:2252 +0x19c

My best guess is this has something to do with the way generics and ssa interplay.

@zpavlinovic zpavlinovic changed the title x/exp/vulndb/govulncheck: stack overflow x/vuln/cmd/govulncheck: stack overflow Mar 24, 2022
@gopherbot gopherbot added the vulncheck or vulndb Issues for the x/vuln or x/vulndb repo label Mar 24, 2022
@zpavlinovic
Copy link
Contributor

Note: the new version of govulncheck is now in x/vuln/cmd/govulncheck. The previous version is not supported anymore and has been deleted.

@zpavlinovic
Copy link
Contributor

I gotiped the latest version of Go and used the latest version of x/tools

go version -m govulncheck
govulncheck: devel go1.19-5bb2628c Wed Apr 6 03:13:34 2022 +0000
	path	golang.org/x/vuln/cmd/govulncheck
	mod	golang.org/x/vuln	(devel)	
	dep	golang.org/x/mod	v0.6.0-dev.0.20220106191415-9b9b3d81d5e3	h1:kQgndtyPBW/JIYERgdxfwMYh3AVStj88WQTlNDi2a+o=
	dep	golang.org/x/sys	v0.0.0-20211213223007-03aa0b5f6827	h1:A0Qkn7Z/n8zC1xd9LTw17AiKlBRK64tw3ejWQiEqca0=
	dep	golang.org/x/tools	v0.1.10-0.20220406142415-74cea6e8cb44	h1:joLUJEk8wdt8AeCBOXTHIX7INd4Os05+0v17R5ZXmHQ=
	dep	golang.org/x/xerrors	v0.0.0-20200804184101-5ec99f83aff1	h1:go1bK/D/BFZV2I8cIQd1NKEZ+0owSTG1fDTci4IqFcE=
	build	-compiler=gc
	build	CGO_ENABLED=1
	build	CGO_CFLAGS=
	build	CGO_CPPFLAGS=
	build	CGO_CXXFLAGS=
	build	CGO_LDFLAGS=
	build	GOARCH=amd64
	build	GOOS=linux
	build	GOAMD64=v1
	build	vcs=git
	build	vcs.revision=38ae2dcb071e3b82a45c6541507e635a1006af05
	build	vcs.time=2022-04-05T17:50:34Z
	build	vcs.modified=true

I can see the error, but it is yet a different one

govulncheck -tests all
panic: T

goroutine 29544 [running]:
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc056ae44e0, {0xa13348?, 0xc04e77a600?}, 0x0)
	/usr/local/google/home/zpavlinovic/go/pkg/mod/golang.org/x/[email protected]/go/ssa/methods.go:237 +0x5b1
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc056ae44e0, {0xa132f8?, 0xc04e77a660?}, 0x1)
	/usr/local/google/home/zpavlinovic/go/pkg/mod/golang.org/x/[email protected]/go/ssa/methods.go:228 +0x669
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc056ae44e0, {0xa13258?, 0xc04e751e30?}, 0x0)
	/usr/local/google/home/zpavlinovic/go/pkg/mod/golang.org/x/[email protected]/go/ssa/methods.go:221 +0x588
golang.org/x/tools/go/ssa.(*Program).needMethodsOf(0xc056ae44e0, {0xa13258?, 0xc04e751e30?})
	/usr/local/google/home/zpavlinovic/go/pkg/mod/golang.org/x/[email protected]/go/ssa/methods.go:145 +0x70
golang.org/x/tools/go/ssa.(*Package).build(0xc0802ba9c0)
	/usr/local/google/home/zpavlinovic/go/pkg/mod/golang.org/x/[email protected]/go/ssa/builder.go:2295 +0x111
sync.(*Once).doSlow(0xc003982fb8?, 0x7d5a1e?)
	/usr/local/google/home/zpavlinovic/sdk/gotip/src/sync/once.go:67 +0xc2
sync.(*Once).Do(...)
	/usr/local/google/home/zpavlinovic/sdk/gotip/src/sync/once.go:58
golang.org/x/tools/go/ssa.(*Package).Build(...)
	/usr/local/google/home/zpavlinovic/go/pkg/mod/golang.org/x/[email protected]/go/ssa/builder.go:2283
golang.org/x/tools/go/ssa.(*Program).Build.func1(0x0?)
	/usr/local/google/home/zpavlinovic/go/pkg/mod/golang.org/x/[email protected]/go/ssa/builder.go:2267 +0x4c
created by golang.org/x/tools/go/ssa.(*Program).Build
	/usr/local/google/home/zpavlinovic/go/pkg/mod/golang.org/x/[email protected]/go/ssa/builder.go:2266 +0x19c

@taking, should we wait for upcoming ssa-generics changes before looking deeper? (Note, the above call stack is suggesting that T is a type parameter that appears as a Field of a named struct.)

@timothy-king
Copy link
Contributor

I'm @timothy-king on github. Apologies to @taking.

I would wait. The panic is now blocked on #48525.

@zpavlinovic
Copy link
Contributor

I'm @timothy-king on github. Apologies to @taking.

Oops, my apologies too.

@timothy-king
Copy link
Contributor

#48525 should now be far enough along to retest this.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/399674 mentions this issue: go/callgraph/vta: avoids cycles for pathological recursive types

gopherbot pushed a commit to golang/tools that referenced this issue Apr 12, 2022
Helper type logic did not handle properly pathological recursive types
such as type B *B. This CL addresses that.

For golang/go#51560

Change-Id: Ibb69ac68d77fc3956b98c701f17d28b9f30ac22c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/399674
Reviewed-by: Tim King <[email protected]>
Run-TryBot: Zvonimir Pavlinovic <[email protected]>
Auto-Submit: Zvonimir Pavlinovic <[email protected]>
gopls-CI: kokoro <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
@zpavlinovic
Copy link
Contributor

I believe this has been addressed. Closing.

@golang golang locked and limited conversation to collaborators Sep 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. vulncheck or vulndb Issues for the x/vuln or x/vulndb repo
Projects
Status: No status
Development

No branches or pull requests

4 participants