GNU bug report logs - #79334
[PATCH] Don't release thread select lock unnecessarily

Previous Next

Full log

View this message in rfc822 format

From: Eli Zaretskii <eliz <at> gnu.org>
To: Spencer Baugh <sbaugh <at> janestreet.com>, Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 79334 <at> debbugs.gnu.org
Subject: bug#79334: [PATCH] Don't release thread select lock unnecessarily
Date: Fri, 29 Aug 2025 15:24:59 +0300

> From: Spencer Baugh <sbaugh <at> janestreet.com>
> Date: Thu, 28 Aug 2025 17:08:43 -0400
> 
> Previously, we were using thread_select to release the thread
> lock around two different calls to select in
> wait_reading_process_output.  The main call to select was fine,
> but we also used thread_select for a pselect call which returns
> immediately (with a timeout of 0) and is supposed to just check
> if any file descriptors are active.
> 
> If we actually thread switch at that pselect, it's likely to
> break internal state for Emacs: namely, the call to
> status_notify immediately after can close file descriptors which
> another thread is selecting on, causing that thread to get EBADF
> from select and then call emacs_abort.
> 
> We don't need to thread switch here, so don't.
> 
> * src/process.c (wait_reading_process_output): Remove
> unnecessary thread_select wrapper.

Thanks.

However, I don't think I follow the logic behind this change.  I agree
that status_notify does things that can be dangerous (more about that
below), but the call to status_notify will happen whether we call
thread_select or pselect directly, so if status_notify can do some
damage, it will do that regardless of whether we call pselect directly
or not.  I think you assume that the problematic call to status_notify
will happen in another thread, if we allow switching threads at this
point, is that right?  IOW, I think you assume that, if we switch to
another thread, that other thread could call status_notify, and thus
close some descriptors on which the current thread, the one which
calls thread_select at this point, is waiting.  If that is your
assumption, then the actual situation could also be the opposite: the
current thread, the one where you want to call pselect, will call
status_notify after pselect returns, and that will close some
descriptors used by other threads.  In that case, calling pselect
directly will make things worse, not better, because it makes this
closing of descriptors imminent.

So, while I agree we have a problem here, I think the place and the
way we should solve it are different.

Which brings us to status_notify.  It indeed can close the descriptors
of processes which terminated, so we should probably make it safer.
For example, we could leave alone descriptors marked with non-NULL
waiting_thread if that thread is still alive and is different from the
current thread.  Would that fix the problem?  If yes, I think we
should install such a change.

We should perhaps also understand better what happens with the current
code if and when descriptors are closed by some other thread in this
scenario.  A thread using such a descriptor could be either stuck in
the pselect call, or it could be before or after the call to pselect.
Paul, what happens if a descriptor passed to pselect becomes closed
while pselect waits? does pselect return with EBADF, or does pselect
handle that gracefully, like by considering such descriptors to be
never "ready"?

If the thread is before or after pselect, then either calling pselect
or attempting to read from a closed descriptor after pselect returns
will cause EBADF.  Is our code prepared to deal with such a situation
(e.g., by ignoring the failed read), or will that cause some Lisp API
to signal an error or produce unexpected results?  If the latter, we
should probably handle EBADF specially, because I don't believe we
could completely prevent this from happening.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.