code review 5283045: http, crypto/tls: fix read timeouts and closing.

http, crypto/tls: fix read timeouts and closing.

tls.Conn.Close() didn't close the underlying connection and tried to
do a handshake in order to send the close notify alert.

http didn't look for errors from the TLS handshake.

Fixes issue 2281.

[agl1, 2011-10-14 19:00:18]

Hello bradfitz@golang.org (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[bradfitz, 2011-10-14 19:11:54]

LGTM

On Fri, Oct 14, 2011 at 12:00 PM, <agl@golang.org> wrote:

> Reviewers: bradfitz,
>
> Message:
> Hello bradfitz@golang.org (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> http, crypto/tls: fix read timeouts and closing.
>
> tls.Conn.Close() didn't close the underlying connection and tried to
> do a handshake in order to send the close notify alert.
>
> http didn't look for errors from the TLS handshake.
>
> Fixes issue 2281.
>
> Please review this at
http://codereview.appspot.com/**5283045/<http://codereview.appspot.com/5283045/>
>
> Affected files:
>  M src/pkg/crypto/tls/conn.go
>  M src/pkg/http/serve_test.go
>  M src/pkg/http/server.go
>
>
> Index: src/pkg/crypto/tls/conn.go
> ==============================**==============================**=======
> --- a/src/pkg/crypto/tls/conn.go
> +++ b/src/pkg/crypto/tls/conn.go
> @@ -658,7 +658,9 @@
>                if c.err != nil {
>                        return nil, c.err
>                }
> -               c.readRecord(**recordTypeHandshake)
> +               if err := c.readRecord(**recordTypeHandshake); err != nil
> {
> +                       return nil, err
> +               }
>        }
>
>        data := c.hand.Bytes()
> @@ -671,7 +673,9 @@
>                if c.err != nil {
>                        return nil, c.err
>                }
> -               c.readRecord(**recordTypeHandshake)
> +               if err := c.readRecord(**recordTypeHandshake); err != nil
> {
> +                       return nil, err
> +               }
>        }
>        data = c.hand.Next(4 + n)
>        var m handshakeMessage
> @@ -762,10 +766,18 @@
>
>  // Close closes the connection.
>  func (c *Conn) Close() os.Error {
> -       if err := c.Handshake(); err != nil {
> +       var alertErr os.Error
> +
> +       c.handshakeMutex.Lock()
> +       defer c.handshakeMutex.Unlock()
> +       if c.handshakeComplete {
> +               alertErr = c.sendAlert(alertCloseNotify)
> +       }
> +
> +       if err := c.conn.Close(); err != nil {
>                return err
>        }
> -       return c.sendAlert(alertCloseNotify)
> +       return alertErr
>  }
>
>  // Handshake runs the client or server handshake
> Index: src/pkg/http/serve_test.go
> ==============================**==============================**=======
> --- a/src/pkg/http/serve_test.go
> +++ b/src/pkg/http/serve_test.go
> @@ -536,10 +536,6 @@
>  }
>
>  func TestTLSHandshakeTimeout(t *testing.T) {
> -       if true {
> -               t.Logf("Skipping broken test; issue 2281")
> -               return
> -       }
>        ts := httptest.NewUnstartedServer(**HandlerFunc(func(w
> ResponseWriter, r *Request) {}))
>        ts.Config.ReadTimeout = 250e6
>        ts.StartTLS()
> Index: src/pkg/http/server.go
> ==============================**==============================**=======
> --- a/src/pkg/http/server.go
> +++ b/src/pkg/http/server.go
> @@ -556,7 +556,10 @@
>        }()
>
>        if tlsConn, ok := c.rwc.(*tls.Conn); ok {
> -               tlsConn.Handshake()
> +               if err := tlsConn.Handshake(); err != nil {
> +                       c.close()
> +                       return
> +               }
>                c.tlsState = new(tls.ConnectionState)
>                *c.tlsState = tlsConn.ConnectionState()
>        }
>
>
>

[agl1, 2011-10-18 16:59:38]

*** Submitted as http://code.google.com/p/go/source/detail?r=330818296ada ***

http, crypto/tls: fix read timeouts and closing.

tls.Conn.Close() didn't close the underlying connection and tried to
do a handshake in order to send the close notify alert.

http didn't look for errors from the TLS handshake.

Fixes issue 2281.

R=bradfitz
CC=golang-dev
http://codereview.appspot.com/5283045