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

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
Assignees
Milestone

Comments

@sabey
Copy link

@sabey sabey commented Dec 25, 2016

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

This comment has been minimized.

Copy link
Contributor

@rsc 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

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jan 16, 2017

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

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Feb 8, 2017

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

@gopherbot gopherbot closed this in f02dda5 Feb 9, 2017
@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
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
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>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.