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/go: go get panics in GOPATH mode on custom import path with an insecure redirect #34049

Closed
hyangah opened this issue Sep 3, 2019 · 8 comments

Comments

@hyangah
Copy link
Contributor

commented Sep 3, 2019

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

$ go version
go version go1.13 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
GO111MODULE="off"
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/hakim/.cache/go-build"
GOENV="/home/hakim/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/tmp/tmp.cdbUwrI8dp"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GCCGO="gccgo"
AR="ar"
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-build162582877=/tmp/go-build -gno-record-gcc-switches"

What did you do?

GO111MODULE=off go get

What did you expect to see?

Successful go get execution

What did you see instead?

Panic.

$ GO111MODULE=off go get -x -v github.com/v2ray/v2ray-core
github.com/v2ray/v2ray-core (download)
cd .
git clone -- https://github.com/v2ray/v2ray-core /tmp/tmp.cdbUwrI8dp/src/github.com/v2ray/v2ray-core
cd /tmp/tmp.cdbUwrI8dp/src/github.com/v2ray/v2ray-core
git submodule update --init --recursive
cd /tmp/tmp.cdbUwrI8dp/src/github.com/v2ray/v2ray-core
git show-ref
cd /tmp/tmp.cdbUwrI8dp/src/github.com/v2ray/v2ray-core
git submodule update --init --recursive
github.com/golang/protobuf (download)
cd .
git clone -- https://github.com/golang/protobuf /tmp/tmp.cdbUwrI8dp/src/github.com/golang/protobuf
cd /tmp/tmp.cdbUwrI8dp/src/github.com/golang/protobuf
git submodule update --init --recursive
cd /tmp/tmp.cdbUwrI8dp/src/github.com/golang/protobuf
git show-ref
cd /tmp/tmp.cdbUwrI8dp/src/github.com/golang/protobuf
git submodule update --init --recursive
# get https://v2ray.com/core/common?go-get=1
# get //v2ray.com/core/common?go-get=1: 200 OK (0.286s)
get "v2ray.com/core/common": found meta tag get.metaImport{Prefix:"v2ray.com/core", VCS:"git", RepoRoot:"https://github.com/v2ray/v2ray-core"} at //v2ray.com/core/common?go-get=1
get "v2ray.com/core/common": verifying non-authoritative meta tag
# get https://v2ray.com/core?go-get=1
# get //v2ray.com/core?go-get=1: Get http://www.v2ray.com/core/?go-get=1: redirected from secure URL https://v2ray.com/core?go-get=1 to insecure URL http://www.v2ray.com/core/?go-get=1
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x8 pc=0x889e7d]

goroutine 1 [running]:
cmd/go/internal/get.metaImportsForPrefix.func2(0x0, 0x0, 0xb27d20, 0xc0005002d0)
	/usr/local/go/src/cmd/go/internal/get/vcs.go:907 +0x29d
internal/singleflight.(*Group).doCall(0xeabf60, 0xc0001f4320, 0xc0003c00c0, 0xe, 0xc0002972f8)
	/usr/local/go/src/internal/singleflight/singleflight.go:95 +0x2e
internal/singleflight.(*Group).Do(0xeabf60, 0xc0003c00c0, 0xe, 0xc0002972f8, 0x1, 0xc0, 0x41, 0x0, 0xbf53d46af9677368)
	/usr/local/go/src/internal/singleflight/singleflight.go:63 +0x1ba
cmd/go/internal/get.metaImportsForPrefix(0xc0003c00c0, 0xe, 0x0, 0x0, 0x1, 0xc0003c00c0, 0xe, 0xc0003c00cf, 0x3, 0xc0003c00d3)
	/usr/local/go/src/cmd/go/internal/get/vcs.go:893 +0xf1
cmd/go/internal/get.repoRootForImportDynamic(0xc00023afa1, 0x15, 0x0, 0x0, 0x0, 0x0, 0x0)
	/usr/local/go/src/cmd/go/internal/get/vcs.go:827 +0x665
cmd/go/internal/get.RepoRootForImportPath(0xc00023afa1, 0x15, 0x0, 0x0, 0xffffffffffffffff, 0xc0002976c0, 0xc000297838)
	/usr/local/go/src/cmd/go/internal/get/vcs.go:662 +0x345
cmd/go/internal/get.downloadPackage(0xc00036db00, 0xc000121200, 0xc00023afa1)
	/usr/local/go/src/cmd/go/internal/get/get.go:448 +0x1568
cmd/go/internal/get.download(0xc00023afa1, 0x15, 0xc000286000, 0xc000297d78, 0x0)
	/usr/local/go/src/cmd/go/internal/get/get.go:275 +0xd26
cmd/go/internal/get.download(0x7ffe995e01c6, 0x1b, 0x0, 0xc000297d78, 0x0)
	/usr/local/go/src/cmd/go/internal/get/get.go:371 +0x718
cmd/go/internal/get.runGet(0xea3a40, 0xc0000c2040, 0x1, 0x1)
	/usr/local/go/src/cmd/go/internal/get/get.go:162 +0x170
main.main()
	/usr/local/go/src/cmd/go/main.go:189 +0x57f

Note: this works in GO111MODULE=on

@dmitshur dmitshur added this to the Go1.14 milestone Sep 3, 2019

@dmitshur

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

A problem is here:

resp, err := web.Get(security, url)
if err != nil {
return setCache(fetchResult{url: url, err: fmt.Errorf("fetch %s: %v", resp.URL, err)})
}

Accessing resp.URL when err != nil is not a valid action, because resp may be nil. It should likely be url instead (the parameter to web.Get).

Luckily, this is a problem that will only occur when web.Get fails. That means it will not happen during normal execution.

Edit: There is another problem, see my comment below.
Edit 2: It turned out not be a problem but expected new more secure behavior of Go 1.13. See issue #29591. I've updated my comment below.

/cc @bcmills @jayconrod

@dmitshur dmitshur added NeedsInvestigation and removed NeedsFix labels Sep 3, 2019

@dmitshur

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

That seems to be only a part of the problem.

Edit: Go 1.12.9 behavior here was sub-optimal. See issue #29591. Go 1.13 behavior of rejecting the redirect to plain-HTTP without -insecure flag is better. So this issue was just the panic in the error handling.

With Go 1.12.9, the entire command succeeds:

$ export GO111MODULE=off
$ export GOPATH=$(mktemp -d)
$ go get v2ray.com/core/common/serial
[panic]

$ export GOPATH=$(mktemp -d)
$ go1.12.9 get v2ray.com/core/common/serial

I picked v2ray.com/core/common/serial because it's a package with fewer dependencies that still reproduces the problem.

@dmitshur

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

As @heschik pointed out to me, one of the properties that makes the v2ray.com/core/common/serial custom import path more unusual is that it involves a redirect:

$ curl -i 'https://v2ray.com/core/common/serial?go-get=1'
HTTP/2 301
[...]
location: https://www.v2ray.com/core/index.html?go-get=1
[...]

In the original log, a part that stands out is:

# get https://v2ray.com/core/common?go-get=1
# get //v2ray.com/core/common?go-get=1: 200 OK (0.286s)

One possible explanation is that something is causing the URL scheme to get dropped, which then makes it default to http (instead of https), causing "redirected from secure URL https://v2ray.com/core?go-get=1 to insecure URL http://www.v2ray.com/core/?go-get=1" , then web.Get fails, and the panic happens when constructing an error message.

Some evidence that supports the above is that using the -insecure flag makes it work:

$ export GOPATH=$(mktemp -d)
$ go get -insecure -d v2ray.com/core/common/serial
$ echo $?
0

@dmitshur dmitshur changed the title cmd/go: go get panics in GOPATH mode cmd/go: go get fails in GOPATH mode on custom import path with a redirect Sep 3, 2019

@bcmills bcmills self-assigned this Sep 4, 2019

@gopherbot

This comment has been minimized.

Copy link

commented Sep 4, 2019

Change https://golang.org/cl/193259 mentions this issue: cmd/go/internal/get: avoid panic in metaImportsForPrefix if web.Get fails

@bcmills

This comment has been minimized.

Copy link
Member

commented Sep 4, 2019

@gopherbot, please backport to 1.13. This panic makes insecure redirects much harder to diagnose, and the fix is trivial.

@gopherbot

This comment has been minimized.

Copy link

commented Sep 4, 2019

Backport issue(s) opened: #34081 (for 1.13).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

@bcmills

This comment has been minimized.

Copy link
Member

commented Sep 4, 2019

With CL 193259 patched in, the reason for the failure becomes clearer:

example.com$ GO111MODULE=off go get -d v2ray.com/core/common/serial
package v2ray.com/core/common/serial: unrecognized import path "v2ray.com/core/common/serial" (fetching v2ray.com/core: Get "http://www.v2ray.com/core/?go-get=1": redirected from secure URL https://v2ray.com/core?go-get=1 to insecure URL http://www.v2ray.com/core/?go-get=1)


example.com$ curl -sI https://v2ray.com/core?go-get=1 | egrep '301|location'
HTTP/2 301
location: http://www.v2ray.com/core/?go-get=1

@bcmills bcmills changed the title cmd/go: go get fails in GOPATH mode on custom import path with a redirect cmd/go: go get fails in GOPATH mode on custom import path with an insecure redirect Sep 4, 2019

@bcmills bcmills changed the title cmd/go: go get fails in GOPATH mode on custom import path with an insecure redirect cmd/go: go get panics in GOPATH mode on custom import path with an insecure redirect Sep 4, 2019

@gopherbot

This comment has been minimized.

Copy link

commented Sep 4, 2019

Change https://golang.org/cl/193264 mentions this issue: [release-branch.go1.13] cmd/go/internal/get: avoid panic in metaImportsForPrefix if web.Get fails

@gopherbot gopherbot closed this in 6719d88 Sep 4, 2019

t4n6a1ka added a commit to t4n6a1ka/go that referenced this issue Sep 5, 2019
cmd/go/internal/get: avoid panic in metaImportsForPrefix if web.Get f…
…ails

Fixes golang#34049

Change-Id: I817b83ee2d0ca6d01ec64998f14bc4f32e365d66
Reviewed-on: https://go-review.googlesource.com/c/go/+/193259
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
gopherbot pushed a commit that referenced this issue Sep 11, 2019
[release-branch.go1.13] cmd/go/internal/get: avoid panic in metaImpor…
…tsForPrefix if web.Get fails

Updates #29591
Updates #34049
Fixes #34081

Change-Id: I817b83ee2d0ca6d01ec64998f14bc4f32e365d66
Reviewed-on: https://go-review.googlesource.com/c/go/+/193259
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
(cherry picked from commit 1eab1aa6ba6c3f4d6f084751bca9a065707c3085)
Reviewed-on: https://go-review.googlesource.com/c/go/+/193264
Reviewed-by: Jay Conrod <jayconrod@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.