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

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

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

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

> Did you succeed in understanding what else has to happen before that
> flag could be safely reset?

Yes. The TCP connection needs to be established, the socket descriptor removed from write monitoring (because it is now connected) and added to read monitoring (so that we can get incoming traffic).

This suggests a second solution: remove the condition on line 3277 on the grounds that since the TLS handshake succeeded, we are evidently connected; then remove the write monitoring and add the read monitoring before calling the sentinel:


[-- Attachment #2: alt.diff --]
[-- Type: application/octet-stream, Size: 649 bytes --]

diff --git a/src/process.c b/src/process.c
index b8c3e4ecfb..7d2a9e9a62 100644
--- a/src/process.c
+++ b/src/process.c
@@ -3274,8 +3274,13 @@ finish_after_tls_connection (Lisp_Object proc)
       pset_status (p, Qfailed);
       deactivate_process (proc);
     }
-  else if ((fd_callback_info[p->outfd].flags & NON_BLOCKING_CONNECT_FD) == 0)
+  else
     {
+      delete_write_fd (p->outfd);
+
+      if (0 <= p->infd && !EQ (p->filter, Qt) && !EQ (p->command, Qt))
+	add_process_read_fd (p->infd);
+
       /* If we cleared the connection wait mask before we did the TLS
 	 setup, then we have to say that the process is finally "open"
 	 here. */

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


> And anyway, if those conditions are not yet set, I wonder why are we
> calling finish_after_tls_connection at that place?

There's no harm in calling `gnutls_handshake`; it will just return E_AGAIN if the connection hasn't been established. On the other hand there's little point in doing so until we have a connection.

Which suggests a third solution: do the handshake right away after establishing the connection. That would go into the code somewhere before line 5900, which right now is a condition that I don't quite understand. I think Lars wrote it but apparently forgot all about it (happens to everyone once in a while).

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.