crypto/tls: conflicting lock order in Handshake() and Read()

[BurtonQin]

What version of Go are you using (go version)?

1.14.2

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/boqin/.cache/go-build"
GOENV="/home/boqin/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/boqin/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/data/suz305/go-workspace/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/data/suz305/go-workspace/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build519488021=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I analyzed the source code crypto/tls/conn.go and found a possible conflicting lock order which may lead to deadlock.
If Conn.Handshake() and Conn.Read() are executed concurrently in two threads, then a deadlock may occur.
In crypto/tls/conn.go

type Conn struct {
	...
	handshakeMutex sync.Mutex
	...

	// input/output
	in, out   halfConn
	
}

type halfConn struct {
	sync.Mutex

	...
}

1. Conn.handshakeMutex.Lock() -> Conn.in.Lock()
   In Handshake(): Conn.handshakeMutex.Lock() is on L1344. Conn.in.Lock() is on L1354
   

   go/src/crypto/tls/conn.go

   Lines 1343 to 1355 in 9b18968

   	func (c *Conn) Handshake() error {
   	c.handshakeMutex.Lock()
   	defer c.handshakeMutex.Unlock()
   	if err := c.handshakeErr; err != nil {
   	return err
   	}
   	if c.handshakeComplete() {
   	return nil
   	}
   	c.in.Lock()
   	defer c.in.Unlock()

2. Conn.in.Lock() -> Conn.handshakeMutex.Lock()
   In Read(): c.in.Lock() is on L1243. handlePostHandshakeMessage() is on L1251.
   

   go/src/crypto/tls/conn.go

   Lines 1233 to 1251 in 9b18968

   	func (c *Conn) Read(b []byte) (int, error) {
   	if err := c.Handshake(); err != nil {
   	return 0, err
   	}
   	if len(b) == 0 {
   	// Put this after Handshake, in case people were calling
   	// Read(nil) for the side effect of the Handshake.
   	return 0, nil
   	}
   	c.in.Lock()
   	defer c.in.Unlock()
   	for c.input.Len() == 0 {
   	if err := c.readRecord(); err != nil {
   	return 0, err
   	}
   	for c.hand.Len() > 0 {
   	if err := c.handlePostHandshakeMessage(); err != nil {

   
   Then handleRenegotiation() is on L1178.
   

   go/src/crypto/tls/conn.go

   Lines 1176 to 1179 in 9b18968

   	func (c *Conn) handlePostHandshakeMessage() error {
   	if c.vers != VersionTLS13 {
   	return c.handleRenegotiation()
   	}

   
   Conn.handshakeMutex.Lock() is on L1164
   

   go/src/crypto/tls/conn.go

   Lines 1164 to 1172 in 9b18968

   	c.handshakeMutex.Lock()
   	defer c.handshakeMutex.Unlock()
   	atomic.StoreUint32(&c.handshakeStatus, 0)
   	if c.handshakeErr = c.clientHandshake(); c.handshakeErr == nil {
   	c.handshakes++
   	}
   	return c.handshakeErr
   	}

What did you expect to see?

--

What did you see instead?

--

[networkimprov]

cc @FiloSottile

[dmitshur]

Also /cc @katiehockman.