From c9b9cd73bb7a7828d34f4a7844f16c3fbc0674dd Mon Sep 17 00:00:00 2001
From: Katie Hockman <katie@golang.org>
Date: Wed, 28 Oct 2020 15:13:33 -0400
Subject: [PATCH] crypto/tls: set Deadline before sending close notify alert

This change also documents the need to set a Deadline before
calling Read or Write.

Fixes #31224

Change-Id: I89d6fe3ecb0a0076b4c61765f61c88056f951406
Reviewed-on: https://go-review.googlesource.com/c/go/+/266037
Trust: Katie Hockman <katie@golang.org>
Run-TryBot: Katie Hockman <katie@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
---
 doc/go1.16.html        | 10 ++++++++--
 src/crypto/tls/conn.go | 22 ++++++++++++++++++----
 2 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/doc/go1.16.html b/doc/go1.16.html
index 99e8e3c980..a97c369885 100644
--- a/doc/go1.16.html
+++ b/doc/go1.16.html
@@ -237,12 +237,18 @@ Do not send CLs removing the interior tags from such phrases.
 
 <p><!-- CL 256897 -->
   I/O operations on closing or closed TLS connections can now be detected using
-  the new <a href="/pkg/net/#ErrClosed">ErrClosed</a> error.  A typical use
-  would be <code>errors.Is(err, net.ErrClosed)</code>.  In earlier releases
+  the new <a href="/pkg/net/#ErrClosed">ErrClosed</a> error. A typical use
+  would be <code>errors.Is(err, net.ErrClosed)</code>. In earlier releases
   the only way to reliably detect this case was to match the string returned
   by the <code>Error</code> method with <code>"tls: use of closed connection"</code>.
 </p>
 
+<p><!-- CL 266037 -->
+  A default deadline is set in <a href="/pkg/crypto/tls/#Conn.Close">Close</a>
+  before sending the close notify alert, in order to prevent blocking
+  indefinitely.
+</p>
+
 <h3 id="crypto/x509"><a href="/pkg/crypto/x509">crypto/x509</a></h3>
 
 <p><!-- CL 235078 -->
diff --git a/src/crypto/tls/conn.go b/src/crypto/tls/conn.go
index f1d4cb926c..ada19d6e7a 100644
--- a/src/crypto/tls/conn.go
+++ b/src/crypto/tls/conn.go
@@ -1074,6 +1074,11 @@ var (
 )
 
 // Write writes data to the connection.
+//
+// As Write calls Handshake, in order to prevent indefinite blocking a deadline
+// must be set for both Read and Write before Write is called when the handshake
+// has not yet completed. See SetDeadline, SetReadDeadline, and
+// SetWriteDeadline.
 func (c *Conn) Write(b []byte) (int, error) {
 	// interlock with Close below
 	for {
@@ -1232,8 +1237,12 @@ func (c *Conn) handleKeyUpdate(keyUpdate *keyUpdateMsg) error {
 	return nil
 }
 
-// Read can be made to time out and return a net.Error with Timeout() == true
-// after a fixed time limit; see SetDeadline and SetReadDeadline.
+// Read reads data from the connection.
+//
+// As Read calls Handshake, in order to prevent indefinite blocking a deadline
+// must be set for both Read and Write before Read is called when the handshake
+// has not yet completed. See SetDeadline, SetReadDeadline, and
+// SetWriteDeadline.
 func (c *Conn) Read(b []byte) (int, error) {
 	if err := c.Handshake(); err != nil {
 		return 0, err
@@ -1301,9 +1310,10 @@ func (c *Conn) Close() error {
 	}
 
 	var alertErr error
-
 	if c.handshakeComplete() {
-		alertErr = c.closeNotify()
+		if err := c.closeNotify(); err != nil {
+			alertErr = fmt.Errorf("tls: failed to send closeNotify alert (but connection was closed anyway): %w", err)
+		}
 	}
 
 	if err := c.conn.Close(); err != nil {
@@ -1330,8 +1340,12 @@ func (c *Conn) closeNotify() error {
 	defer c.out.Unlock()
 
 	if !c.closeNotifySent {
+		// Set a Write Deadline to prevent possibly blocking forever.
+		c.SetWriteDeadline(time.Now().Add(time.Second * 5))
 		c.closeNotifyErr = c.sendAlertLocked(alertCloseNotify)
 		c.closeNotifySent = true
+		// Any subsequent writes will fail.
+		c.SetWriteDeadline(time.Now())
 	}
 	return c.closeNotifyErr
 }
-- 
2.50.0