Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.Sign up
net/http: allows read from Response.Body after Close #13648
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
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 <firstname.lastname@example.org>
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 <email@example.com>