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>

diff --git a/doc/go1.16.html b/doc/go1.16.html
index 99e8e3c980d23e3d99d6c67576794611e44d8388..a97c369885580fdc76fd85bb83f6b10e89ec2fc5 100644 (file)
--- 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 f1d4cb926c0ff1b4b1ed0a5a7f8946f210515cf0..ada19d6e7ae6e26b0c2940306bc98648fe1b70dc 100644 (file)
--- 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
 }