code review 69090044: crypto/tls: split connErr to avoid read/write races.

crypto/tls: split connErr to avoid read/write races.

Currently a write error will cause future reads to return that same error.
However, there may have been extra information from a peer pending on
the read direction that is now unavailable.

This change splits the single connErr into errors for the read, write and
handshake. (Splitting off the handshake error is needed because both read
and write paths check the handshake error.)

Fixes issue 7414.

[agl1, 2014-02-26 20:28:41]

Hello golang-codereviews@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go/

[r, 2014-02-27 23:12:40]

https://codereview.appspot.com/69090044/diff/90001/src/pkg/crypto/tls/conn.go
File src/pkg/crypto/tls/conn.go (right):

https://codereview.appspot.com/69090044/diff/90001/src/pkg/crypto/tls/conn.go...
src/pkg/crypto/tls/conn.go:542: c.in.err = err
is there a good reason not to use setErrorLocked in all these assignments? or
else never use it, and do the set and an explicit return c.in.err? the
inconsistency bothers me.

[agl1, 2014-02-28 14:45:22]

https://codereview.appspot.com/69090044/diff/90001/src/pkg/crypto/tls/conn.go
File src/pkg/crypto/tls/conn.go (right):

https://codereview.appspot.com/69090044/diff/90001/src/pkg/crypto/tls/conn.go...
src/pkg/crypto/tls/conn.go:542: c.in.err = err
On 2014/02/27 23:12:40, r wrote:
> is there a good reason not to use setErrorLocked in all these assignments? or
> else never use it, and do the set and an explicit return c.in.err? the
> inconsistency bothers me.

There's no problem with using setErrorLocked everywhere. Done.

[bradfitz, 2014-02-28 15:54:49]

LGTM

https://codereview.appspot.com/69090044/diff/110001/src/pkg/crypto/tls/conn.go
File src/pkg/crypto/tls/conn.go (right):

https://codereview.appspot.com/69090044/diff/110001/src/pkg/crypto/tls/conn.g...
src/pkg/crypto/tls/conn.go:840: if err := c.Handshake(); err != nil {
Not for this CL, but I wonder if it would be better to put the handshake behind
a Once:

  func (c *Conn) Handshake() error {
     c.handshakeOnce.Do(c.handshake)
     return c.handshakeErr
  }

  func (c *Conn) handshake() {
       .....
  }

[r, 2014-02-28 21:23:43]

LGTM

[agl1, 2014-03-03 14:01:58]

*** Submitted as https://code.google.com/p/go/source/detail?r=bc595acf7db4 ***

crypto/tls: split connErr to avoid read/write races.

Currently a write error will cause future reads to return that same error.
However, there may have been extra information from a peer pending on
the read direction that is now unavailable.

This change splits the single connErr into errors for the read, write and
handshake. (Splitting off the handshake error is needed because both read
and write paths check the handshake error.)

Fixes issue 7414.

LGTM=bradfitz, r
R=golang-codereviews, r, bradfitz
CC=golang-codereviews
https://codereview.appspot.com/69090044

[gobot, 2014-03-03 14:07:10]

This CL appears to have broken the darwin-386-cheney builder.

[agl1, 2014-03-03 14:14:07]

On Mon, Mar 3, 2014 at 9:07 AM,  <gobot@golang.org> wrote:
> This CL appears to have broken the darwin-386-cheney builder.

Seems unlikely:

# go run run.go -- index2.go

tmp__.go:3062: missing error "index|overflow|truncated|must be integer"
Unmatched Errors:
tmp__.go:3062: invalid slice index cni64big (index must be non-negative)

FAIL index2.go 30.544s

[josharian, 2014-03-03 14:26:29]

Issue 7370


On Monday, March 3, 2014, Adam Langley <agl@golang.org> wrote:

> On Mon, Mar 3, 2014 at 9:07 AM,  <gobot@golang.org <javascript:;>> wrote:
> > This CL appears to have broken the darwin-386-cheney builder.
>
> Seems unlikely:
>
> # go run run.go -- index2.go
>
> tmp__.go:3062: missing error "index|overflow|truncated|must be integer"
> Unmatched Errors:
> tmp__.go:3062: invalid slice index cni64big (index must be non-negative)
>
> FAIL index2.go 30.544s
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com <javascript:;>.
> For more options, visit https://groups.google.com/groups/opt_out.
>

[agl1, 2014-09-09 20:59:03]

*** Abandoned ***