mirrors/utls
1
0
Fork 0
mirror of https://github.com/refraction-networking/utls.git synced 2025-04-04 12:37:35 +03:00
Code Issues Projects Releases Packages Wiki Activity

crypto/tls: simplify the Handshake locking strategy

Browse source 
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>
This commit is contained in:
Filippo Valsorda 2016-12-02 19:43:45 +00:00 • committed by Filippo Valsorda
parent 3ec0b31ffa
commit d8f27b6eac
3 changed files with 11 additions and 74 deletions
Download patch file Download diff file Expand all files Collapse all files

83
conn.go
View 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
}

1
handshake_client.go
View 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()

1
handshake_server.go
View 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.