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

net/http: allows read from Response.Body after Close #13648

Closed
rsc opened this issue Dec 17, 2015 · 5 comments
Closed

net/http: allows read from Response.Body after Close #13648

rsc opened this issue Dec 17, 2015 · 5 comments
Milestone

Comments

@rsc
Copy link
Contributor

rsc commented Dec 17, 2015

This program runs correctly in Go 1.5: it exits 0. In Go 1.6 it dies because it detects the ReadAll succeeding despite having called Body.Close.

This was masking some bad code in go get but it's a real problem in its own right: Read after Close should fail.

package main

import (
    "io/ioutil"
    "log"
    "net/http"
)

func main() {
    resp, err := http.DefaultClient.Get("https://bazil.org/fuse/fs/fstestutil?go-get=1")
    if err != nil {
        log.Fatal(err)
    }
    resp.Body.Close()
    data, err := ioutil.ReadAll(resp.Body)
    if len(data) != 0 || err == nil {
        log.Fatalf("ReadAll returned %q, %v", data, err)
    }
}
@rsc rsc added this to the Go1.6 milestone Dec 17, 2015
@gopherbot
Copy link

CL https://golang.org/cl/17944 mentions this issue.

@rsc
Copy link
Contributor Author

rsc commented Dec 17, 2015

For the Github record, that CL mentions the issue but is not a fix for it.

@bradfitz
Copy link
Contributor

Ah! This is only in http2 mode.

@gopherbot
Copy link

CL https://golang.org/cl/17937 mentions this issue.

bradfitz added a commit to golang/net that referenced this issue Dec 17, 2015
The Transport's Response.Body.Close call was closing the
Response.Body, but the Reader implementation was yielding its buffered
data before returning the error. Add a new method to force an
immediate close.  An audit of the other CloseWithError callers found
that the after-buffered-data behavior was correct for them.

New tests in the main go repo in net/http/clientserver_test.go:
TestResponseBodyReadAfterClose_h1 and TestResponseBodyReadAfterClose_h2

Updates golang/go#13648

Change-Id: If3a13a20c106b5a7bbe668ccb4e3c704a0e0682b
Reviewed-on: https://go-review.googlesource.com/17937
Reviewed-by: Russ Cox <rsc@golang.org>
@gopherbot
Copy link

CL https://golang.org/cl/17938 mentions this issue.

rsc added a commit that referenced this issue Dec 17, 2015
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>
@golang golang locked and limited conversation to collaborators Dec 29, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants