http: Server sends no error for malformed requests.

[gopherbot]

by consalus:

What steps will reproduce the problem?
1. Start a minimal webserver, such as this:

package main

import "http"

func main() {
    http.HandleFunc("/", func(rw http.ResponseWriter, req *http.Request) {
        rw.Write([]byte("RawURL: " + req.RawURL))
    })
    http.ListenAndServe(":8080", nil)
}

2. Visit it with a bad URL, for example "http://localhost:8080/%q";.


What is the expected output?

It should serve a 400, or maybe log an error.


What do you see instead?

The browser gets a zero-byte response.
Server gives no indication of what happened.


Which compiler are you using (5g, 6g, 8g, gccgo)?

6g


Which operating system are you using?

OS X 10.5.8


Which revision are you using?  (hg identify)

d5785050f61d (release-branch.r59) release/release.r59


Please provide any additional information below.

conn.serve checks for an error from conn.readRequest, but discards it after checking for
nil. 
We don't have a Request (or a response) if URL parsing fails, so writing back an error
isn't entirely trivial.

I hacked together a local fix that works in all cases I've tested, but it is a bit
inelegant and likely incomplete. Also, I'm not an expert on the http package (or http,
for that matter). Still, I've attached the diff, in the off chance that it might be a
useful starting point.

I think the real fix will probably include a modification to the public interface,
either a new error type or an additional output parameter to ReadRequest.

Attachments:

1. naivefix.diff (2739 bytes)

[robpike]

Comment 1:

Owner changed to @bradfitz.

Status changed to Accepted.

[bradfitz]

Comment 2:

Your patch is much longer than I'd expect, and makes me think I don't understand the
issue.
Why not just a one-liner in,
func (c *conn) serve() {
...
  w, err := c.readRequest()
  if err != nil {
    if err == errTooLarge {
      fmt.Fprintf(c.rwc, "HTTP/1.1 400 Request Too Large\r\n\r\n")
    } else {
      fmt.Fprintf(c.rwc, "HTTP/1.1 400 Bad Request\r\n\r\n")   // <<<<<<<<<<<<
    }
...
Would that work?

[gopherbot]

Comment 3 by consalus:

That looks ok to me as fix. 
However, that still results in a no-content page for bad requests. One of the reasons my
patch was ugly and long was that I assumed we'd want to write the 400 with a Body
indicating what went wrong, along with maybe Date, Content-Type, Content-Length, and
probably "Connection: close" headers. We have code for that, but it requires a *response
and a Request, and trying to fulfill that needs even when ReadRequest failed lead me
down a bumpy, awkward road.
Granted, you could just add a "Bad Request" to the end of the Fprintf string, but I have
no idea what the spec or accepted browser/cache behavior says about that. 
Also, the one-liner you propose sends a 400 for ErrHijacked, ErrNotSupported,
ErrUnexpectedEOF, and others. 
In my change I tried to avoid sending for any other than the few cases in ReadRequest I
was sure about, and that added some complexity. However, sending a 400 is likely better
than dropping the request silently for most errors, and is certainly entirely correct
for at least some of them. 
Anyway, my fix was thrown together relatively quickly, and was certainly more
complicated than necessary as a result of my assumptions and assumed requirements. You
know better than I what assumptions are safe and what requirements are reasonable, so if
you think it can be done simpler, it almost certainly can be.

[rsc]

Comment 4:

There's no need for hand-holding.
Brad's fix is good.

[bradfitz]

Comment 5:

This issue was closed by revision bb4cf3f.

Status changed to Fixed.