]> Cypherpunks repositories - gostls13.git/commitdiff
crypto/tls: simplify the Handshake locking strategy
authorFilippo Valsorda <hi@filippo.io>
Fri, 2 Dec 2016 19:43:45 +0000 (19:43 +0000)
committerFilippo Valsorda <filippo@golang.org>
Tue, 3 Apr 2018 16:44:55 +0000 (16:44 +0000)
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
src/crypto/tls/handshake_client.go
src/crypto/tls/handshake_server.go

index ec5c903c03f68f967595b4b74d91502af9f21b8a..dc3b8911c771d9e72bb7936b179bcae2760f2aa0 100644 (file)
@@ -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
 }
 
index 62bb1d06c649b3c36a1597bc574d6a5040a3bdb5..fd6df4251cf31da34c5be6609c197ce6ea21a37b 100644 (file)
@@ -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()
index dfb9a59a1602e8e0c180ebe7167205276ab3339e..f8dd630a04b796feb74e07a6e1b3748101126d66 100644 (file)
@@ -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.