Re: [PATCH] Cygwin: open: Add missing unlock on error

Hi Takashi,

I independently reviewed this and reached the same conclusion: the fix is
correct and minimal. I see it already landed on master as 0d3ea0ee653f
(with Mark's Reviewed-by), so it's kind of a belated review.

On Mon, 3 Aug 2026, Takashi Yano wrote:

> the commit 524d75ff7398 ("Cygwin: open: Unlock fdtab before open_
> with_arch()") introduced a bug that fdtab lock was not unlocked
> when new fd cannot be allocated due to an error.

Worth restating how much this matters, because the leaked lock is not
fd-table-local. The dtable lock acquires and releases
`lock_process::locker`, a process-wide recursive muto that also guards
`fork`, exec/spawn, `dup`, and every `open` and `close`. So the pre-fix
code let the owning thread hold it forever, turning a transient
out-of-descriptors failure (`EMFILE`) into a permanent, process-wide
deadlock. That is why it is a showstopper even though the trigger is rare.

> This patch adds missing unlock for fdtab lock on error.
>=20
> Fixes: 524d75ff7398 ("Cygwin: open: Unlock fdtab before open_with_arch()=
")
> Suggested-by: Johannes Schindelin <[email protected]>
> Signed-off-by: Takashi Yano <[email protected]>
> Reviewed-by:
> ---
>  winsup/cygwin/syscalls.cc | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>=20
> diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
> index e3ba8c65c..8373482e9 100644
> --- a/winsup/cygwin/syscalls.cc
> +++ b/winsup/cygwin/syscalls.cc
> @@ -1554,7 +1554,10 @@ open (const char *unix_path, int flags, ...)
>        cygheap->fdtab.lock();
>        fd =3D cygheap->fdtab.find_unused_handle ();
>        if (fd < 0)
> -	__leave;		/* errno already set */
> +	{
> +	  cygheap->fdtab.unlock();
> +	  __leave;		/* errno already set */
> +	}

This is safe and addresses the issue. At that early exit the lock is
unconditionally still held: neither `find_unused_handle()` nor `extend()`
touches it; they just return a negative value on failure. So exactly one
unlock is correct, with no risk of a double-release.

The rest of the error handling stays sound. The return value is still
negative and the reserved index is still negative, so the end-of-function
cleanup (which only runs for a non-negative index) is correctly skipped,
while the built `fhandler` is still freed. And this is the only early exit
between the lock and its matching unlock.

Incidentally, the version that landed differs from the posted hunk in
exactly one whitespace detail: the new call is written
`cygheap->fdtab.unlock ();` with a space before the parenthesis (GNU
style). No functional difference.

To reiterate the scope from the v2 discussion: this addresses the first of
the two follow-ups, releasing the lock when no descriptor is available.

For the second item, the reserved-marker design so that a
reserved-but-not-yet-open descriptor does not look fully open to the rest
of the fd table, you sent two alternative patches, which I will review
next.

Ciao,
Johannes

>        cygheap->fdtab[fd] =3D fh; /* tentative setting to mark as used *=
/
>        cygheap->fdtab.unlock();
> =20
> --=20
> 2.51.0
>=20
>=20