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

Previous Next

Full log

View this message in rfc822 format

From: Spencer Baugh <sbaugh <at> janestreet.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 79334 <at> debbugs.gnu.org, eggert <at> cs.ucla.edu, dmitry <at> gutov.dev
Subject: bug#79334: [PATCH] Don't release thread select lock unnecessarily
Date: Fri, 29 Aug 2025 09:45:07 -0400

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Spencer Baugh <sbaugh <at> janestreet.com>
>> Cc: Paul Eggert <eggert <at> cs.ucla.edu>,  79334 <at> debbugs.gnu.org, Dmitry Gutov
>>   <dmitry <at> gutov.dev>
>> Date: Fri, 29 Aug 2025 09:07:50 -0400
>> 
>> Eli Zaretskii <eliz <at> gnu.org> writes:
>> 
>> > 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.
>> 
>> Yes.  I think there's multiple problems in this code and I haven't
>> tracked them all down yet.
>> 
>> That being said, I'm confident that it's incorrect and unnecessary to be
>> using thread_select instead of pselect here, so we should stop doing
>> that in addition to any other fix.
>
> If we fix status_notify, what other reasons remain for not calling
> thread_select there?

Primarily: Calling thread_select here complicates reasoning about the
code.  We should not release the lock when not necessary, because it
makes it harder to think about possible thread interleavings.  And it's
not necessary here because this pselect call returns immediately.

(The only reason the pselect call wouldn't return immediately is if we
released the lock and then switched to another thread)

Also, avoiding thread_select here will slightly improve performance by
avoiding lock operations.

Previous Next

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