cmd/go: fix processing of HTTPS 404 without -insecure

The change here is to move the closeBody call into the if block.
The logging adjustments are just arranging to tell the truth:
in particular if we're not in insecure mode and we get a non-200
error then we do not actually ignore the response
(except as caused by closing the body incorrectly).

As the comment below the change indicates, it is intentional that
we process non-200 pages. The code does process them, because
the if err != nil || status != 200 block does not return.
But that block does close the body, which depending on timing
can apparently poison the later read from the body.

See #13037's initial report:

	$ go get -v bosun.org/cmd/bosun/cache
	Fetching https://bosun.org/cmd/bosun/cache?go-get=1
	ignoring https fetch with status code 404
	Parsing meta tags from https://bosun.org/cmd/bosun/cache?go-get=1 (status code 404)
	import "bosun.org/cmd/bosun/cache": parsing bosun.org/cmd/bosun/cache: http: read on closed response body
	package bosun.org/cmd/bosun/cache: unrecognized import path "bosun.org/cmd/bosun/cache"

The log print about ignoring the https fetch is not strictly true,
since the next thing that happened was parsing the body of that fetch.
But the read on the closed response body failed during parsing.

Moving the closeBody to happen only when we're about to discard the
result and start over (that is, only in -insecure mode) fixes the parse.

At least it should fix the parse. I can't seem to break the parse anymore,
because of #13648 (close not barring future reads anymore),
but this way is clearly better than the old way. If nothing else the old code
closed the body twice when err != nil and -insecure was not given.

Fixes #13037.

Change-Id: Idf57eceb6d5518341a2f7f75eb8f8ab27ed4e0b4
Reviewed-on: https://go-review.googlesource.com/17944
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

2 files changed, 17 insertions, 7 deletions

diff --git a/src/cmd/go/go_test.go b/src/cmd/go/go_test.go
index f3dbe85c01..cb983e97e9 100644
--- a/src/cmd/go/go_test.go
+++ b/src/cmd/go/go_test.go
@@ -2118,6 +2118,17 @@ func TestGoGetRscIoToolstash(t *testing.T) {
 	tg.run("get", "./toolstash")
 }
 
+// Issue 13037: Was not parsing <meta> tags in 404 served over HTTPS
+func TestGoGetHTTPS404(t *testing.T) {
+	testenv.MustHaveExternalNetwork(t)
+
+	tg := testgo(t)
+	defer tg.cleanup()
+	tg.tempDir("src")
+	tg.setenv("GOPATH", tg.path("."))
+	tg.run("get", "bazil.org/fuse/fs/fstestutil")
+}
+
 // Test that you can not import a main package.
 func TestIssue4210(t *testing.T) {
 	tg := testgo(t)
diff --git a/src/cmd/go/http.go b/src/cmd/go/http.go
index 7979c41b11..d558dcd0b9 100644
--- a/src/cmd/go/http.go
+++ b/src/cmd/go/http.go
@@ -84,15 +84,14 @@ func httpsOrHTTP(importPath string, security securityMode) (urlStr string, body
 	}
 	urlStr, res, err := fetch("https")
 	if err != nil || res.StatusCode != 200 {
-		if buildV {
-			if err != nil {
-				log.Printf("https fetch failed.")
-			} else {
-				log.Printf("ignoring https fetch with status code %d", res.StatusCode)
-			}
+		if buildV && err != nil {
+			log.Printf("https fetch failed: %v", err)
 		}
-		closeBody(res)
 		if security == insecure {
+			if buildV && res.StatusCode != 200 {
+				log.Printf("https fetch: status %s", res.Status)
+			}
+			closeBody(res)
 			urlStr, res, err = fetch("http")
 		}
 	}