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>
diff --git a/src/crypto/tls/conn.go b/src/crypto/tls/conn.go
index ec5c903..dc3b891 100644
--- a/src/crypto/tls/conn.go
+++ b/src/crypto/tls/conn.go
@@ -28,15 +28,11 @@
isClient bool
// constant after handshake; protected by handshakeMutex
- handshakeMutex sync.Mutex // handshakeMutex < in.Mutex, out.Mutex, errMutex
- // 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
- config *Config // configuration passed to constructor
+ handshakeMutex sync.Mutex
+ handshakeErr error // error resulting from handshake
+ vers uint16 // TLS version
+ haveVers bool // version has been negotiated
+ config *Config // configuration passed to constructor
// handshakeComplete is true if the connection is currently transferring
// application data (i.e. is not currently processing a handshake).
handshakeComplete bool
@@ -84,7 +80,7 @@
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 @@
// 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 @@
}
// 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 @@
}
// 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 @@
//
// 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 @@
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 @@
// 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 @@
// 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 @@
// 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 @@
}
// 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 @@
// 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()
+ if err := c.handshakeErr; err != nil {
+ return err
}
-
- // Set handshakeCond to indicate that this goroutine is committing to
- // running the handshake.
- c.handshakeCond = sync.NewCond(&c.handshakeMutex)
- c.handshakeMutex.Unlock()
+ if c.handshakeComplete {
+ return nil
+ }
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 @@
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
}
diff --git a/src/crypto/tls/handshake_client.go b/src/crypto/tls/handshake_client.go
index 62bb1d0..fd6df42 100644
--- a/src/crypto/tls/handshake_client.go
+++ b/src/crypto/tls/handshake_client.go
@@ -91,7 +91,6 @@
return hello, nil
}
-// c.out.Mutex <= L; c.handshakeMutex <= L.
func (c *Conn) clientHandshake() error {
if c.config == nil {
c.config = defaultConfig()
diff --git a/src/crypto/tls/handshake_server.go b/src/crypto/tls/handshake_server.go
index dfb9a59..f8dd630 100644
--- a/src/crypto/tls/handshake_server.go
+++ b/src/crypto/tls/handshake_server.go
@@ -36,7 +36,6 @@
}
// 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.
