net/http: regression in handling bad Content-Length set by client request

[gopherbot]

by kushal.pisavadia@digital.cabinet-office.gov.uk:

Tested using `go version go1.2.1 linux/amd64` and `go version go1.1.2 linux/amd64`. Also
tested against `go version devel +f8b50ad4cac4 Mon Apr 21 17:00:27 2014 -0700 +
darwin/amd64` and the issue still exists.

What steps reproduce the problem?

1. Run `go run backend.go` in a shell
2. Run `go run server.go` in a separate shell
3. Run `ruby test_client.rb`

What happened?

In Go 1.2 the HTTP server responds with a 500 Internal Server Error

What should have happened instead?

In both cases it should respond with HTTP/1.1 400 Bad Request because the error string
will match `http: Request.ContentLength=\d+ with Body length \d+`.

Please provide any additional information below.

Full output link and code: https://gist.github.com/KushalP/46d2651acae353ee8ee6

Attachments:

1. backend.go (225 bytes)
2. server.go (1268 bytes)
3. test_client.rb (757 bytes)

[gopherbot]

Comment 1 by kushal.pisavadia@digital.cabinet-office.gov.uk:

This patch set could be related: https://golang.org/cl/10237050/

[bradfitz]

Comment 2:

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.

[gopherbot]

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/

[bradfitz]

Comment 4:

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.

[bradfitz]

Comment 5:

> And the server is replying immediately.
By server above I meant what you're calling "backend"... I meant the net/http.Server.

[gopherbot]

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

[rsc]

Comment 7:

Brad can you take a look? I saw some failures that had to do with EOF vs
ErrUnexpectedEOF in some non-open code too so this sounds like something worth at least
understanding.

Labels changed: added release-go1.3maybe.

Status changed to Accepted.

[bradfitz]

Comment 8:

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.

[gopherbot]

Comment 9 by alex.tomlins@digital.cabinet-office.gov.uk:

Thanks for taking the time to explain this.  This all makes sense now.