Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

crypto/tls: c.ConnectionState deadlock when called from c.conn.Close #18426

Closed
sabey opened this issue Dec 25, 2016 · 3 comments
Closed

crypto/tls: c.ConnectionState deadlock when called from c.conn.Close #18426

sabey opened this issue Dec 25, 2016 · 3 comments
Labels
FrozenDueToAge
Milestone
Go1.9Early

Comments

@sabey
Copy link

sabey commented Dec 25, 2016 •
edited

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

go1.7

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

linux/amd64

What did you do?

	c.handshakeMutex.Lock()
	defer c.handshakeMutex.Unlock()

	if c.handshakeComplete {
		alertErr = c.sendAlert(alertCloseNotify)
	}

	if err := c.conn.Close(); err != nil {
		// c.conn.Close() calls c.ConnectionState() before returning
		// this will cause a deadlock
		// can the defer be removed from c.handshakeMutex.Unlock() before c.conn.Close()?
		return err
	}

	return alertErr

}

What did you expect to see?

Close() will call c.handshakeMutex.Lock()
Close() will deadlock if c.conn.Close() calls ConnectionState()
https://golang.org/src/crypto/tls/conn.go?s=34139:34345#L1163
ConnectionState() will call c.handshakeMutex.Lock()
https://golang.org/src/crypto/tls/conn.go?s=36916:36964#L1271

What did you see instead?

Is there a reason I shouldn't be able to call ConnectionState from inside of my net.Conn that *tls.Conn is wrapped around?
Could the defer unlock be replaced?

thank you
@rsc
Copy link
Contributor

rsc commented Jan 4, 2017

/cc @agl

@rsc rsc added this to the Go1.9Early milestone Jan 4, 2017
@agl agl self-assigned this Jan 16, 2017
@gopherbot
Copy link

CL https://golang.org/cl/35241 mentions this issue.

@gopherbot
Copy link

CL https://golang.org/cl/36561 mentions this issue.

@golang golang locked and limited conversation to collaborators Feb 9, 2018
FiloSottile pushed a commit to FiloSottile/go that referenced this issue Oct 12, 2018
@agl
crypto/tls: don't hold lock when closing underlying net.Conn.
0fc02e2 
There's no need to hold the handshake lock across this call and it can
lead to deadlocks if the net.Conn calls back into the tls.Conn.

Fixes golang#18426.

Change-Id: Ib1b2813cce385949d970f8ad2e52cfbd1390e624
Reviewed-on: https://go-review.googlesource.com/36561
Run-TryBot: Adam Langley <agl@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
FiloSottile pushed a commit to FiloSottile/go that referenced this issue Oct 12, 2018
@agl
crypto/tls: don't hold lock when closing underlying net.Conn.
fedcc1e 
There's no need to hold the handshake lock across this call and it can
lead to deadlocks if the net.Conn calls back into the tls.Conn.

Fixes golang#18426.

Change-Id: Ib1b2813cce385949d970f8ad2e52cfbd1390e624
Reviewed-on: https://go-review.googlesource.com/36561
Run-TryBot: Adam Langley <agl@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@rsc rsc unassigned agl Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.9Early
Development

No branches or pull requests
4 participants
@agl @rsc @sabey @gopherbot