From ee7dd810f9ca4e63ecfc1d3044869591783b8b74 Mon Sep 17 00:00:00 2001 From: Filippo Valsorda Date: Fri, 2 Dec 2016 19:43:45 +0000 Subject: [PATCH] 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 --- src/crypto/tls/conn.go | 83 ++++-------------------------- src/crypto/tls/handshake_client.go | 1 - src/crypto/tls/handshake_server.go | 1 - 3 files changed, 11 insertions(+), 74 deletions(-) diff --git a/src/crypto/tls/conn.go b/src/crypto/tls/conn.go index ec5c903c03..dc3b8911c7 100644 --- a/src/crypto/tls/conn.go +++ b/src/crypto/tls/conn.go @@ -28,15 +28,11 @@ type Conn struct { 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 @@ 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() + if err := c.handshakeErr; err != nil { + return err + } + if c.handshakeComplete { + return nil } - - // 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 } diff --git a/src/crypto/tls/handshake_client.go b/src/crypto/tls/handshake_client.go index 62bb1d06c6..fd6df4251c 100644 --- a/src/crypto/tls/handshake_client.go +++ b/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() diff --git a/src/crypto/tls/handshake_server.go b/src/crypto/tls/handshake_server.go index dfb9a59a16..f8dd630a04 100644 --- a/src/crypto/tls/handshake_server.go +++ b/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. -- 2.50.0