[PATCH] Cygwin: Implement 'reserved' marker in fdtable entries

[Mark Geisert]

Hi Christian,

Thank you for the review.  I've inlined answers below...

On 5/22/2026 7:21 AM, Christian Franke wrote:
> Jon Turney wrote:
>> On 22/05/2026 08:28, Mark Geisert wrote:
>>> ...
>>>
>>> The notion is that an fdtable entry provided by cygheap_fdnew is marked
>>> so that another thread can't obtain it.  Care is taken to reset the
>>> marker when the entry is no longer needed.  Actually, in the usual case
>>> the marker is overwritten with a pointer to an fhandler_base structure,
>>> by the reserving thread, as the syscall completes.
>>>
>>> Reported-by: Christian Franke <Christian.Franke@t-online.de>
>>> Addresses: https://cygwin.com/pipermail/cygwin/2026-May/259664.html
>>> Signed-off-by: Mark Geisert <mark@maxrnd.com>
>>> Fixes: e859706578ba (* autoload.cc (NtCreateFile): Add.)
>>
>> Thanks!
>>
>> This all seems fine and reasonable, but I have a couple of small 
>> comments.
> 
> A test with an enhanced version of the STC was successful.
> I could push this version (attached) to cygwin-apps/stc if desired.

That sounds great to me!

[...]
>>> @@ -595,7 +599,11 @@ class cygheap_fdnew : public cygheap_fdmanip
>>>       else
>>>         fd = cygheap->fdtab.find_unused_handle (seed_fd + 1);
>>>       if (fd >= 0)
>>> -      locked = lockit;
>>> +      {
>>> +        locked = lockit;
>>> +        /* mark as "reserved" for open(), or other syscall, in 
>>> progress */
>>> +        cygheap->fdtab[fd] = (fhandler_base *)(int64_t) fd;
>>
>> So, we're already relying on "a small integer cast to pointer can't 
>> collide with an actual pointer value we might get" (which is fine).
>>
>> But then there's no reason why we can't use a distinct constant (like 
>> 1 or -1), to indicate a reserved slow throughout, which would make 
>> this easier to understand?
> 
> If the current method is kept, I would suggest to change the cast to:
>    (fhandler_base *)(intptr_t) fd

That looks good; I'm thinking of a #define defining the expression that 
we decide on to lessen code clutter.

>>
>>> ...
>>> @@ -607,7 +615,18 @@ class cygheap_fdnew : public cygheap_fdmanip
>>>     ~cygheap_fdnew ()
>>>     {
>>>       if (cygheap->fdtab[fd])
>>> -      cygheap->fdtab[fd]->inc_refcnt ();
>>> +      {
>>> +        /* check if fdtab entry is a "reserved" marker */
>>> +        if (cygheap->fdtab[fd] == (fhandler_base *)(int64_t) fd)
>>> +          {
>>> +            /* remove "reserved" marker */
>>> +            cygheap->fdtab.lock ();
>>> +            cygheap->fdtab[fd] = NULL;
>>> +            cygheap->fdtab.unlock ();
>>> +          }
>>> +        else
>>> +          cygheap->fdtab[fd]->inc_refcnt ();
>>> +      }
> 
> Are the fdtab.lock()/unlock() calls really needed here?
> 
> If yes, this variant prevents nested lock()ing and leaves the unlock() 
> for the base class dtor:
> 
>            {
>              /* remove "reserved" marker */
>             if (!locked)
>                {
>                  cygheap->fdtab.lock ();
>                  locked = true;
>                }
>              cygheap->fdtab[fd] = NULL;
>            }

I'm glad you raised this question!  I do like your variant better.
I would update my patch to have this coding.

But your mentioning the lock being unlocked in the dtor made me look at 
the classes again (in cygheap.h).  The default on cygheap_fdnew() is to 
lock the fdtable lock in the ctor.  And now I see the lock is unlocked 
in the dtor.  SMH that means the fdtable is locked for the same duration 
of time that my proposed "reserved" flag covers!

This shows us (me especially) that we don't currently have concurrent 
open()s between threads as I assumed we did.  Something like my scheme 
could allow them, maybe, but the fdtable locking would have to be given 
up to achieve it.  Maybe this is something for the future...

The upshot is that only the patch to syscalls.cc is needed to fix the 
issue you reported.  The other changes are subject to withdrawal...

Comments welcome from anybody reading this.
Sheepishly,

..mark