Re: [PATCH v2] Cygwin: open: Unlock fdtab before open_with_arch()

Hi Takashi,

sorry for getting to reply only after you pushed this to `master`. I
wanted to take the time to double-check a couple of things, and other
responsibilities got into the way.

On Fri, 17 Jul 2026, Takashi Yano wrote:

> [...]
> diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
> index 2bea79768..e3ba8c65c 100644
> --- a/winsup/cygwin/syscalls.cc
> +++ b/winsup/cygwin/syscalls.cc
> @@ -1451,6 +1451,7 @@ extern "C" int
>  open (const char *unix_path, int flags, ...)
>  {
>    int res =3D -1;
> +  int fd =3D -1;
>    va_list ap;
>    mode_t mode =3D 0;
>    fhandler_base *fh =3D NULL;
> @@ -1550,9 +1551,12 @@ open (const char *unix_path, int flags, ...)
>        /* Reserve an fdtable entry here, before calling open_with_arch()=
 below.
>           Otherwise there's a tiny chance of hitting OPEN_MAX further on=
 which
>           could create a new file without any way for Cygwin to refer to=
 it. */
> -      cygheap_fdnew fd;
> +      cygheap->fdtab.lock();
> +      fd =3D cygheap->fdtab.find_unused_handle ();
>        if (fd < 0)
> -        __leave;		/* errno already set */
> +	__leave;		/* errno already set */
> +      cygheap->fdtab[fd] =3D fh; /* tentative setting to mark as used *=
/
> +      cygheap->fdtab.unlock();
> =20
>        if (fh->dev () =3D=3D FH_PROCESSFD && fh->pc.follow_fd_symlink ()=
)
>  	{

I see three problems here:

When `fd` is negative, the `unlock()` right below is skipped, and
`__endtry` only unlocks for non-negative ones. So an `open()` that ran out
of descriptors returns with the fdtab still locked, and the next fdtab
operation on any other thread waits for good. `cygheap_fdnew` used to
release the lock in that case; this one does not. I reproduced [*1*] it
three times out of three on a local build. It stays invisible while
single-threaded, because the muto is reentrant -- which is also why the
OPEN_MAX test still passes.

The tentative assignment puts `fh` into the table with a zero reference
count, then drops the lock across `open_with_arch()`, which for a FIFO
waits for the other end. Any lookup in that window -- an `fcntl()`, a
`close()`, a `close_range()` -- raises the count from zero to one and
lowers it straight back, freeing `fh` while `open()` is still using it and
leaving the slot pointing at nothing valid. I reproduced [*2*] it by
querying the descriptor from a second thread while the open was waiting:
the process ended with an access violation (status 0xC0000005), and the
same run without the query was clean.

The `FH_PROCESSFD` branch shows the same thing with no threads at all: it
deletes `fh` and repoints the local variable at the reopened handler, but
the table still refers to the deleted one until the re-assignment further
down.

> @@ -1580,13 +1584,23 @@ open (const char *unix_path, int flags, ...)
>  	try_to_bin (fh->pc, fh->get_handle (), DELETE,
>  		    FILE_OPEN_FOR_BACKUP_INTENT);
> =20
> -      fd =3D fh;
> +      cygheap->fdtab.lock ();
> +      cygheap->fdtab[fd] =3D fh;
> +      fh->inc_refcnt ();
> +      cygheap->fdtab.unlock ();
> +
>        if (fd <=3D 2)
>  	set_std_handle (fd);
>        res =3D fd;
>      }
>    __except (EFAULT) {}
>    __endtry
> +    if (res < 0 && fd >=3D 0)
> +      {
> +	cygheap->fdtab.lock ();
> +	cygheap->fdtab[fd] =3D NULL; /* Mark as unused */
> +	cygheap->fdtab.unlock ();
> +      }
>    if (res < 0 && fh)
>      delete fh;
>    syscall_printf ("%R =3D open(%s, %y)", res, unix_path, flags);

This is the first point where the slot and its reference count agree
again; Too late for anything that looked in between. And the cleanup only
runs for non-negative descriptors, so it never covers the case above.

Since it is already in `master`, a follow-up patch probably makes most
sense. Two things to fix: release the lock when no descriptor is
available, and stop a reserved-but-not-yet-open descriptor from looking
like a fully open one to the rest of the fdtable. Reviving the old integer
marker would mean teaching every consumer of the table about it, so it is
not a drop-in.

Ciao,
Johannes

Footnotes:

[*1*] I let GPT 5.6 write the code to reproduce this (it's a bit verbose,
as AI-generated code tends to be, but looked correct to me, hopefully you
find it useful; Originally I wanted to condense this into a better form,
but I realized that I simply lack the time to do so):

=2D- snip --
#include <errno.h>
#include <fcntl.h>
#include <limits.h>
#include <pthread.h>
#include <stdio.h>
#include <time.h>
#include <unistd.h>

static pthread_mutex_t mutex =3D PTHREAD_MUTEX_INITIALIZER;
static pthread_cond_t condition =3D PTHREAD_COND_INITIALIZER;
static int requested_stage;
static int completed_stage;
static int stage_result[3];
static int stage_errno[3];

static void
set_deadline (struct timespec *deadline, int seconds)
{
  clock_gettime (CLOCK_REALTIME, deadline);
  deadline->tv_sec +=3D seconds;
}

static int
wait_for_stage (int stage, int seconds)
{
  struct timespec deadline;
  int result =3D 0;

  set_deadline (&deadline, seconds);
  pthread_mutex_lock (&mutex);
  while (completed_stage < stage)
    {
      int err =3D pthread_cond_timedwait (&condition, &mutex, &deadline);
      if (err =3D=3D ETIMEDOUT)
        {
          result =3D ETIMEDOUT;
          break;
        }
      if (err)
        {
          result =3D err;
          break;
        }
    }
  pthread_mutex_unlock (&mutex);
  return result;
}

static void
request_stage (int stage)
{
  pthread_mutex_lock (&mutex);
  requested_stage =3D stage;
  pthread_cond_broadcast (&condition);
  pthread_mutex_unlock (&mutex);
}

static void *
descriptor_thread (void *unused)
{
  (void) unused;

  for (int stage =3D 1; stage <=3D 2; stage++)
    {
      pthread_mutex_lock (&mutex);
      while (requested_stage < stage)
        pthread_cond_wait (&condition, &mutex);
      pthread_mutex_unlock (&mutex);

      errno =3D 0;
      int result =3D close (-1);
      int saved_errno =3D errno;

      pthread_mutex_lock (&mutex);
      stage_result[stage] =3D result;
      stage_errno[stage] =3D saved_errno;
      completed_stage =3D stage;
      pthread_cond_broadcast (&condition);
      pthread_mutex_unlock (&mutex);
    }
  return NULL;
}

int
main (void)
{
  pthread_t thread;
  int opened =3D 0;
  int failure_errno =3D 0;

  setvbuf (stdout, NULL, _IONBF, 0);

  if (pthread_create (&thread, NULL, descriptor_thread, NULL))
    {
      perror ("pthread_create");
      return 1;
    }

  request_stage (1);
  if (wait_for_stage (1, 3))
    {
      printf ("baseline descriptor operation did not complete\n");
      return 1;
    }
  printf ("baseline close(-1): result=3D%d errno=3D%d\n",
          stage_result[1], stage_errno[1]);
  if (stage_result[1] !=3D -1 || stage_errno[1] !=3D EBADF)
    return 1;

  for (; opened < OPEN_MAX + 16; opened++)
    {
      errno =3D 0;
      if (open ("/dev/null", O_RDONLY) < 0)
        {
          failure_errno =3D errno;
          break;
        }
    }

  printf ("descriptor allocation stopped after %d opens: errno=3D%d\n",
          opened, failure_errno);
  if (failure_errno !=3D EMFILE)
    return 1;

  request_stage (2);
  if (wait_for_stage (2, 3) =3D=3D ETIMEDOUT)
    {
      printf ("second-thread descriptor operation remained waiting\n");
      return 0;
    }

  printf ("second-thread close(-1): result=3D%d errno=3D%d\n",
          stage_result[2], stage_errno[2]);
  return 2;
}
=2D- snap --

[*2*] Also here, I let GPT 5.6 write the code for reproducing the issue:

=2D- snip --
#include <errno.h>
#include <fcntl.h>
#include <pthread.h>
#include <stdio.h>
#include <sys/stat.h>
#include <time.h>
#include <unistd.h>

static const char *fifo_name =3D "fifo-fd-lookup-check.pipe";
static pthread_mutex_t mutex =3D PTHREAD_MUTEX_INITIALIZER;
static pthread_cond_t condition =3D PTHREAD_COND_INITIALIZER;
static int start_writer;
static int writer_done;
static int writer_fd =3D -1;
static int writer_errno;

static int
before_deadline (const struct timespec *deadline)
{
  struct timespec now;

  clock_gettime (CLOCK_MONOTONIC, &now);
  return now.tv_sec < deadline->tv_sec
         || (now.tv_sec =3D=3D deadline->tv_sec
             && now.tv_nsec < deadline->tv_nsec);
}

static void *
open_writer (void *unused)
{
  (void) unused;

  pthread_mutex_lock (&mutex);
  while (!start_writer)
    pthread_cond_wait (&condition, &mutex);
  pthread_mutex_unlock (&mutex);

  errno =3D 0;
  int fd =3D open (fifo_name, O_WRONLY);
  int saved_errno =3D errno;

  pthread_mutex_lock (&mutex);
  writer_fd =3D fd;
  writer_errno =3D saved_errno;
  writer_done =3D 1;
  pthread_cond_broadcast (&condition);
  pthread_mutex_unlock (&mutex);
  return NULL;
}

int
main (void)
{
  pthread_t writer;
  struct timespec poll_deadline;
  struct timespec pause =3D { 0, 1000000 };
  struct timespec wait_deadline;
  int anchor;
  int expected_fd;
  int query_result =3D -1;
  int reader_fd;

  setvbuf (stdout, NULL, _IONBF, 0);
  unlink (fifo_name);
  if (mkfifo (fifo_name, 0600))
    {
      perror ("mkfifo");
      return 1;
    }

  anchor =3D open ("/dev/null", O_RDONLY);
  if (anchor < 0)
    {
      perror ("open /dev/null");
      return 1;
    }
  if (pthread_create (&writer, NULL, open_writer, NULL))
    {
      perror ("pthread_create");
      return 1;
    }

  expected_fd =3D dup (anchor);
  if (expected_fd < 0)
    {
      perror ("dup");
      return 1;
    }
  close (expected_fd);
  printf ("expected FIFO writer descriptor: %d\n", expected_fd);

  pthread_mutex_lock (&mutex);
  start_writer =3D 1;
  pthread_cond_broadcast (&condition);
  pthread_mutex_unlock (&mutex);

  clock_gettime (CLOCK_MONOTONIC, &poll_deadline);
  poll_deadline.tv_sec +=3D 5;
  while (before_deadline (&poll_deadline))
    {
      errno =3D 0;
      query_result =3D fcntl (expected_fd, F_GETFD);
      if (query_result >=3D 0)
        break;
      if (errno !=3D EBADF)
        {
          printf ("fcntl(%d, F_GETFD): result=3D%d errno=3D%d\n",
                  expected_fd, query_result, errno);
          return 1;
        }
      nanosleep (&pause, NULL);
    }

  pthread_mutex_lock (&mutex);
  int completed_before_reader =3D writer_done;
  pthread_mutex_unlock (&mutex);
  printf ("provisional descriptor query: result=3D%d writer_done=3D%d\n",
          query_result, completed_before_reader);
  if (query_result < 0 || completed_before_reader)
    return 1;

  reader_fd =3D open (fifo_name, O_RDONLY | O_NONBLOCK);
  printf ("reader open: fd=3D%d errno=3D%d\n", reader_fd, errno);
  if (reader_fd < 0)
    return 1;

  clock_gettime (CLOCK_REALTIME, &wait_deadline);
  wait_deadline.tv_sec +=3D 5;
  pthread_mutex_lock (&mutex);
  while (!writer_done)
    {
      int err =3D pthread_cond_timedwait (&condition, &mutex, &wait_deadli=
ne);
      if (err)
        break;
    }
  int completed_after_reader =3D writer_done;
  int result_fd =3D writer_fd;
  int result_errno =3D writer_errno;
  pthread_mutex_unlock (&mutex);

  printf ("writer completion: done=3D%d fd=3D%d errno=3D%d\n",
          completed_after_reader, result_fd, result_errno);
  if (!completed_after_reader)
    return 2;

  if (result_fd >=3D 0)
    {
      int close_result =3D close (result_fd);
      printf ("writer close: result=3D%d errno=3D%d\n", close_result, errn=
o);
    }
  close (reader_fd);
  close (anchor);
  pthread_join (writer, NULL);
  unlink (fifo_name);
  return 0;
}
=2D- snap --

> --=20
> 2.51.0
>=20
>=20