bug#65211: 30.0.50; threads and accept-process-output

[Helmut Eller]

On Thu, Aug 10 2023, Robert Pluim wrote:

> Hmm. Maybe putting it in `deactivate_process' is better (not that Iʼve
> tested 😺)

After a night of sleep, it occurred to me that this could also be
considered a problem of incomplete initialization of the
fd_callback_info[fd] struct.

So I wondered who is supposed to initialize it.  Unfortunately, there is
half a dozen of add_<foo>_fd functions that set various bits, some call
each other, some duplicate code.  None of them sets the waiting_thread
slot.  And there is no single point that clears everything out (except
init_process_emacs).

I also discovered that recompute_max_desc considers an fd unused if the
.flags field is zero.  Luckily, recompute_max_desc is only called in
three places: delete_write_fd, delete_keyboard_wait_descriptor and
deactivate_process.  The call in deactivate_process seems redundant as
it calls the other two anyway.  It seems easier to re-initialize the
struct when it becomes unused than to initialize it when it is used the
first time.

So I'm proposing the patch below that moves the re-initialization to one
single place, namely: recompute_max_desc.

Helmut

>From 675a76b297e9f050f719489a1419f0278ad8e9ae Mon Sep 17 00:00:00 2001
From: Helmut Eller <eller.helmut@gmail.com>
Date: Fri, 11 Aug 2023 10:31:24 +0200
Subject: [PATCH] Re-initialize fd_callback_data[fd] more thoroughly

The waiting_thread field was not re-initialized properly when a a
closed file descriptor resued by another process. (bug#65211)

As recompute_max_desc must be called when a file descriptor becomes
unused, we can do some re-initialization there.

* src/process.c (recompute_max_desc): Take the fd that was just
deleted as argument fd.  If the flags field is 0, reset the rest of
the struct too.
(delete_write_fd, delete_keyboard_wait_descriptor): Update callers
accordingly.
(delete_read_fd): This is now just an alias for
delete_keyboard_wait_descriptor.
(deactivate_process): Remove the call to recompute_max_desc, as
delete_read_fd is called anyway.
---
 src/process.c | 46 ++++++++++++++++------------------------------
 1 file changed, 16 insertions(+), 30 deletions(-)

diff --git a/src/process.c b/src/process.c
index 08cb810ec13..cd5fd97e9d6 100644
--- a/src/process.c
+++ b/src/process.c
@@ -507,13 +507,6 @@ void
 delete_read_fd (int fd)
 {
   delete_keyboard_wait_descriptor (fd);
-
-  eassert (0 <= fd && fd < FD_SETSIZE);
-  if (fd_callback_info[fd].flags == 0)
-    {
-      fd_callback_info[fd].func = 0;
-      fd_callback_info[fd].data = 0;
-    }
 }
 
 /* Add a file descriptor FD to be monitored for when write is possible.
@@ -544,19 +537,22 @@ add_non_blocking_write_fd (int fd)
 }
 
 static void
-recompute_max_desc (void)
+recompute_max_desc (int fd)
 {
-  int fd;
-
+  eassert (0 <= fd && fd < FD_SETSIZE);
   eassert (max_desc < FD_SETSIZE);
-  for (fd = max_desc; fd >= 0; --fd)
-    {
+
+  if (fd_callback_info[fd].flags == 0)
+    fd_callback_info[fd] = (struct fd_callback_data){ 0 };
+
+  if (fd == max_desc)
+    for (; fd >= 0; --fd)
       if (fd_callback_info[fd].flags != 0)
-       {
-         max_desc = fd;
-         break;
-       }
-    }
+        {
+          max_desc = fd;
+          break;
+        }
+
   eassert (max_desc < FD_SETSIZE);
 }
 
@@ -569,17 +565,10 @@ delete_write_fd (int fd)
   if ((fd_callback_info[fd].flags & NON_BLOCKING_CONNECT_FD) != 0)
     {
       if (--num_pending_connects < 0)
-       emacs_abort ();
+        emacs_abort ();
     }
   fd_callback_info[fd].flags &= ~(FOR_WRITE | NON_BLOCKING_CONNECT_FD);
-  if (fd_callback_info[fd].flags == 0)
-    {
-      fd_callback_info[fd].func = 0;
-      fd_callback_info[fd].data = 0;
-
-      if (fd == max_desc)
-       recompute_max_desc ();
-    }
+  recompute_max_desc (fd);
 }
 
 static void
@@ -4809,8 +4798,6 @@ deactivate_process (Lisp_Object proc)
       delete_read_fd (inchannel);
       if ((fd_callback_info[inchannel].flags & NON_BLOCKING_CONNECT_FD) != 0)
        delete_write_fd (inchannel);
-      if (inchannel == max_desc)
-       recompute_max_desc ();
     }
 }
 
@@ -8134,8 +8121,7 @@ delete_keyboard_wait_descriptor (int desc)
 
   fd_callback_info[desc].flags &= ~(FOR_READ | KEYBOARD_FD | PROCESS_FD);
 
-  if (desc == max_desc)
-    recompute_max_desc ();
+  recompute_max_desc (desc);
 #endif
 }
 
-- 
2.39.2