FAQ
Reviewers: dvyukov, agl1,

Message:
Hello dvyukov@google.com, agl@golang.org (cc:
golang-dev@googlegroups.com),

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


Description:
crypto/tls: fix data race on conn.err

Fixes issue 3862.

There were many areas where conn.err was being accessed
outside the mutex. This proposal moves the err value to
an embedded struct to make it more obvious when err.value
is being accessed.

As there are not Benchmark tests in this package I cannot
feel confident on the impact of this additional locking,
although most will be uncontended.

Please review this at http://codereview.appspot.com/6497070/

Affected files:
M src/pkg/crypto/tls/conn.go
M src/pkg/crypto/tls/handshake_client.go


Index: src/pkg/crypto/tls/conn.go
===================================================================
--- a/src/pkg/crypto/tls/conn.go
+++ b/src/pkg/crypto/tls/conn.go
@@ -44,8 +44,7 @@
clientProtocolFallback bool

// first permanent error
- errMutex sync.Mutex
- err error
+ err

// input/output
in, out halfConn // in.Mutex < out.Mutex
@@ -56,21 +55,25 @@
tmp [16]byte
}

-func (c *Conn) setError(err error) error {
- c.errMutex.Lock()
- defer c.errMutex.Unlock()
+type err struct {
+ mu sync.Mutex
+ value error
+}

- if c.err == nil {
- c.err = err
+func (e *err) setError(err error) error {
+ e.mu.Lock()
+ defer e.mu.Unlock()
+
+ if e.value == nil {
+ e.value = err
}
return err
}

-func (c *Conn) error() error {
- c.errMutex.Lock()
- defer c.errMutex.Unlock()
-
- return c.err
+func (e *err) error() error {
+ e.mu.Lock()
+ defer e.mu.Unlock()
+ return e.value
}

// Access to net.Conn methods.
@@ -660,8 +663,7 @@
c.tmp[0] = alertLevelError
c.tmp[1] = byte(err.(alert))
c.writeRecord(recordTypeAlert, c.tmp[0:2])
- c.err = &net.OpError{Op: "local error", Err: err}
- return n, c.err
+ return n, c.setError(&net.OpError{Op: "local error", Err: err})
}
}
return
@@ -672,8 +674,8 @@
// c.in.Mutex < L; c.out.Mutex < L.
func (c *Conn) readHandshake() (interface{}, error) {
for c.hand.Len() < 4 {
- if c.err != nil {
- return nil, c.err
+ if err := c.error(); err != nil {
+ return nil, err
}
if err := c.readRecord(recordTypeHandshake); err != nil {
return nil, err
@@ -684,11 +686,11 @@
n := int(data[1])<<16 | int(data[2])<<8 | int(data[3])
if n > maxHandshake {
c.sendAlert(alertInternalError)
- return nil, c.err
+ return nil, c.error()
}
for c.hand.Len() < 4+n {
- if c.err != nil {
- return nil, c.err
+ if err := c.error(); err != nil {
+ return nil, err
}
if err := c.readRecord(recordTypeHandshake); err != nil {
return nil, err
@@ -738,12 +740,12 @@

// Write writes data to the connection.
func (c *Conn) Write(b []byte) (int, error) {
- if c.err != nil {
- return 0, c.err
+ if err := c.error(); err != nil {
+ return 0, err
}

- if c.err = c.Handshake(); c.err != nil {
- return 0, c.err
+ if err := c.setError(c.Handshake()); err != nil {
+ return 0, err
}

c.out.Lock()
@@ -753,9 +755,8 @@
return 0, alertInternalError
}

- var n int
- n, c.err = c.writeRecord(recordTypeApplicationData, b)
- return n, c.err
+ n, err := c.writeRecord(recordTypeApplicationData, b)
+ return n, c.setError(err)
}

// Read can be made to time out and return a net.Error with Timeout() ==
true
@@ -768,14 +769,14 @@
c.in.Lock()
defer c.in.Unlock()

- for c.input == nil && c.err == nil {
+ for c.input == nil && c.error() == nil {
if err := c.readRecord(recordTypeApplicationData); err != nil {
// Soft error, like EAGAIN
return 0, err
}
}
- if c.err != nil {
- return 0, c.err
+ if err := c.error(); err != nil {
+ return 0, err
}
n, err = c.input.Read(b)
if c.input.off >= len(c.input.data) {
Index: src/pkg/crypto/tls/handshake_client.go
===================================================================
--- a/src/pkg/crypto/tls/handshake_client.go
+++ b/src/pkg/crypto/tls/handshake_client.go
@@ -306,8 +306,8 @@
serverHash := suite.mac(c.vers, serverMAC)
c.in.prepareCipherSpec(c.vers, serverCipher, serverHash)
c.readRecord(recordTypeChangeCipherSpec)
- if c.err != nil {
- return c.err
+ if err := c.error(); err != nil {
+ return err
}

msg, err = c.readHandshake()

Search Discussions

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedSep 5, '12 at 11:15a
activeSep 6, '12 at 7:50a
posts7
users3
websitegolang.org

3 users in discussion

Dave: 4 posts Dvyukov: 2 posts Agl: 1 post

People

Translate

site design / logo © 2022 Grokbase