crypto/tls: simplify the Handshake locking strategy

If in.Mutex is never locked by Handshake when c.handshakeComplete is
true, and since c.handshakeComplete is unset and then set back by
handleRenegotiation all under both in.Mutex and handshakeMutex, we can
significantly simplify the locking strategy by removing the sync.Cond.

See also https://groups.google.com/forum/#!topic/golang-dev/Xxiai-R_jH0
and a more complete analysis at https://go-review.googlesource.com/c/go/+/33776#message-223a3ccc819f7015cc773d214c65bad70de5dfd7

Change-Id: I6052695ece9aff9e3112c2fb176596fde8aa9cb2
Reviewed-on: https://go-review.googlesource.com/33776
Reviewed-by: Adam Langley <agl@golang.org>

src/crypto/tls/conn.go

@@ -28,11 +28,7 @@ type Conn struct {
 	isClient bool
 
 	// constant after handshake; protected by handshakeMutex
-	handshakeMutex sync.Mutex // handshakeMutex < in.Mutex, out.Mutex, errMutex
+	handshakeMutex sync.Mutex
-	// handshakeCond, if not nil, indicates that a goroutine is committed
-	// to running the handshake for this Conn. Other goroutines that need
-	// to wait for the handshake can wait on this, under handshakeMutex.
-	handshakeCond *sync.Cond
 	handshakeErr   error   // error resulting from handshake
 	vers           uint16  // TLS version
 	haveVers       bool    // version has been negotiated
@@ -84,7 +80,7 @@ type Conn struct {
 	clientProtocolFallback bool
 
 	// input/output
-	in, out   halfConn     // in.Mutex < out.Mutex
+	in, out   halfConn
 	rawInput  *block       // raw input, right off the wire
 	input     *block       // application data waiting to be read
 	hand      bytes.Buffer // handshake data waiting to be read
@@ -566,7 +562,6 @@ func (c *Conn) newRecordHeaderError(msg string) (err RecordHeaderError) {
 
 // readRecord reads the next TLS record from the connection
 // and updates the record layer state.
-// c.in.Mutex <= L; c.input == nil.
 func (c *Conn) readRecord(want recordType) error {
 	// Caller must be in sync with connection:
 	// handshake data if handshake not yet completed,
@@ -738,7 +733,6 @@ Again:
 }
 
 // sendAlert sends a TLS alert message.
-// c.out.Mutex <= L.
 func (c *Conn) sendAlertLocked(err alert) error {
 	switch err {
 	case alertNoRenegotiation, alertCloseNotify:
@@ -758,7 +752,6 @@ func (c *Conn) sendAlertLocked(err alert) error {
 }
 
 // sendAlert sends a TLS alert message.
-// L < c.out.Mutex.
 func (c *Conn) sendAlert(err alert) error {
 	c.out.Lock()
 	defer c.out.Unlock()
@@ -795,8 +788,6 @@ const (
 //
 // In the interests of simplicity and determinism, this code does not attempt
 // to reset the record size once the connection is idle, however.
-//
-// c.out.Mutex <= L.
 func (c *Conn) maxPayloadSizeForWrite(typ recordType, explicitIVLen int) int {
 	if c.config.DynamicRecordSizingDisabled || typ != recordTypeApplicationData {
 		return maxPlaintext
@@ -846,7 +837,6 @@ func (c *Conn) maxPayloadSizeForWrite(typ recordType, explicitIVLen int) int {
 	return n
 }
 
-// c.out.Mutex <= L.
 func (c *Conn) write(data []byte) (int, error) {
 	if c.buffering {
 		c.sendBuf = append(c.sendBuf, data...)
@@ -872,7 +862,6 @@ func (c *Conn) flush() (int, error) {
 
 // writeRecordLocked writes a TLS record with the given type and payload to the
 // connection and updates the record layer state.
-// c.out.Mutex <= L.
 func (c *Conn) writeRecordLocked(typ recordType, data []byte) (int, error) {
 	b := c.out.newBlock()
 	defer c.out.freeBlock(b)
@@ -948,7 +937,6 @@ func (c *Conn) writeRecordLocked(typ recordType, data []byte) (int, error) {
 
 // writeRecord writes a TLS record with the given type and payload to the
 // connection and updates the record layer state.
-// L < c.out.Mutex.
 func (c *Conn) writeRecord(typ recordType, data []byte) (int, error) {
 	c.out.Lock()
 	defer c.out.Unlock()
@@ -958,7 +946,6 @@ func (c *Conn) writeRecord(typ recordType, data []byte) (int, error) {
 
 // readHandshake reads the next handshake message from
 // the record layer.
-// c.in.Mutex < L; c.out.Mutex < L.
 func (c *Conn) readHandshake() (interface{}, error) {
 	for c.hand.Len() < 4 {
 		if err := c.in.err; err != nil {
@@ -1094,7 +1081,6 @@ func (c *Conn) Write(b []byte) (int, error) {
 }
 
 // handleRenegotiation processes a HelloRequest handshake message.
-// c.in.Mutex <= L
 func (c *Conn) handleRenegotiation() error {
 	msg, err := c.readHandshake()
 	if err != nil {
@@ -1272,61 +1258,19 @@ func (c *Conn) closeNotify() error {
 // Most uses of this package need not call Handshake
 // explicitly: the first Read or Write will call it automatically.
 func (c *Conn) Handshake() error {
-	// c.handshakeErr and c.handshakeComplete are protected by
-	// c.handshakeMutex. In order to perform a handshake, we need to lock
-	// c.in also and c.handshakeMutex must be locked after c.in.
-	//
-	// However, if a Read() operation is hanging then it'll be holding the
-	// lock on c.in and so taking it here would cause all operations that
-	// need to check whether a handshake is pending (such as Write) to
-	// block.
-	//
-	// Thus we first take c.handshakeMutex to check whether a handshake is
-	// needed.
-	//
-	// If so then, previously, this code would unlock handshakeMutex and
-	// then lock c.in and handshakeMutex in the correct order to run the
-	// handshake. The problem was that it was possible for a Read to
-	// complete the handshake once handshakeMutex was unlocked and then
-	// keep c.in while waiting for network data. Thus a concurrent
-	// operation could be blocked on c.in.
-	//
-	// Thus handshakeCond is used to signal that a goroutine is committed
-	// to running the handshake and other goroutines can wait on it if they
-	// need. handshakeCond is protected by handshakeMutex.
 	c.handshakeMutex.Lock()
 	defer c.handshakeMutex.Unlock()
 
-	for {
 	if err := c.handshakeErr; err != nil {
 		return err
 	}
 	if c.handshakeComplete {
 		return nil
 	}
-		if c.handshakeCond == nil {
-			break
-		}
-
-		c.handshakeCond.Wait()
-	}
-
-	// Set handshakeCond to indicate that this goroutine is committing to
-	// running the handshake.
-	c.handshakeCond = sync.NewCond(&c.handshakeMutex)
-	c.handshakeMutex.Unlock()
 
 	c.in.Lock()
 	defer c.in.Unlock()
 
-	c.handshakeMutex.Lock()
-
-	// The handshake cannot have completed when handshakeMutex was unlocked
-	// because this goroutine set handshakeCond.
-	if c.handshakeErr != nil || c.handshakeComplete {
-		panic("handshake should not have been able to complete after handshakeCond was set")
-	}
-
 	if c.isClient {
 		c.handshakeErr = c.clientHandshake()
 	} else {
@@ -1344,11 +1288,6 @@ func (c *Conn) Handshake() error {
 		panic("handshake should have had a result.")
 	}
 
-	// Wake any other goroutines that are waiting for this handshake to
-	// complete.
-	c.handshakeCond.Broadcast()
-	c.handshakeCond = nil
-
 	return c.handshakeErr
 }
 

src/crypto/tls/handshake_client.go

@@ -91,7 +91,6 @@ NextCipherSuite:
 	return hello, nil
 }
 
-// c.out.Mutex <= L; c.handshakeMutex <= L.
 func (c *Conn) clientHandshake() error {
 	if c.config == nil {
 		c.config = defaultConfig()

src/crypto/tls/handshake_server.go

@@ -36,7 +36,6 @@ type serverHandshakeState struct {
 }
 
 // serverHandshake performs a TLS handshake as a server.
-// c.out.Mutex <= L; c.handshakeMutex <= L.
 func (c *Conn) serverHandshake() error {
 	// If this is the first server handshake, we generate a random key to
 	// encrypt the tickets with.