Subject: Re: bug#79201: 30.1.90; set-process-thread can permanently break fd_callback_info slots

From: Spencer Baugh <[email-masked]>
Date: Fri, 15 Aug 2025 10:22:20 -0400

Eli Zaretskii <[email-masked]> writes:

>> From: Spencer Baugh <[email-masked]>
>> Cc: [email-masked],  [email-masked],  [email-masked],
>>    [email-masked]
>> Date: Thu, 14 Aug 2025 12:21:15 -0400
>> 
>> The cause is that recompute_max_desc runs when a file descriptor is
>> deleted, and reduces max_desc.  But when it does so, that means
>> clear_waiting_thread_info will not get the chance to clear the
>> waiting_thread field in fd_callback_info slots above the new max_desc.
>
> Makes sense, thanks.
>
>> My initial thought was that recompute_max_desc should zero the
>> fd_callback_info[fd] entry when it reduces max_desc.  But it turns out
>> to be equivalent, and simpler, to zero fd_callback_info[desc] completely
>> instead of just setting flags to 0 right before we call
>> recompute_max_desc.
>> 
>> I do this in the attached patch, which fixes the bug.
>
> Thanks, the patch seems OK with a minor comment:
>
>> --- a/src/process.c
>> +++ b/src/process.c
>> @@ -574,8 +574,7 @@ delete_write_fd (int fd)
>>    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;
>> +      fd_callback_info[fd] = (struct fd_callback_data) {};
>
> Please just clear the .thread and .waiting_thread members.  This way,
> the code is easier to read, and it is also easier to find all the
> places which assign something to these members.

Agreed.  Done in the attached patch.

>From e6bddffd8b8e11e598f2ca75d7915184d38def4e Mon Sep 17 00:00:00 2001
From: Spencer Baugh <[email-masked]>
Date: Thu, 14 Aug 2025 12:17:23 -0400
Subject: [PATCH] Zero fd_callback_info when deleting an fd

.waiting_thread and .thread could be left set to non-NULL values
in a deleted fd_callback_info entry.  These would never be
cleared by e.g. clear_waiting_thread_info since that only clears
fd_callback_info entries up to max_desc.  Clear fd_callback_info
entirely when deleting an entry.

* src/process.c (clear_fd_callback_data): Add.
(delete_write_fd, delete_keyboard_wait_descriptor): Call
clear_fd_callback_data. (bug#79201)
(delete_read_fd): Remove duplicated clearing code.
(deactivate_process): Remove duplicate recompute_max_desc.
---
 src/process.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/src/process.c b/src/process.c
index 1a5c99139a6..042df7108eb 100644
--- a/src/process.c
+++ b/src/process.c
@@ -466,6 +466,16 @@ make_lisp_proc (struct Lisp_Process *p)
   struct thread_state *waiting_thread;
 } fd_callback_info[FD_SETSIZE];
 
+static void
+clear_fd_callback_data(struct fd_callback_data* elem)
+{
+  elem->func = NULL;
+  elem->data = NULL;
+  elem->flags = 0;
+  elem->thread = NULL;
+  elem->waiting_thread = NULL;
+}
+
 
 /* Add a file descriptor FD to be monitored for when read is possible.
    When read is possible, call FUNC with argument DATA.  */
@@ -507,13 +517,6 @@ add_process_read_fd (int fd)
 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.
@@ -574,8 +577,7 @@ delete_write_fd (int fd)
   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;
+      clear_fd_callback_data(&fd_callback_info[fd]);
 
       if (fd == max_desc)
 	recompute_max_desc ();
@@ -4809,8 +4811,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 ();
     }
 }
 
@@ -8282,7 +8282,7 @@ delete_keyboard_wait_descriptor (int desc)
 #ifdef subprocesses
   eassert (desc >= 0 && desc < FD_SETSIZE);
 
-  fd_callback_info[desc].flags &= ~(FOR_READ | KEYBOARD_FD | PROCESS_FD);
+  clear_fd_callback_data(&fd_callback_info[desc]);
 
   if (desc == max_desc)
     recompute_max_desc ();
-- 
2.39.3