master 7a803ec: Block TLS handshake until TCP connection established

[Mattias Engdegård]

branch: master
commit 7a803ecd3d455999cfc9266fa219d58109fac786
Author: Mattias Engdegård <mattiase@acm.org>
Commit: Mattias Engdegård <mattiase@acm.org>

    Block TLS handshake until TCP connection established
    
    If a TLS handshake is attempted before the completion of an
    asynchronous TCP connection has been ascertained, our local state will
    not be set up correctly for further progress and the sentinel "open"
    event will never be sent.  This can occur if sufficient time passes
    after the initiation of an async TCP connection so that by the time
    `wait_reading_process_output` is called, the connection has already
    been established on the TCP level.
    
    This somewhat timing-sensitive bug has plagued HTTPS connections on
    some platforms, notably macOS, for a long time (bug#49449).
    
    * src/process.c (wait_reading_process_output): Gate the TLS handshake
    by the NON_BLOCKING_CONNECT_FD flag.  The flag will be cleared as soon
    as the TCP socket is found to be writable.
    * test/src/process-tests.el (process-async-https-with-delay):
    New test.
---
 src/process.c             |  5 ++++-
 test/src/process-tests.el | 30 ++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/src/process.c b/src/process.c
index b8c3e4e..c3186ee 100644
--- a/src/process.c
+++ b/src/process.c
@@ -5232,7 +5232,10 @@ wait_reading_process_output (intmax_t time_limit, int 
nsecs, int read_kbd,
 #ifdef HAVE_GNUTLS
                /* Continue TLS negotiation. */
                if (p->gnutls_initstage == GNUTLS_STAGE_HANDSHAKE_TRIED
-                   && p->is_non_blocking_client)
+                   && p->is_non_blocking_client
+                   /* Don't proceed until we have established a connection. */
+                   && !(fd_callback_info[p->outfd].flags
+                        & NON_BLOCKING_CONNECT_FD))
                  {
                    gnutls_try_handshake (p);
                    p->gnutls_handshakes_tried++;
diff --git a/test/src/process-tests.el b/test/src/process-tests.el
index 1774f2f..9bab523 100644
--- a/test/src/process-tests.el
+++ b/test/src/process-tests.el
@@ -28,6 +28,7 @@
 (require 'puny)
 (require 'subr-x)
 (require 'dns)
+(require 'url-http)
 
 ;; Timeout in seconds; the test fails if the timeout is reached.
 (defvar process-test-sentinel-wait-timeout 2.0)
@@ -916,5 +917,34 @@ Return nil if FILENAME doesn't exist."
       ;; ...and the change description should be "interrupt".
       (should (equal '("interrupt\n") events)))))
 
+(ert-deftest process-async-https-with-delay ()
+  "Bug#49449: asynchronous TLS connection with delayed completion."
+  (skip-unless (and internet-is-working (gnutls-available-p)))
+  (let* ((status nil)
+         (buf (url-http
+                 #s(url "https" nil nil "elpa.gnu.org" nil
+                        "/packages/archive-contents" nil nil t silent t t)
+                 (lambda (s) (setq status s))
+                 '(nil) nil 'tls)))
+    (unwind-protect
+        (progn
+          ;; Busy-wait for 1 s to allow for the TCP connection to complete.
+          (let ((delay 1.0)
+                (t0 (float-time)))
+            (while (< (float-time) (+ t0 delay))))
+          ;; Wait for the entire operation to finish.
+          (let ((limit 4.0)
+                (t0 (float-time)))
+            (while (and (null status)
+                        (< (float-time) (+ t0 limit)))
+              (sit-for 0.1)))
+          (should status)
+          (should-not (assq :error status))
+          (should buf)
+          (should (> (buffer-size buf) 0))
+          )
+      (when buf
+        (kill-buffer buf)))))
+
 (provide 'process-tests)
 ;;; process-tests.el ends here