Navigation Menu

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

Jump to bottom

cmd/go/internal/modfetch: document known bug in isVendoredPackage #31562

Closed
haamed opened this issue Apr 19, 2019 · 6 comments
Closed

cmd/go/internal/modfetch: document known bug in isVendoredPackage #31562

haamed opened this issue Apr 19, 2019 · 6 comments
Labels
Documentation FrozenDueToAge modules NeedsFix The path to resolution is known, but the work has not been done. Unfortunate
Milestone
Go1.13

Comments

@haamed
Copy link

haamed commented Apr 19, 2019

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

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 += len("/vendor/")
} else {
return false
}
return strings.Contains(name[i:], "/")
}

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

@bcmills @jayconrod

@bcmills
Copy link
Contributor

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

bcmills commented Apr 19, 2019

At least the failure mode here is fairly benign: we know that len(name) is at least len("/vendor/"), so we might not chop off enough of the path, but we won't panic, and it won't cause a spurious true return 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.

@bcmills bcmills changed the title A bug in go module isVendoredPackage() cmd/go/internal/modfetch: isVendoredPackage returns false for some packages that are actually vendored Apr 19, 2019
@bcmills
Copy link
Contributor

bcmills commented 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.

@bcmills bcmills added this to the Go1.13 milestone Apr 19, 2019
@bcmills bcmills added the NeedsFix The path to resolution is known, but the work has not been done. label Apr 19, 2019
@bcmills bcmills changed the title cmd/go/internal/modfetch: isVendoredPackage returns false for some packages that are actually vendored cmd/go/internal/modfetch: document known bug in isVendoredPackage Apr 19, 2019
@gopherbot
Copy link

Change https://golang.org/cl/172977 mentions this issue: cmd/go/internal/modfetch: comment on known bug in isVendoredPackage

@bcmills bcmills self-assigned this Apr 19, 2019
@bep bep mentioned this issue Feb 24, 2020
Open
@gopherbot
Copy link

Change https://golang.org/cl/220657 mentions this issue: zip: document isVendoredPackage false-positives

gopherbot pushed a commit to golang/mod that referenced this issue Feb 24, 2020
zip: document isVendoredPackage false-positives
e5e73c1 
I had thought that the known bug in isVendoredPackage was strictly
conservative, but it turns out not to be.

Updates golang/go#37397
Updates golang/go#31562

Change-Id: I60f6062c41ec2d116766930f751d7df083535355
Reviewed-on: https://go-review.googlesource.com/c/mod/+/220657
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
@golang golang locked and limited conversation to collaborators Feb 23, 2021
@rsc rsc unassigned bcmills Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Documentation FrozenDueToAge modules NeedsFix The path to resolution is known, but the work has not been done. Unfortunate
Projects
None yet
Milestone
Go1.13
Development

No branches or pull requests
4 participants
@agnivade @haamed @bcmills @gopherbot