caddyhttp/proxy: invalid use of Hijack in reverseproxy.go

[dsnet]

Hello, in preparation for Go1.8, I detected this misuse of the http.Hijacker API.

reverseproxy.go makes a call to Hijack, but ignores the returned bufio.ReadWriter and proceeds to directly use the connection. In Go1.8, the probability that data is buffered in the bufio.Reader is increased, such that there is a higher change that this logic fails. The proper fix is to handle the data in the read buffer (accessed via brw.Reader.Peek(brw.Reader.Buffered())) and forward that to the backendConn before performing the pair of io.Copy calls.

See https://golang.org/cl/35232 for more information.

cc @mholt

[mholt]

Dang, I'm swamped for a bit with other things. Can @lhecker or anyone else look into this? (Thanks for finding and reporting this @dsnet!)

[lhecker]

I'd be fine with fixing this, but unfortunately I don't quite get why it doesn't work already. I was under the impression that the returned bufio.ReadWriter is basically just an aliased interface to the returned net.Conn. I thought if you read from the net.Conn it will also read all buffered bytes.

Can you explain (if you don't mind) why that's wrong? Or maybe you know where to find some sample code which makes use of the bufio.ReadWriter _as well as_ the net.Conn?

[dsnet]

You are correct that the returned bufio.ReadWriter wraps the net.Conn, which implies that the net.Conn does not wrap the bufio.ReadWriter. Thus, if there are bytes buffered in the bufio.Reader's read buffer, directly reading from the net.Conn will loose knowledge about those bytes.

One way to solve this is to use a wrapped net.Conn that returns the buffered bytes before reading from the connection.

type rbufConn struct {
    net.Conn
    rbuf []byte
}

func (c *rbufConn) Read(p []byte) (int, error) {
    if len(c.rbuf) > 0 {
        n := copy(p, c.rbuf)
        c.rbuf = c.rbuf[n:]
        return n, nil
    }
    return c.Conn.Read(p)
}

func (c *rbufConn) Close() error {
    c.rbuf = nil
    return c.Conn.Close()
}

// Elsewhere in the code (ignoring error checking):
c, brw, _ := resp.(http.Hijacker).Hijack()
rbuf, _ := brw.Reader.Peek(brw.Reader.Buffered())
c = &rbufConn{conn, rbuf}

[lhecker]

@dsnet @mholt How's that?

[dsnet]

The change LGTM.