Repository navigation
cmd/go/internal/modfetch: document known bug in isVendoredPackage #31562
Description
What version of Go are you using (go version)?
$ go version go version go1.12.1 linux/amd64
Does this issue reproduce with the latest release?
Yes
What operating system and processor architecture are you using (go env)?
go env Output
$ go env GOARCH="amd64" GOBIN="/home/gheibi/.go/bin/" GOCACHE="/home/gheibi/.cache/go-build" GOEXE="" GOFLAGS="" GOHOSTARCH="amd64" GOHOSTOS="linux" GOOS="linux" GOPATH="/home/gheibi/.go/" GOPROXY="" GORACE="" GOROOT="/local/go1.12.1.linux_amd64/go" GOTMPDIR="" GOTOOLDIR="/local/go1.12.1.linux_amd64/go /pkg/tool/linux_amd64" GCCGO="gccgo" CC="gcc" CXX="g++" CGO_ENABLED="1" GOMOD="" CGO_CFLAGS="-g -O2" CGO_CPPFLAGS="" CGO_CXXFLAGS="-g -O2" CGO_FFLAGS="-g -O2" CGO_LDFLAGS="-g -O2" PKG_CONFIG="pkg-config" GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build865790926=/tmp/go-build"
What did you do?
$grep -A12 "func isVendoredPackage" $GOROOT/src/cmd/go/internal/modfetch/coderepo.go
go/src/cmd/go/internal/modfetch/coderepo.go
Lines 632 to 642 in c8aaec2
What did you expect to see?
func isVendoredPackage(name string) bool {
var i int
if strings.HasPrefix(name, "vendor/") {
i += len("vendor/")
} else if j := strings.Index(name, "/vendor/"); j >= 0 {
i += j + len("/vendor/")
} else {
return false
}
return strings.Contains(name[i:], "/")
}
What did you see instead?
The function is not documented, but it is obvious that it is not doing what the intention is.
It seems that the intention is to match against this regex: (^vendor/|/vendor/).*/
But in fact it does random match by skipping first 8 bytes in case the input contains /vendor/.
The fix is to add the j to the i as states above.
It is quite important to fix it early on, because it may cause go-modules with different hashes and a late fix can cause more hash mismatch of the same content (defy the immutability logic).
Activity
agnivade commentedon Apr 19, 2019
@bcmills @jayconrod
bcmills commentedon Apr 19, 2019
That sure looks like a bug — it should probably be
i = j + len("/vendor/").Unfortunately, at this point we can't safely make any changes to the extracted contents of modules; see #30369 and #29278.
bcmills commentedon Apr 19, 2019
At least the failure mode here is fairly benign: we know that
len(name)is at leastlen("/vendor/"), so we might not chop off enough of the path, but we won't panic, and it won't cause a spurioustruereturn for packages that aren't actually vendored.So the net effect is that we'll include some packages that aren't strictly necessary, but fail to prune some contents that really aren't needed.
[-]A bug in go module isVendoredPackage()[/-][+]cmd/go/internal/modfetch: `isVendoredPackage` returns false for some packages that are actually vendored[/+]bcmills commentedon Apr 19, 2019
As @rsc put it in #30369 (comment):
“Now the algorithm is the algorithm. Let's leave it there. Any bugs that remain are now features.”
Since this error won't break builds (it includes packages that should be excluded, but does not exclude packages that should be included), it doesn't seem worth the churn of fixing at this time. We can revisit if we discover more severe bugs that are worth breaking the hash function over; for now, we should just document the known issue in the code.
[-]cmd/go/internal/modfetch: `isVendoredPackage` returns false for some packages that are actually vendored[/-][+]cmd/go/internal/modfetch: document known bug in `isVendoredPackage`[/+]gopherbot commentedon Apr 19, 2019
Change https://golang.org/cl/172977 mentions this issue:
cmd/go/internal/modfetch: comment on known bug in isVendoredPackagegopherbot commentedon Feb 24, 2020
Change https://golang.org/cl/220657 mentions this issue:
zip: document isVendoredPackage false-positiveszip: document isVendoredPackage false-positives
matloob commentedon Jun 18, 2024
@samthanawalla I think post-1.21 we can now fix this, but only on modules declaring a new enough version of go.
dmitshur commentedon Jun 18, 2024
This issue from 2019 was resolved and frozen due to age. For new work, maybe it'd be better to file a new issue and refer to this one.
matloob commentedon Jun 18, 2024
Oops! Closing