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: regression in handling bad Content-Length set by client request #8003
Labels
Milestone
Comments
Comment 1 by kushal.pisavadia@digital.cabinet-office.gov.uk: This patch set could be related: https://golang.org/cl/10237050/ |
Can you describe in words the problem, rather than making everybody read a mix of code, including a reverse proxy which seems unrelated? If I understand correctly, your Ruby HTTP client sends a Content-Length: 2000 header, but then doesn't send that many bytes of data? But your server replies with a 200 OK before it even reads the body. I think you expect the server to buffer up the whole body before your Handler is run? The server won't re-use that underlying TCP connection if you didn't consume the whole request body or it didn't match its declared length, so it's still protecting itself, and you would've gotten an error if you tried to read the short body from your handler, but you didn't. So I think this is all working as intended. Please comment if otherwise. Status changed to WaitingForReply. |
Comment 3 by alex.tomlins@digital.cabinet-office.gov.uk: We're building a frontline router[1] that reverse-proxies requests to various backend servers based on the path. We want to be able to handle the case where the client sends an invalid Content-Length header and return a 400 status. We were intercepting the error raised here: https://code.google.com/p/go/source/browse/src/pkg/net/http/transfer.go?name=release-branch.go1.2#212, and using that to do this. Since https://golang.org/cl/10237050/ it seems that this error is no longer being raised. The sample code we've included in this bugreport is a reduction of our reverse proxy to the minimal code that exposes the problem. The thing we need to be able to do is to detect an error caused by an invalid content-length header in the request, and respond accordingly. If there's a better way of achieving this that would be great. [1] https://github.com/alphagov/router/ |
The Transport (the base of the HTTP Client) doesn't know the Body was short until it's done writing it, but in the meantime the headers & start of the short body are being sent off. And the server is replying immediately. The Transport, if it sees a response before it's done writing, will return the response it got. This typically occurs when a client is sending a large file (without Expect 100-continue) and the server is replying Unauthorized and hanging up, rather than reading 1GB from a client just to discard it. In this case we want the client to see the Unauthorized HTTP error, and not a generic transport failure. Proof that the Transport is still checking it: http://play.golang.org/p/YdbTG7kWJF If you really want to check this, you'll need to count & buffer in your reverse proxy, but be careful that you have limits, so people can't swamp your memory. Alternatively, actually read the whole body in your server handlers and return a 400 there if the client was wrong. |
Comment 6 by alex.tomlins@digital.cabinet-office.gov.uk: Hi, thanks for getting back to me. I've done some more digging, and the problem only seems to surface with a request read from the network. More specifically, only when req.Body is an instance of http.body. I've created an example here: http://play.golang.org/p/2BkktJSnjr. On go 1.1.2 it returns: $ go run test.go go1.1.2 http: Request.ContentLength=10 with Body length 3 Whereas on go 1.2.1 it returns: $ go run test.go go1.2.1 unexpected EOF I think this is because the Read method in http.body now returns an io.ErrUnexpectedEOF[1], which is therefore not swallowed by io.Copy called in transferWriter.WriteBody[2]. I'm aware that the way we're currently intercepting these errors is somewhat fragile. Is there a better way of identifying an error returned by http.Transport.RoundTrip that's caused by a mismatched Content-Length header? thanks, Alex [1] https://code.google.com/p/go/source/browse/src/pkg/net/http/transfer.go?name=release-branch.go1.2#547 [2] https://code.google.com/p/go/source/browse/src/pkg/net/http/transfer.go?name=release-branch.go1.2#196 |
Russ, this is a bug report about a difference between 1.1 and 1.2, not 1.2 to 1.3. In Go 1.2, we properly now return an ErrUnexpectedEOF on the read of an HTTP body shorter than its declared Content-Length. That was a good bug fix. So naturally when you io.Copy that, the resulting error (from the Read side) will also be ErrUnexpectedEOF. If your application cares about whether the read side failed or the write side failed, do them yourself and don't use io.Copy or similar helpers. If you can't (somebody else is calling io.Copy), you can wrap the Reader and/or Writer with versions that record their errors. Or even slurp the body and replace the http.Request.Body with a validated one. (that's reasonable only if there's a sane upper bound on request body size). But for this load balancer, I'd just get the incoming HTTP request and replace the Request.Body with your own io.ReadCloser that wraps the underlying http.body ReadCloser and tracks whether it ever got a Read error. If it did, then don't echo the HTTP response from the backend and instead send your 400. But note that the request has already been written to the backend and therefore may have side effects if your backend has side effects without reading its Request.Body to the end. Status changed to WorkingAsIntended. |
Comment 9 by alex.tomlins@digital.cabinet-office.gov.uk: Thanks for taking the time to explain this. This all makes sense now. |
This issue was closed.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
by kushal.pisavadia@digital.cabinet-office.gov.uk:
Attachments:
The text was updated successfully, but these errors were encountered: