bug#49449: 28: TLS connection never gets to "open" stage

["Mattias Engdegård" <mattiase@acm.org>]

[-- Attachment #1: Type: text/plain, Size: 992 bytes --]

11 juli 2021 kl. 17.01 skrev Eli Zaretskii <eliz@gnu.org>:

>> I still favour the less intrusive patch posted previously (adding a condition at line 5235) since it avoids duplication; there is already far too much of that in the code (everything seems to be done in at least two places). The code is obviously in the need of restructuring, but we shouldn't conflate that effort with fixing this specific bug.
> 
> I tend to agree.

Attached is the patch that I intend to push if there are no objections. The actual change is the same as before and I anticipate no trouble arising from it but tests are usually more fragile.

This issue could very well be the root cause of or at least connected to other bugs: maybe bug#36017 or bug#34341? In any case it's good to see it fixed; it annoyed me (with GNU ELPA in particular) for quite some time and the various unsatisfactory workarounds suggested each time this came up (such as using HTTP instead of HTTPS) are no longer required.


[-- Attachment #2: 0001-Block-TLS-handshake-until-TCP-connection-established.patch --]
[-- Type: application/octet-stream, Size: 3703 bytes --]

From 88b0b7e0dd0daec88eab7dde0c9bd3263d8b52de Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Mon, 12 Jul 2021 13:58:28 +0200
Subject: [PATCH] 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 b8c3e4ecfb..c3186eed75 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 1774f2fc74..9bab523708 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 @@ process-sentinel-interrupt-event
       ;; ...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
-- 
2.21.1 (Apple Git-122.3)