from:"Ken Brown via Cygwin\-patches"

Re: [PATCH 00/21] FIFO: Support multiple readers

2020-05-18 Thread Ken Brown via Cygwin-patches


On 5/18/2020 1:36 AM, Takashi Yano via Cygwin-patches wrote:

On Mon, 18 May 2020 14:25:19 +0900
Takashi Yano via Cygwin-patches  wrote:

However, mc hangs by several operations.

To reproduce this:
1. Start mc with 'env SHELL=tcsh mc -a'


I mean 'env SHELL=/bin/tcsh mc -a'


2. Select a file using up/down cursor keys.
3. Press F3 (View) key.


Thanks for the report.  I can reproduce the problem and will look into it.

Ken

Re: [PATCH 00/21] FIFO: Support multiple readers

2020-05-18 Thread Ken Brown via Cygwin-patches


Hi Takashi,

On 5/18/2020 12:03 PM, Ken Brown via Cygwin-patches wrote:

On 5/18/2020 1:36 AM, Takashi Yano via Cygwin-patches wrote:

On Mon, 18 May 2020 14:25:19 +0900
Takashi Yano via Cygwin-patches  wrote:

However, mc hangs by several operations.

To reproduce this:
1. Start mc with 'env SHELL=tcsh mc -a'


I mean 'env SHELL=/bin/tcsh mc -a'


2. Select a file using up/down cursor keys.
3. Press F3 (View) key.


Thanks for the report.  I can reproduce the problem and will look into it.


I'm not convinced that this is a FIFO bug.  I tried two things.

1. I attached gdb to mc while it was hanging and got the following backtrace 
(abbreviated):


#1  0x7ff901638037 in WaitForMultipleObjectsEx ()
#2  0x7ff901637f1e in WaitForMultipleObjects ()
#3  0x000180048df5 in cygwait () at ...winsup/cygwin/cygwait.cc:75
#4  0x00018019b1c0 in wait4 () at ...winsup/cygwin/wait.cc:80
#5  0x00018019afea in waitpid () at ...winsup/cygwin/wait.cc:28
#6  0x00018017d2d8 in pclose () at ...winsup/cygwin/syscalls.cc:4627
#7  0x00018015943b in _sigfe () at sigfe.s:35
#8  0x00010040d002 in get_popen_information () at filemanager/ext.c:561
[...]

So pclose is blocking after calling waitpid.  As far as I can tell from looking 
at backtraces of all threads, there are no FIFOs open.


2. I ran mc under strace (after exporting SHELL=/bin/tcsh), and I didn't see 
anything suspicious involving FIFOs.  But I saw many EBADF errors from fstat and 
close that don't appear to be related to FIFOs.


So my best guess at this point is that the FIFO changes just exposed some 
unrelated bug(s).


Prior to the FIFO changes, mc would get an error when it tried to open tcsh_fifo 
the second time, and it would then set


  mc_global.tty.use_subshell = FALSE;

see the mc source file subshell/common.c:1087.

Ken

Re: [PATCH 00/21] FIFO: Support multiple readers

2020-05-19 Thread Ken Brown via Cygwin-patches


On 5/19/2020 2:15 AM, Takashi Yano via Cygwin-patches wrote:

On Tue, 19 May 2020 10:26:09 +0900
Takashi Yano via Cygwin-patches  wrote:

Hi Ken,

On Mon, 18 May 2020 13:42:19 -0400
Ken Brown via Cygwin-patches  wrote:

Hi Takashi,

On 5/18/2020 12:03 PM, Ken Brown via Cygwin-patches wrote:

On 5/18/2020 1:36 AM, Takashi Yano via Cygwin-patches wrote:

On Mon, 18 May 2020 14:25:19 +0900
Takashi Yano via Cygwin-patches  wrote:

However, mc hangs by several operations.

To reproduce this:
1. Start mc with 'env SHELL=tcsh mc -a'


I mean 'env SHELL=/bin/tcsh mc -a'


2. Select a file using up/down cursor keys.
3. Press F3 (View) key.


Thanks for the report.  I can reproduce the problem and will look into it.


I'm not convinced that this is a FIFO bug.  I tried two things.

1. I attached gdb to mc while it was hanging and got the following backtrace
(abbreviated):

#1  0x7ff901638037 in WaitForMultipleObjectsEx ()
#2  0x7ff901637f1e in WaitForMultipleObjects ()
#3  0x000180048df5 in cygwait () at ...winsup/cygwin/cygwait.cc:75
#4  0x00018019b1c0 in wait4 () at ...winsup/cygwin/wait.cc:80
#5  0x00018019afea in waitpid () at ...winsup/cygwin/wait.cc:28
#6  0x00018017d2d8 in pclose () at ...winsup/cygwin/syscalls.cc:4627
#7  0x00018015943b in _sigfe () at sigfe.s:35
#8  0x00010040d002 in get_popen_information () at filemanager/ext.c:561
[...]

So pclose is blocking after calling waitpid.  As far as I can tell from looking
at backtraces of all threads, there are no FIFOs open.

2. I ran mc under strace (after exporting SHELL=/bin/tcsh), and I didn't see
anything suspicious involving FIFOs.  But I saw many EBADF errors from fstat and
close that don't appear to be related to FIFOs.

So my best guess at this point is that the FIFO changes just exposed some
unrelated bug(s).

Prior to the FIFO changes, mc would get an error when it tried to open tcsh_fifo
the second time, and it would then set

mc_global.tty.use_subshell = FALSE;

see the mc source file subshell/common.c:1087.


I looked into this problem and found pclose() stucks if FIFO
is opened.

Attached is a simple test case. It works under cygwin 3.1.4,
but stucks at pclose() under cygwin git head.

Isn't this a FIFO related issue?


In the test case, fhandler_fifo::close() called from /bin/true
seems to get into infinite loop at:

do
...
while (inc_nreaders () > 0 && !found);


Thank you!  I see the problem.  After the popen call, the original 
fhandler_fifo's fifo_reader_thread was no longer running, so it was unable to 
take ownership.


It might take a little while for me to figure out how to fix this.

Thanks for your persistence and, especially, for the test case.

Ken

[PATCH 05/21] Cygwin: FIFO: remove the arm method

2020-05-07 Thread Ken Brown via Cygwin-patches

There's no reason to check for errors when we set read_ready or
write_ready.  We don't do that for other events.
---
 winsup/cygwin/fhandler.h   |  1 -
 winsup/cygwin/fhandler_fifo.cc | 34 +++---
 2 files changed, 3 insertions(+), 32 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index c8f7a39a2..4d691a0fc 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1343,7 +1343,6 @@ public:
   void set_close_on_exec (bool val);
   void __reg3 raw_read (void *ptr, size_t& ulen);
   ssize_t __reg3 raw_write (const void *ptr, size_t ulen);
-  bool arm (HANDLE h);
   bool need_fixup_before () const { return reader; }
   int fixup_before_fork_exec (DWORD) { stop_listen_client (); return 0; }
   void init_fixup_before ();
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index fb20e5a7e..44919c19e 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -93,28 +93,6 @@ sec_user_cloexec (bool cloexec, PSECURITY_ATTRIBUTES sa, 
PSID sid)
   return cloexec ? sec_user_nih (sa, sid) : sec_user (sa, sid);
 }
 
-bool inline
-fhandler_fifo::arm (HANDLE h)
-{
-#ifdef DEBUGGING
-  const char *what;
-  if (h == read_ready)
-what = "reader";
-  else
-what = "writer";
-  debug_only_printf ("arming %s", what);
-#endif
-
-  bool res = SetEvent (h);
-  if (!res)
-#ifdef DEBUGGING
-debug_printf ("SetEvent for %s failed, %E", what);
-#else
-debug_printf ("SetEvent failed, %E");
-#endif
-  return res;
-}
-
 static HANDLE
 create_event ()
 {
@@ -348,11 +326,7 @@ fhandler_fifo::listen_client_thread ()
api_fatal ("Can't add a client handler, %E");
 
   /* Allow a writer to open. */
-  if (!arm (read_ready))
-   {
- __seterrno ();
- goto out;
-   }
+  SetEvent (read_ready);
 
   /* Listen for a writer to connect to the new client handler. */
   fifo_client_handler& fc = fc_handler[nhandlers - 1];
@@ -555,10 +529,8 @@ fhandler_fifo::open (int flags, mode_t)
  if (NT_SUCCESS (status))
{
  set_pipe_non_blocking (get_handle (), flags & O_NONBLOCK);
- if (!arm (write_ready))
-   res = error_set_errno;
- else
-   res = success;
+ SetEvent (write_ready);
+ res = success;
  goto out;
}
  else if (STATUS_PIPE_NO_INSTANCE_AVAILABLE (status))
-- 
2.21.0

[PATCH 16/21] Cygwin: FIFO: add a shared fifo_client_handler list

2020-05-07 Thread Ken Brown via Cygwin-patches

This is in a new shared memory section.  We will use it for temporary
storage of the owner's fc_handler list when we need to change owner.
The new owner can then duplicate the pipe handles from that list
before taking ownership.

Add several shared data members and methods that are needed for the
duplication process

Add methods update_my_handlers and update_shared_handlers that carry
out the duplication.

Allow the shared list to grow dynamically, up to a point.  Do this by
initially reserving a block of memory (currently 100 pages) and only
committing pages as needed.

Add methods create_shared_fc_handler, reopen_shared_fc_handler, and
remap_shared_fc_handler to create the new shared memory section,
reopen it, and commit new pages.  The first is called in open, the
second is called in dup/fork/exec, and the third is called in
update_shared_handlers if more shared memory is needed.

Modify the fifo_reader_thread function to call update_my_handlers when
it finds that there is no owner.  Also make it call
update_shared_handlers when the owner's thread terminates, so that the
new owner will have an accurate shared fc_handler list from which to
duplicate.

For convenience, add new methods cleanup_handlers and
close_all_handlers.  And add an optional arg to add_client_handler
that allows it to create a new fifo_client_handler without creating a
new pipe instance.
---
 winsup/cygwin/fhandler.h   |  43 +-
 winsup/cygwin/fhandler_fifo.cc | 253 +
 2 files changed, 269 insertions(+), 27 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 4f42cf1b8..34b209f5d 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1323,17 +1323,33 @@ struct fifo_reader_id_t
 class fifo_shmem_t
 {
   LONG _nreaders;
-  fifo_reader_id_t _owner;
+  fifo_reader_id_t _owner, _prev_owner;
   af_unix_spinlock_t _owner_lock;
 
+  /* Info about shared memory block used for temporary storage of the
+ owner's fc_handler list. */
+  LONG _sh_nhandlers, _sh_shandlers, _sh_fc_handler_committed;
+
 public:
   int inc_nreaders () { return (int) InterlockedIncrement (&_nreaders); }
   int dec_nreaders () { return (int) InterlockedDecrement (&_nreaders); }
 
   fifo_reader_id_t get_owner () const { return _owner; }
   void set_owner (fifo_reader_id_t fr_id) { _owner = fr_id; }
+  fifo_reader_id_t get_prev_owner () const { return _prev_owner; }
+  void set_prev_owner (fifo_reader_id_t fr_id) { _prev_owner = fr_id; }
+
   void owner_lock () { _owner_lock.lock (); }
   void owner_unlock () { _owner_lock.unlock (); }
+
+  int get_shared_nhandlers () const { return (int) _sh_nhandlers; }
+  void set_shared_nhandlers (int n) { InterlockedExchange (&_sh_nhandlers, n); 
}
+  int get_shared_shandlers () const { return (int) _sh_shandlers; }
+  void set_shared_shandlers (int n) { InterlockedExchange (&_sh_shandlers, n); 
}
+  size_t get_shared_fc_handler_committed () const
+  { return (size_t) _sh_fc_handler_committed; }
+  void set_shared_fc_handler_committed (size_t n)
+  { InterlockedExchange (&_sh_fc_handler_committed, (LONG) n); }
 };
 
 class fhandler_fifo: public fhandler_base
@@ -1360,24 +1376,47 @@ class fhandler_fifo: public fhandler_base
 
   HANDLE shmem_handle;
   fifo_shmem_t *shmem;
+  HANDLE shared_fc_hdl;
+  /* Dynamically growing array in shared memory. */
+  fifo_client_handler *shared_fc_handler;
 
   bool __reg2 wait (HANDLE);
   static NTSTATUS npfs_handle (HANDLE &);
   HANDLE create_pipe_instance ();
   NTSTATUS open_pipe (HANDLE&);
   NTSTATUS wait_open_pipe (HANDLE&);
-  int add_client_handler ();
+  int add_client_handler (bool new_pipe_instance = true);
   void delete_client_handler (int);
+  void cleanup_handlers ();
+  void close_all_handlers ();
   void cancel_reader_thread ();
   void record_connection (fifo_client_handler&,
  fifo_client_connect_state = fc_connected);
 
   int create_shmem ();
   int reopen_shmem ();
+  int create_shared_fc_handler ();
+  int reopen_shared_fc_handler ();
+  int remap_shared_fc_handler (size_t);
 
   int inc_nreaders () { return shmem->inc_nreaders (); }
   int dec_nreaders () { return shmem->dec_nreaders (); }
 
+  fifo_reader_id_t get_prev_owner () const { return shmem->get_prev_owner (); }
+  void set_prev_owner (fifo_reader_id_t fr_id)
+  { shmem->set_prev_owner (fr_id); }
+
+  int get_shared_nhandlers () { return shmem->get_shared_nhandlers (); }
+  void set_shared_nhandlers (int n) { shmem->set_shared_nhandlers (n); }
+  int get_shared_shandlers () { return shmem->get_shared_shandlers (); }
+  void set_shared_shandlers (int n) { shmem->set_shared_shandlers (n); }
+  size_t get_shared_fc_handler_committed () const
+  { return shmem->get_shared_fc_handler_committed (); }
+  void set_shared_fc_handler_committed (size_t n)
+  { shmem->set_shared_fc_handler_committed (n); }
+  int update_my_handlers ();
+  int update_shared_handlers ();
+
 public:
   fhandler_fifo ();
   bool

[PATCH 12/21] Cygwin: FIFO: keep track of the number of readers

2020-05-07 Thread Ken Brown via Cygwin-patches

Add data and methods to the shared memory that keep track of the
number of open readers.

Increment this number in open, dup, fork, and exec.  Decrement it in
close.  Reset read_ready if there are no readers left.
---
 winsup/cygwin/fhandler.h   |  8 
 winsup/cygwin/fhandler_fifo.cc | 22 ++
 2 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 8d6b94021..b2ee7e6b6 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1303,6 +1303,11 @@ struct fifo_client_handler
 /* Info needed by all readers of a FIFO, stored in named shared memory. */
 class fifo_shmem_t
 {
+  LONG _nreaders;
+
+public:
+  int inc_nreaders () { return (int) InterlockedIncrement (&_nreaders); }
+  int dec_nreaders () { return (int) InterlockedDecrement (&_nreaders); }
 };
 
 class fhandler_fifo: public fhandler_base
@@ -1342,6 +1347,9 @@ class fhandler_fifo: public fhandler_base
   int create_shmem ();
   int reopen_shmem ();
 
+  int inc_nreaders () { return shmem->inc_nreaders (); }
+  int dec_nreaders () { return shmem->dec_nreaders (); }
+
 public:
   fhandler_fifo ();
   bool hit_eof ();
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index 9a0db3f33..d87070ac7 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -570,8 +570,9 @@ fhandler_fifo::open (int flags, mode_t)
   SetEvent (read_ready);
   if (create_shmem () < 0)
goto err_close_writer_opening;
+  inc_nreaders ();
   if (!(cancel_evt = create_event ()))
-   goto err_close_shmem;
+   goto err_dec_nreaders;
   if (!(thr_sync_evt = create_event ()))
goto err_close_cancel_evt;
   new cygthread (fifo_reader_thread, this, "fifo_reader", thr_sync_evt);
@@ -680,7 +681,10 @@ err_close_reader:
   return 0;
 err_close_cancel_evt:
   NtClose (cancel_evt);
-err_close_shmem:
+err_dec_nreaders:
+  if (dec_nreaders () == 0)
+ResetEvent (read_ready);
+/* err_close_shmem: */
   NtUnmapViewOfSection (NtCurrentProcess (), shmem);
   NtClose (shmem_handle);
 err_close_writer_opening:
@@ -1003,15 +1007,13 @@ fhandler_fifo::close ()
 {
   if (reader)
 {
+  if (dec_nreaders () == 0)
+   ResetEvent (read_ready);
   cancel_reader_thread ();
   if (cancel_evt)
NtClose (cancel_evt);
   if (thr_sync_evt)
NtClose (thr_sync_evt);
-  /* FIXME: There could be several readers open because of
-dup/fork/exec; we should only reset read_ready when the last
-one closes. */
-  ResetEvent (read_ready);
   if (shmem)
NtUnmapViewOfSection (NtCurrentProcess (), shmem);
   if (shmem_handle)
@@ -1116,8 +1118,8 @@ fhandler_fifo::dup (fhandler_base *child, int flags)
goto err_close_handlers;
   if (!(fhf->thr_sync_evt = create_event ()))
goto err_close_cancel_evt;
-  new cygthread (fifo_reader_thread, fhf, "fifo_reader",
-fhf->thr_sync_evt);
+  inc_nreaders ();
+  new cygthread (fifo_reader_thread, fhf, "fifo_reader", 
fhf->thr_sync_evt);
 }
   return 0;
 err_close_cancel_evt:
@@ -1161,6 +1163,7 @@ fhandler_fifo::fixup_after_fork (HANDLE parent)
api_fatal ("Can't create reader thread cancel event during fork, %E");
   if (!(thr_sync_evt = create_event ()))
api_fatal ("Can't create reader thread sync event during fork, %E");
+  inc_nreaders ();
   new cygthread (fifo_reader_thread, this, "fifo_reader", thr_sync_evt);
 }
 }
@@ -1180,6 +1183,9 @@ fhandler_fifo::fixup_after_exec ()
api_fatal ("Can't create reader thread cancel event during exec, %E");
   if (!(thr_sync_evt = create_event ()))
api_fatal ("Can't create reader thread sync event during exec, %E");
+  /* At this moment we're a new reader.  The count will be
+decremented when the parent closes. */
+  inc_nreaders ();
   new cygthread (fifo_reader_thread, this, "fifo_reader", thr_sync_evt);
 }
 }
-- 
2.21.0

[PATCH 09/21] Cygwin: FIFO: make opening a writer more robust

2020-05-07 Thread Ken Brown via Cygwin-patches

- Make read_ready a manual-reset event.

- Signal read_ready in open instead of in the listen_client_thread.

- Don't reset read_ready when the listen_client thread terminates;
  instead do it in close().

- Rearrange open and change its error handling.

- Add a wait_open_pipe method that waits for a pipe instance to be
  available and then calls open_pipe.  Use it when opening a writer if
  we can't connect immediately.  This can happen if the system is
  heavily loaded and/or if many writers are trying to open
  simultaneously.
---
 winsup/cygwin/fhandler.h   |   1 +
 winsup/cygwin/fhandler_fifo.cc | 267 +
 2 files changed, 168 insertions(+), 100 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 3bc04cf13..2516c93b4 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1323,6 +1323,7 @@ class fhandler_fifo: public fhandler_base
   static NTSTATUS npfs_handle (HANDLE &);
   HANDLE create_pipe_instance (bool);
   NTSTATUS open_pipe (HANDLE&);
+  NTSTATUS wait_open_pipe (HANDLE&);
   int add_client_handler ();
   void delete_client_handler (int);
   bool listen_client ();
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index 21faf4ec2..5c3df5497 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -222,7 +222,64 @@ fhandler_fifo::open_pipe (HANDLE& ph)
  openflags & O_CLOEXEC ? 0 : OBJ_INHERIT,
  npfsh, NULL);
   sharing = FILE_SHARE_READ | FILE_SHARE_WRITE;
-  status = NtOpenFile (, access, , , sharing, 0);
+  return NtOpenFile (, access, , , sharing, 0);
+}
+
+/* Wait up to 100ms for a pipe instance to be available, then connect. */
+NTSTATUS
+fhandler_fifo::wait_open_pipe (HANDLE& ph)
+{
+  HANDLE npfsh;
+  HANDLE evt;
+  NTSTATUS status;
+  IO_STATUS_BLOCK io;
+  ULONG pwbuf_size;
+  PFILE_PIPE_WAIT_FOR_BUFFER pwbuf;
+  LONGLONG stamp;
+  LONGLONG orig_timeout = -100 * NS100PERSEC / MSPERSEC;   /* 100ms */
+
+  status = npfs_handle (npfsh);
+  if (!NT_SUCCESS (status))
+return status;
+  if (!(evt = create_event ()))
+api_fatal ("Can't create event, %E");
+  pwbuf_size
+= offsetof (FILE_PIPE_WAIT_FOR_BUFFER, Name) + get_pipe_name ()->Length;
+  pwbuf = (PFILE_PIPE_WAIT_FOR_BUFFER) alloca (pwbuf_size);
+  pwbuf->Timeout.QuadPart = orig_timeout;
+  pwbuf->NameLength = get_pipe_name ()->Length;
+  pwbuf->TimeoutSpecified = TRUE;
+  memcpy (pwbuf->Name, get_pipe_name ()->Buffer, get_pipe_name ()->Length);
+  stamp = get_clock (CLOCK_MONOTONIC)->n100secs ();
+  bool retry;
+  do
+{
+  retry = false;
+  status = NtFsControlFile (npfsh, evt, NULL, NULL, , FSCTL_PIPE_WAIT,
+   pwbuf, pwbuf_size, NULL, 0);
+  if (status == STATUS_PENDING)
+   {
+ if (WaitForSingleObject (evt, INFINITE) == WAIT_OBJECT_0)
+   status = io.Status;
+ else
+   api_fatal ("WFSO failed, %E");
+   }
+  if (NT_SUCCESS (status))
+   status = open_pipe (ph);
+  if (STATUS_PIPE_NO_INSTANCE_AVAILABLE (status))
+   {
+ /* Another writer has grabbed the pipe instance.  Adjust
+the timeout and keep waiting if there's time left. */
+ pwbuf->Timeout.QuadPart = orig_timeout
+   + get_clock (CLOCK_MONOTONIC)->n100secs () - stamp;
+ if (pwbuf->Timeout.QuadPart < 0)
+   retry = true;
+ else
+   status = STATUS_IO_TIMEOUT;
+   }
+}
+  while (retry);
+  NtClose (evt);
   return status;
 }
 
@@ -294,7 +351,6 @@ void
 fhandler_fifo::record_connection (fifo_client_handler& fc,
  fifo_client_connect_state s)
 {
-  SetEvent (write_ready);
   fc.state = s;
   maybe_eof (false);
   ResetEvent (writer_opening);
@@ -327,9 +383,6 @@ fhandler_fifo::listen_client_thread ()
   if (add_client_handler () < 0)
api_fatal ("Can't add a client handler, %E");
 
-  /* Allow a writer to open. */
-  SetEvent (read_ready);
-
   /* Listen for a writer to connect to the new client handler. */
   fifo_client_handler& fc = fc_handler[nhandlers - 1];
   NTSTATUS status;
@@ -405,19 +458,13 @@ fhandler_fifo::listen_client_thread ()
 out:
   if (conn_evt)
 NtClose (conn_evt);
-  ResetEvent (read_ready);
   return 0;
 }
 
 int
 fhandler_fifo::open (int flags, mode_t)
 {
-  enum
-  {
-   success,
-   error_errno_set,
-   error_set_errno
-  } res;
+  int saved_errno = 0;
 
   if (flags & O_PATH)
 return open_fs (flags);
@@ -437,8 +484,7 @@ fhandler_fifo::open (int flags, mode_t)
   break;
 default:
   set_errno (EINVAL);
-  res = error_errno_set;
-  goto out;
+  goto err;
 }
 
   debug_only_printf ("reader %d, writer %d, duplexer %d", reader, writer, 
duplexer);
@@ -454,135 +500,151 @@ fhandler_fifo::open (int flags, mode_t)
 
   char npbuf[MAX_PATH];
   __small_sprintf (npbuf, "r-event.%08x.%016X",

[PATCH 04/21] Cygwin: FIFO: simplify the listen_client_thread code

2020-05-07 Thread Ken Brown via Cygwin-patches

Always return 0; no one is doing anything with the return value
anyway.

Remove the return value from stop_listen_client.

Make the connection event auto-reset, so that we don't have to reset
it later.

Simplify the process of connecting a bogus client when thread
termination is signaled.

Make some failures fatal.

Remove the unnecessary extra check for thread termination near the end
of listen_client_thread.
---
 winsup/cygwin/fhandler.h   |   4 +-
 winsup/cygwin/fhandler_fifo.cc | 117 +
 2 files changed, 47 insertions(+), 74 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index c1f47025a..c8f7a39a2 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1319,7 +1319,7 @@ class fhandler_fifo: public fhandler_base
   int add_client_handler ();
   void delete_client_handler (int);
   bool listen_client ();
-  int stop_listen_client ();
+  void stop_listen_client ();
   int check_listen_client_thread ();
   void record_connection (fifo_client_handler&,
  fifo_client_connect_state = fc_connected);
@@ -1345,7 +1345,7 @@ public:
   ssize_t __reg3 raw_write (const void *ptr, size_t ulen);
   bool arm (HANDLE h);
   bool need_fixup_before () const { return reader; }
-  int fixup_before_fork_exec (DWORD) { return stop_listen_client (); }
+  int fixup_before_fork_exec (DWORD) { stop_listen_client (); return 0; }
   void init_fixup_before ();
   void fixup_after_fork (HANDLE);
   void fixup_after_exec ();
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index ba3dbb124..fb20e5a7e 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -324,11 +324,10 @@ fhandler_fifo::record_connection (fifo_client_handler& fc,
 DWORD
 fhandler_fifo::listen_client_thread ()
 {
-  DWORD ret = -1;
-  HANDLE evt;
+  HANDLE conn_evt;
 
-  if (!(evt = create_event ()))
-goto out;
+  if (!(conn_evt = CreateEvent (NULL, false, false, NULL)))
+api_fatal ("Can't create connection event, %E");
 
   while (1)
 {
@@ -346,7 +345,7 @@ fhandler_fifo::listen_client_thread ()
 
   /* Create a new client handler. */
   if (add_client_handler () < 0)
-   goto out;
+   api_fatal ("Can't add a client handler, %E");
 
   /* Allow a writer to open. */
   if (!arm (read_ready))
@@ -359,12 +358,13 @@ fhandler_fifo::listen_client_thread ()
   fifo_client_handler& fc = fc_handler[nhandlers - 1];
   NTSTATUS status;
   IO_STATUS_BLOCK io;
+  bool cancel = false;
 
-  status = NtFsControlFile (fc.h, evt, NULL, NULL, ,
+  status = NtFsControlFile (fc.h, conn_evt, NULL, NULL, ,
FSCTL_PIPE_LISTEN, NULL, 0, NULL, 0);
   if (status == STATUS_PENDING)
{
- HANDLE w[2] = { evt, lct_termination_evt };
+ HANDLE w[2] = { conn_evt, lct_termination_evt };
  DWORD waitret = WaitForMultipleObjects (2, w, false, INFINITE);
  switch (waitret)
{
@@ -372,83 +372,65 @@ fhandler_fifo::listen_client_thread ()
  status = io.Status;
  break;
case WAIT_OBJECT_0 + 1:
- ret = 0;
  status = STATUS_THREAD_IS_TERMINATING;
+ cancel = true;
  break;
default:
- __seterrno ();
- debug_printf ("WaitForMultipleObjects failed, %E");
- status = STATUS_THREAD_IS_TERMINATING;
- break;
+ api_fatal ("WFMO failed, %E");
}
}
   HANDLE ph = NULL;
-  int ps = -1;
+  NTSTATUS status1;
+
   fifo_client_lock ();
   switch (status)
{
case STATUS_SUCCESS:
case STATUS_PIPE_CONNECTED:
  record_connection (fc);
- ResetEvent (evt);
  break;
case STATUS_PIPE_CLOSING:
  record_connection (fc, fc_closing);
- ResetEvent (evt);
  break;
case STATUS_THREAD_IS_TERMINATING:
- /* Force NtFsControlFile to complete.  Otherwise the next
-writer to connect might not be recorded in the client
-handler list. */
- status = open_pipe (ph);
- if (NT_SUCCESS (status)
- && (NT_SUCCESS (io.Status) || io.Status == STATUS_PIPE_CONNECTED))
-   {
- debug_printf ("successfully connected bogus client");
- delete_client_handler (nhandlers - 1);
-   }
- else if ((ps = fc.pipe_state ()) == FILE_PIPE_CONNECTED_STATE
-  || ps == FILE_PIPE_INPUT_AVAILABLE_STATE)
-   {
- /* A connection was made under our nose. */
- debug_printf ("recording connection before terminating");
- record_connection (fc);
-   }
+ /* Try to connect a bogus client.  Otherwise fc is still
+listening, and the next connection might not get recorded. */
+ status1 = open_pipe (ph);
+

[PATCH 01/21] Cygwin: FIFO: minor change - use NtClose

2020-05-07 Thread Ken Brown via Cygwin-patches

Replace CloseHandle by NtClose since all handles are created by NT
functions.
---
 winsup/cygwin/fhandler_fifo.cc | 32 
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index 19cd0e507..c091b0add 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -319,7 +319,7 @@ fhandler_fifo::listen_client ()
   __seterrno ();
   HANDLE evt = InterlockedExchangePointer (_termination_evt, NULL);
   if (evt)
-   CloseHandle (evt);
+   NtClose (evt);
   return false;
 }
   return true;
@@ -441,7 +441,7 @@ fhandler_fifo::listen_client_thread ()
  ret = -1;
}
  if (ph)
-   CloseHandle (ph);
+   NtClose (ph);
  fifo_client_unlock ();
  goto out;
default:
@@ -462,7 +462,7 @@ fhandler_fifo::listen_client_thread ()
 }
 out:
   if (evt)
-CloseHandle (evt);
+NtClose (evt);
   ResetEvent (read_ready);
   if (ret < 0)
 debug_printf ("exiting with error, %E");
@@ -617,16 +617,16 @@ out:
 {
   if (read_ready)
{
- CloseHandle (read_ready);
+ NtClose (read_ready);
  read_ready = NULL;
}
   if (write_ready)
{
- CloseHandle (write_ready);
+ NtClose (write_ready);
  write_ready = NULL;
}
   if (get_handle ())
-   CloseHandle (get_handle ());
+   NtClose (get_handle ());
   if (listen_client_thr)
stop_listen_client ();
 }
@@ -775,7 +775,7 @@ fhandler_fifo::raw_write (const void *ptr, size_t len)
ret = nbytes;
 }
   if (evt)
-CloseHandle (evt);
+NtClose (evt);
   if (status == STATUS_THREAD_SIGNALED && ret < 0)
 set_errno (EINTR);
   else if (status == STATUS_THREAD_CANCELED)
@@ -819,7 +819,7 @@ fhandler_fifo::check_listen_client_thread ()
   switch (waitret)
{
case WAIT_OBJECT_0:
- CloseHandle (listen_client_thr);
+ NtClose (listen_client_thr);
  break;
case WAIT_TIMEOUT:
  ret = 1;
@@ -828,7 +828,7 @@ fhandler_fifo::check_listen_client_thread ()
  debug_printf ("WaitForSingleObject failed, %E");
  ret = -1;
  __seterrno ();
- CloseHandle (listen_client_thr);
+ NtClose (listen_client_thr);
  break;
}
 }
@@ -1001,11 +1001,11 @@ fhandler_fifo::stop_listen_client ()
  ret = -1;
  debug_printf ("listen_client_thread exited with error");
}
-  CloseHandle (thr);
+  NtClose (thr);
 }
   evt = InterlockedExchangePointer (_termination_evt, NULL);
   if (evt)
-CloseHandle (evt);
+NtClose (evt);
   return ret;
 }
 
@@ -1017,9 +1017,9 @@ fhandler_fifo::close ()
   fifo_client_unlock ();
   int ret = stop_listen_client ();
   if (read_ready)
-CloseHandle (read_ready);
+NtClose (read_ready);
   if (write_ready)
-CloseHandle (write_ready);
+NtClose (write_ready);
   fifo_client_lock ();
   for (int i = 0; i < nhandlers; i++)
 if (fc_handler[i].close () < 0)
@@ -1070,7 +1070,7 @@ fhandler_fifo::dup (fhandler_base *child, int flags)
GetCurrentProcess (), >write_ready,
0, true, DUPLICATE_SAME_ACCESS))
 {
-  CloseHandle (fhf->read_ready);
+  NtClose (fhf->read_ready);
   fhf->close ();
   __seterrno ();
   goto out;
@@ -1084,8 +1084,8 @@ fhandler_fifo::dup (fhandler_base *child, int flags)
0, true, DUPLICATE_SAME_ACCESS))
{
  fifo_client_unlock ();
- CloseHandle (fhf->read_ready);
- CloseHandle (fhf->write_ready);
+ NtClose (fhf->read_ready);
+ NtClose (fhf->write_ready);
  fhf->close ();
  __seterrno ();
  goto out;
-- 
2.21.0

[PATCH 20/21] Cygwin: FIFO: support opening multiple readers

2020-05-07 Thread Ken Brown via Cygwin-patches

Although we can have multiple readers open because of dup/fork/exec,
the current code does not support multiple readers opening a FIFO by
explicitly calling 'open'.

The main complication in supporting this is that when a blocking
reader tries to open and there's already one open, it has to check
whether there any writers open.  It can't rely on the write_ready
event, whose state hasn't changed since the first writer opened.

To fix this, add two new named events, check_write_ready_evt and
write_ready_ok_evt, and a new method, check_write_ready().

The first event signals the owner's reader thread to call
check_write_ready(), which polls the fc_handler list to check for
connected writers.  If it finds none, it checks to see if there's a
writer in the process and then sets/resets write_ready appropriately.

When check_write_ready() finishes it sets write_ready_ok_evt to signal
the reader that write_ready has been updated.

The polling is done via fifo_client_handler::pipe_state().  As long as
it's calling that function anyway, check_write_ready() updates the
state of each handler.

Also add a new lock to prevent a race if two readers are trying to
open simultaneously.
---
 winsup/cygwin/fhandler.h   |   9 ++-
 winsup/cygwin/fhandler_fifo.cc | 129 ++---
 2 files changed, 127 insertions(+), 11 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 31c65866e..8a23d6753 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1324,7 +1324,7 @@ class fifo_shmem_t
 {
   LONG _nreaders;
   fifo_reader_id_t _owner, _prev_owner, _pending_owner;
-  af_unix_spinlock_t _owner_lock, _reading_lock;
+  af_unix_spinlock_t _owner_lock, _reading_lock, _reader_opening_lock;
 
   /* Info about shared memory block used for temporary storage of the
  owner's fc_handler list. */
@@ -1346,6 +1346,8 @@ public:
   void owner_unlock () { _owner_lock.unlock (); }
   void reading_lock () { _reading_lock.lock (); }
   void reading_unlock () { _reading_lock.unlock (); }
+  void reader_opening_lock () { _reader_opening_lock.lock (); }
+  void reader_opening_unlock () { _reader_opening_lock.unlock (); }
 
   int get_shared_nhandlers () const { return (int) _sh_nhandlers; }
   void set_shared_nhandlers (int n) { InterlockedExchange (&_sh_nhandlers, n); 
}
@@ -1371,6 +1373,8 @@ class fhandler_fifo: public fhandler_base
   HANDLE owner_needed_evt;  /* The owner is closing. */
   HANDLE owner_found_evt;   /* A new owner has taken over. */
   HANDLE update_needed_evt; /* shared_fc_handler needs updating. */
+  HANDLE check_write_ready_evt; /* write_ready needs to be checked. */
+  HANDLE write_ready_ok_evt;/* check_write_ready is done. */
 
   /* Handles to non-shared events needed for fifo_reader_threads. */
   HANDLE cancel_evt;/* Signal thread to terminate. */
@@ -1448,6 +1452,9 @@ class fhandler_fifo: public fhandler_base
   { return shmem->shared_fc_handler_updated (); }
   void shared_fc_handler_updated (bool val)
   { shmem->shared_fc_handler_updated (val); }
+  void check_write_ready ();
+  void reader_opening_lock () { shmem->reader_opening_lock (); }
+  void reader_opening_unlock () { shmem->reader_opening_unlock (); }
 
 public:
   fhandler_fifo ();
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index 81473015e..cc51c449a 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -75,6 +75,7 @@ fhandler_fifo::fhandler_fifo ():
   fhandler_base (),
   read_ready (NULL), write_ready (NULL), writer_opening (NULL),
   owner_needed_evt (NULL), owner_found_evt (NULL), update_needed_evt (NULL),
+  check_write_ready_evt (NULL), write_ready_ok_evt (NULL),
   cancel_evt (NULL), thr_sync_evt (NULL), _maybe_eof (false),
   fc_handler (NULL), shandlers (0), nhandlers (0),
   reader (false), writer (false), duplexer (false),
@@ -441,6 +442,45 @@ fhandler_fifo::update_shared_handlers ()
   return 0;
 }
 
+/* The write_ready event gets set when a writer opens, to indicate
+   that a blocking reader can open.  If a second reader wants to open,
+   we need to see if there are still any writers open. */
+void
+fhandler_fifo::check_write_ready ()
+{
+  bool set = false;
+
+  fifo_client_lock ();
+  for (int i = 0; i < nhandlers && !set; i++)
+switch (fc_handler[i].pipe_state ())
+  {
+  case FILE_PIPE_CONNECTED_STATE:
+   fc_handler[i].state = fc_connected;
+   set = true;
+   break;
+  case FILE_PIPE_INPUT_AVAILABLE_STATE:
+   fc_handler[i].state = fc_input_avail;
+   set = true;
+   break;
+  case FILE_PIPE_DISCONNECTED_STATE:
+   fc_handler[i].state = fc_disconnected;
+   break;
+  case FILE_PIPE_LISTENING_STATE:
+   fc_handler[i].state = fc_listening;
+  case FILE_PIPE_CLOSING_STATE:
+   fc_handler[i].state = fc_closing;
+  default:
+   fc_handler[i].state = fc_error;
+   break;
+  }
+

[PATCH 08/21] Cygwin: FIFO: fix hit_eof

2020-05-07 Thread Ken Brown via Cygwin-patches

According to Posix, a FIFO open for reading is at EOF if it is empty
and there are no writers open.

The only way to test this is to poll the fifo_client_handlers as in
raw_read and select.cc:peek_fifo.  The current hit_eof instead relies
on the value of nconnected, which can be out of date.  On the one
hand, it doesn't take into account writers that were connected but
have since closed.  On the other hand, it doesn't take into account
writers that are in the process of opening but haven't yet connected.

Fix this by introducing a maybe_eof method that tentatively assumes
EOF if there are no connected writers after polling.  Then check for
writers currently opening (via a new 'writer_opening' event), and wait
for the fifo_reader_thread to record any new connection that was made
while we were polling.

To handle the needs of peek_fifo, replace the get_fc_handle method
by a get_fc_handler method, and add a fifo_client_handler::get_state
method.

Remove the is_connected method, which was used only in peek_fifo and
is no longer needed.

Remove the nconnected data member, which was used only for the flawed
hit_eof.

Add some comments about events to fhandler.h.
---
 winsup/cygwin/fhandler.h   | 19 +---
 winsup/cygwin/fhandler_fifo.cc | 84 ++
 winsup/cygwin/select.cc| 44 --
 3 files changed, 98 insertions(+), 49 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 4d691a0fc..3bc04cf13 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1296,19 +1296,26 @@ struct fifo_client_handler
 /* Returns FILE_PIPE_DISCONNECTED_STATE, FILE_PIPE_LISTENING_STATE,
FILE_PIPE_CONNECTED_STATE, FILE_PIPE_CLOSING_STATE,
FILE_PIPE_INPUT_AVAILABLE_STATE, or -1 on error. */
+  fifo_client_connect_state _state () { return state; }
   int pipe_state ();
 };
 
 class fhandler_fifo: public fhandler_base
 {
-  HANDLE read_ready;
-  HANDLE write_ready;
+  /* Handles to named events shared by all fhandlers for a given FIFO. */
+  HANDLE read_ready;/* A reader is open; OK for a writer to open. 
*/
+  HANDLE write_ready;   /* A writer is open; OK for a reader to open. 
*/
+  HANDLE writer_opening;/* A writer is opening; no EOF. */
+
+  /* Non-shared handles needed for the listen_client_thread. */
   HANDLE listen_client_thr;
   HANDLE lct_termination_evt;
+
   UNICODE_STRING pipe_name;
   WCHAR pipe_name_buf[CYGWIN_FIFO_PIPE_NAME_LEN + 1];
+  bool _maybe_eof;
   fifo_client_handler fc_handler[MAX_CLIENTS];
-  int nhandlers, nconnected;
+  int nhandlers;
   af_unix_spinlock_t _fifo_client_lock;
   bool reader, writer, duplexer;
   size_t max_atomic_write;
@@ -1326,10 +1333,10 @@ class fhandler_fifo: public fhandler_base
 public:
   fhandler_fifo ();
   bool hit_eof ();
+  bool maybe_eof () const { return _maybe_eof; }
+  void maybe_eof (bool val) { _maybe_eof = val; }
   int get_nhandlers () const { return nhandlers; }
-  HANDLE get_fc_handle (int i) const { return fc_handler[i].h; }
-  bool is_connected (int i) const
-  { return fc_handler[i].state == fc_connected; }
+  fifo_client_handler get_fc_handler (int i) const { return fc_handler[i]; }
   PUNICODE_STRING get_pipe_name ();
   DWORD listen_client_thread ();
   void fifo_client_lock () { _fifo_client_lock.lock (); }
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index 4904a535d..21faf4ec2 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -66,9 +66,10 @@ STATUS_PIPE_EMPTY simply means there's no data to be read. */
   || _s == STATUS_PIPE_BUSY; })
 
 fhandler_fifo::fhandler_fifo ():
-  fhandler_base (), read_ready (NULL), write_ready (NULL),
-  listen_client_thr (NULL), lct_termination_evt (NULL), nhandlers (0),
-  nconnected (0), reader (false), writer (false), duplexer (false),
+  fhandler_base (),
+  read_ready (NULL), write_ready (NULL), writer_opening (NULL),
+  listen_client_thr (NULL), lct_termination_evt (NULL), _maybe_eof (false), 
nhandlers (0),
+  reader (false), writer (false), duplexer (false),
   max_atomic_write (DEFAULT_PIPEBUFSIZE)
 {
   pipe_name_buf[0] = L'\0';
@@ -295,7 +296,8 @@ fhandler_fifo::record_connection (fifo_client_handler& fc,
 {
   SetEvent (write_ready);
   fc.state = s;
-  nconnected++;
+  maybe_eof (false);
+  ResetEvent (writer_opening);
   set_pipe_non_blocking (fc.h, true);
 }
 
@@ -465,6 +467,13 @@ fhandler_fifo::open (int flags, mode_t)
   res = error_set_errno;
   goto out;
 }
+  npbuf[0] = 'o';
+  if (!(writer_opening = CreateEvent (sa_buf, true, false, npbuf)))
+{
+  debug_printf ("CreateEvent for %s failed, %E", npbuf);
+  res = error_set_errno;
+  goto out;
+}
 
   /* If we're a duplexer, create the pipe and the first client handler. */
   if (duplexer)
@@ -518,10 +527,12 @@ fhandler_fifo::open (int flags, mode_t)
  listen_client thread is running.  Then signal write_ready.  */

[PATCH 17/21] Cygwin: FIFO: take ownership on exec

2020-05-07 Thread Ken Brown via Cygwin-patches

If fixup_after_exec is called on a non-close-on-exec reader whose
parent is the owner, transfer ownership to the child.  Otherwise the
parent's pipe handles will be closed before any other reader can
duplicate them.

To help with this, make the cancel_evt and thr_sync_evt handles
inheritable, so that the child can terminate the parent's
fifo_reader_thread (and the parent will update the shared fc_handler
list).

Add an optional argument 'from_exec' to update_my_handlers to simplify
its use in this case; no handle duplication is required.
---
 winsup/cygwin/fhandler.h   |   2 +-
 winsup/cygwin/fhandler_fifo.cc | 151 +++--
 2 files changed, 107 insertions(+), 46 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 34b209f5d..1cd7d2b11 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1414,7 +1414,7 @@ class fhandler_fifo: public fhandler_base
   { return shmem->get_shared_fc_handler_committed (); }
   void set_shared_fc_handler_committed (size_t n)
   { shmem->set_shared_fc_handler_committed (n); }
-  int update_my_handlers ();
+  int update_my_handlers (bool from_exec = false);
   int update_shared_handlers ();
 
 public:
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index 846115ad4..1c59bb3f4 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -104,13 +104,14 @@ sec_user_cloexec (bool cloexec, PSECURITY_ATTRIBUTES sa, 
PSID sid)
 }
 
 static HANDLE
-create_event ()
+create_event (bool inherit = false)
 {
   NTSTATUS status;
   OBJECT_ATTRIBUTES attr;
   HANDLE evt = NULL;
 
-  InitializeObjectAttributes (, NULL, 0, NULL, NULL);
+  InitializeObjectAttributes (, NULL, inherit ? OBJ_INHERIT : 0,
+ NULL, NULL);
   status = NtCreateEvent (, EVENT_ALL_ACCESS, ,
  NotificationEvent, FALSE);
   if (!NT_SUCCESS (status))
@@ -353,47 +354,72 @@ fhandler_fifo::record_connection (fifo_client_handler& fc,
   set_pipe_non_blocking (fc.h, true);
 }
 
-/* Called from fifo_reader_thread_func with owner_lock in place. */
+/* Called from fifo_reader_thread_func with owner_lock in place, also
+   from fixup_after_exec with shared handles useable as they are. */
 int
-fhandler_fifo::update_my_handlers ()
+fhandler_fifo::update_my_handlers (bool from_exec)
 {
-  close_all_handlers ();
-  fifo_reader_id_t prev = get_prev_owner ();
-  if (!prev)
+  if (from_exec)
 {
-  debug_printf ("No previous owner to copy handles from");
-  return 0;
+  nhandlers = get_shared_nhandlers ();
+  if (nhandlers > shandlers)
+   {
+ int save = shandlers;
+ shandlers = nhandlers + 64;
+ void *temp = realloc (fc_handler,
+   shandlers * sizeof (fc_handler[0]));
+ if (!temp)
+   {
+ shandlers = save;
+ nhandlers = 0;
+ set_errno (ENOMEM);
+ return -1;
+   }
+ fc_handler = (fifo_client_handler *) temp;
+   }
+  memcpy (fc_handler, shared_fc_handler,
+ nhandlers * sizeof (fc_handler[0]));
 }
-  HANDLE prev_proc;
-  if (prev.winpid == me.winpid)
-prev_proc =  GetCurrentProcess ();
   else
-prev_proc = OpenProcess (PROCESS_DUP_HANDLE, false, prev.winpid);
-  if (!prev_proc)
 {
-  debug_printf ("Can't open process of previous owner, %E");
-  __seterrno ();
-  return -1;
-}
-
-  for (int i = 0; i < get_shared_nhandlers (); i++)
-{
-  /* Should never happen. */
-  if (shared_fc_handler[i].state != fc_connected)
-   continue;
-  if (add_client_handler (false) < 0)
-   api_fatal ("Can't add client handler, %E");
-  fifo_client_handler  = fc_handler[nhandlers - 1];
-  if (!DuplicateHandle (prev_proc, shared_fc_handler[i].h,
-   GetCurrentProcess (), , 0,
-   !close_on_exec (), DUPLICATE_SAME_ACCESS))
+  close_all_handlers ();
+  fifo_reader_id_t prev = get_prev_owner ();
+  if (!prev)
+   {
+ debug_printf ("No previous owner to copy handles from");
+ return 0;
+   }
+  HANDLE prev_proc;
+  if (prev.winpid == me.winpid)
+   prev_proc =  GetCurrentProcess ();
+  else
+   prev_proc = OpenProcess (PROCESS_DUP_HANDLE, false, prev.winpid);
+  if (!prev_proc)
{
- debug_printf ("Can't duplicate handle of previous owner, %E");
- --nhandlers;
+ debug_printf ("Can't open process of previous owner, %E");
  __seterrno ();
  return -1;
}
-  fc.state = fc_connected;
+
+  for (int i = 0; i < get_shared_nhandlers (); i++)
+   {
+ /* Should never happen. */
+ if (shared_fc_handler[i].state != fc_connected)
+   continue;
+ if (add_client_handler (false) < 0)
+   api_fatal ("Can't add client handler, %E");
+ fifo_client_handler

[PATCH 18/21] Cygwin: FIFO: find a new owner when closing

2020-05-07 Thread Ken Brown via Cygwin-patches

If the owning reader is closing, wait for another reader (if there is
one) to take ownership before closing the owner's pipe handles.

To synchronize the ownership transfer, add events owner_needed_evt and
owner_found_evt, and add methods owner_needed and owner_found to
set/reset them.

Modify the fifo_reader_thread function to wake up all non-owners when
a new owner is needed.

Make a cosmetic change in close so that fhandler_base::close is called
only if we have a write handle.  This prevents strace output from
being littered with statements that the null handle is being closed.
---
 winsup/cygwin/fhandler.h   |  14 +
 winsup/cygwin/fhandler_fifo.cc | 109 +
 2 files changed, 112 insertions(+), 11 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 1cd7d2b11..f8c1b52a4 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1359,6 +1359,10 @@ class fhandler_fifo: public fhandler_base
   HANDLE write_ready;   /* A writer is open; OK for a reader to open. 
*/
   HANDLE writer_opening;/* A writer is opening; no EOF. */
 
+  /* Handles to named events needed by all readers of a given FIFO. */
+  HANDLE owner_needed_evt;  /* The owner is closing. */
+  HANDLE owner_found_evt;   /* A new owner has taken over. */
+
   /* Handles to non-shared events needed for fifo_reader_threads. */
   HANDLE cancel_evt;/* Signal thread to terminate. */
   HANDLE thr_sync_evt;  /* The thread has terminated. */
@@ -1405,6 +1409,16 @@ class fhandler_fifo: public fhandler_base
   fifo_reader_id_t get_prev_owner () const { return shmem->get_prev_owner (); }
   void set_prev_owner (fifo_reader_id_t fr_id)
   { shmem->set_prev_owner (fr_id); }
+  void owner_needed ()
+  {
+ResetEvent (owner_found_evt);
+SetEvent (owner_needed_evt);
+  }
+  void owner_found ()
+  {
+ResetEvent (owner_needed_evt);
+SetEvent (owner_found_evt);
+  }
 
   int get_shared_nhandlers () { return shmem->get_shared_nhandlers (); }
   void set_shared_nhandlers (int n) { shmem->set_shared_nhandlers (n); }
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index 1c59bb3f4..bf33a52d6 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -74,6 +74,7 @@ static NO_COPY fifo_reader_id_t null_fr_id = { .winpid = 0, 
.fh = NULL };
 fhandler_fifo::fhandler_fifo ():
   fhandler_base (),
   read_ready (NULL), write_ready (NULL), writer_opening (NULL),
+  owner_needed_evt (NULL), owner_found_evt (NULL),
   cancel_evt (NULL), thr_sync_evt (NULL), _maybe_eof (false),
   fc_handler (NULL), shandlers (0), nhandlers (0),
   reader (false), writer (false), duplexer (false),
@@ -464,14 +465,23 @@ fhandler_fifo::fifo_reader_thread_func ()
  set_owner (me);
  if (update_my_handlers () < 0)
api_fatal ("Can't update my handlers, %E");
+ owner_found ();
  owner_unlock ();
  continue;
}
   else if (cur_owner != me)
{
  owner_unlock ();
- WaitForSingleObject (cancel_evt, INFINITE);
- goto canceled;
+ HANDLE w[2] = { owner_needed_evt, cancel_evt };
+ switch (WaitForMultipleObjects (2, w, false, INFINITE))
+   {
+   case WAIT_OBJECT_0:
+ continue;
+   case WAIT_OBJECT_0 + 1:
+ goto canceled;
+   default:
+ api_fatal ("WFMO failed, %E");
+   }
}
   else
{
@@ -797,8 +807,23 @@ fhandler_fifo::open (int flags, mode_t)
   if (create_shared_fc_handler () < 0)
goto err_close_shmem;
   inc_nreaders ();
+  npbuf[0] = 'n';
+  if (!(owner_needed_evt = CreateEvent (sa_buf, true, false, npbuf)))
+   {
+ debug_printf ("CreateEvent for %s failed, %E", npbuf);
+ __seterrno ();
+ goto err_dec_nreaders;
+   }
+  npbuf[0] = 'f';
+  if (!(owner_found_evt = CreateEvent (sa_buf, true, false, npbuf)))
+   {
+ debug_printf ("CreateEvent for %s failed, %E", npbuf);
+ __seterrno ();
+ goto err_close_owner_needed_evt;
+   }
+  /* Make cancel and sync inheritable for exec. */
   if (!(cancel_evt = create_event (true)))
-   goto err_dec_nreaders;
+   goto err_close_owner_found_evt;
   if (!(thr_sync_evt = create_event (true)))
goto err_close_cancel_evt;
   me.winpid = GetCurrentProcessId ();
@@ -918,6 +943,10 @@ err_close_reader:
   return 0;
 err_close_cancel_evt:
   NtClose (cancel_evt);
+err_close_owner_found_evt:
+  NtClose (owner_found_evt);
+err_close_owner_needed_evt:
+  NtClose (owner_needed_evt);
 err_dec_nreaders:
   if (dec_nreaders () == 0)
 ResetEvent (read_ready);
@@ -1255,13 +1284,49 @@ fhandler_fifo::close ()
 {
   if (reader)
 {
-  if (dec_nreaders () == 0)
-   ResetEvent (read_ready);
+  /* If we're the owner, try to find a new owner. */
+  bool

[PATCH 10/21] Cygwin: FIFO: use a cygthread instead of a homemade thread

2020-05-07 Thread Ken Brown via Cygwin-patches

This will simplify future work.

Rename the thread from "listen_client_thread" to "fifo_reader_thread"
because it will be used for more than just listening.

Remove the fixup_before stuff, which won't be needed after future
changes to fixup_after_fork and fixup_after_exec.
---
 winsup/cygwin/fhandler.h   |  17 ++--
 winsup/cygwin/fhandler_fifo.cc | 173 +++--
 2 files changed, 65 insertions(+), 125 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 2516c93b4..5e6a1d1db 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1307,9 +1307,9 @@ class fhandler_fifo: public fhandler_base
   HANDLE write_ready;   /* A writer is open; OK for a reader to open. 
*/
   HANDLE writer_opening;/* A writer is opening; no EOF. */
 
-  /* Non-shared handles needed for the listen_client_thread. */
-  HANDLE listen_client_thr;
-  HANDLE lct_termination_evt;
+  /* Handles to non-shared events needed for fifo_reader_threads. */
+  HANDLE cancel_evt;/* Signal thread to terminate. */
+  HANDLE thr_sync_evt;  /* The thread has terminated. */
 
   UNICODE_STRING pipe_name;
   WCHAR pipe_name_buf[CYGWIN_FIFO_PIPE_NAME_LEN + 1];
@@ -1326,11 +1326,10 @@ class fhandler_fifo: public fhandler_base
   NTSTATUS wait_open_pipe (HANDLE&);
   int add_client_handler ();
   void delete_client_handler (int);
-  bool listen_client ();
-  void stop_listen_client ();
-  int check_listen_client_thread ();
+  void cancel_reader_thread ();
   void record_connection (fifo_client_handler&,
  fifo_client_connect_state = fc_connected);
+
 public:
   fhandler_fifo ();
   bool hit_eof ();
@@ -1339,7 +1338,7 @@ public:
   int get_nhandlers () const { return nhandlers; }
   fifo_client_handler get_fc_handler (int i) const { return fc_handler[i]; }
   PUNICODE_STRING get_pipe_name ();
-  DWORD listen_client_thread ();
+  DWORD fifo_reader_thread_func ();
   void fifo_client_lock () { _fifo_client_lock.lock (); }
   void fifo_client_unlock () { _fifo_client_lock.unlock (); }
   int open (int, mode_t);
@@ -1351,9 +1350,6 @@ public:
   void set_close_on_exec (bool val);
   void __reg3 raw_read (void *ptr, size_t& ulen);
   ssize_t __reg3 raw_write (const void *ptr, size_t ulen);
-  bool need_fixup_before () const { return reader; }
-  int fixup_before_fork_exec (DWORD) { stop_listen_client (); return 0; }
-  void init_fixup_before ();
   void fixup_after_fork (HANDLE);
   void fixup_after_exec ();
   int __reg2 fstatvfs (struct statvfs *buf);
@@ -1375,7 +1371,6 @@ public:
 void *ptr = (void *) ccalloc (malloc_type, 1, sizeof (fhandler_fifo));
 fhandler_fifo *fhf = new (ptr) fhandler_fifo (ptr);
 /* We don't want our client list to change any more. */
-stop_listen_client ();
 copyto (fhf);
 /* fhf->pipe_name_buf is a *copy* of this->pipe_name_buf, but
fhf->pipe_name.Buffer == this->pipe_name_buf. */
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index 5c3df5497..09a7eb321 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -32,11 +32,11 @@
  When a FIFO is opened for reading,
  fhandler_fifo::create_pipe_instance is called to create the first
  instance of a Windows named pipe server (Windows terminology).  A
- "listen_client" thread is also started; it waits for pipe clients
+ "fifo_reader" thread is also started; it waits for pipe clients
  (Windows terminology again) to connect.  This happens every time
  a process opens the FIFO for writing.
 
- The listen_client thread creates new instances of the pipe server
+ The fifo_reader thread creates new instances of the pipe server
  as needed, so that there is always an instance available for a
  writer to connect to.
 
@@ -68,7 +68,7 @@ STATUS_PIPE_EMPTY simply means there's no data to be read. */
 fhandler_fifo::fhandler_fifo ():
   fhandler_base (),
   read_ready (NULL), write_ready (NULL), writer_opening (NULL),
-  listen_client_thr (NULL), lct_termination_evt (NULL), _maybe_eof (false), 
nhandlers (0),
+  cancel_evt (NULL), thr_sync_evt (NULL), _maybe_eof (false), nhandlers (0),
   reader (false), writer (false), duplexer (false),
   max_atomic_write (DEFAULT_PIPEBUFSIZE)
 {
@@ -319,34 +319,6 @@ fhandler_fifo::delete_client_handler (int i)
 (nhandlers - i) * sizeof (fc_handler[i]));
 }
 
-/* Just hop to the listen_client_thread method. */
-DWORD WINAPI
-listen_client_func (LPVOID param)
-{
-  fhandler_fifo *fh = (fhandler_fifo *) param;
-  return fh->listen_client_thread ();
-}
-
-/* Start a thread that listens for client connections. */
-bool
-fhandler_fifo::listen_client ()
-{
-  if (!(lct_termination_evt = create_event ()))
-return false;
-
-  listen_client_thr = CreateThread (NULL, PREFERRED_IO_BLKSIZE,
-   listen_client_func, (PVOID) this, 0, NULL);
-  if (!listen_client_thr)
-

[PATCH 06/21] Cygwin: FIFO: honor the flags argument in dup

2020-05-07 Thread Ken Brown via Cygwin-patches

Also improve the error handling.
---
 winsup/cygwin/fhandler_fifo.cc | 60 +++---
 1 file changed, 33 insertions(+), 27 deletions(-)

diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index 44919c19e..f61e2fe72 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -955,56 +955,62 @@ fhandler_fifo::fcntl (int cmd, intptr_t arg)
 int
 fhandler_fifo::dup (fhandler_base *child, int flags)
 {
-  int ret = -1;
+  int i = 0;
   fhandler_fifo *fhf = NULL;
 
   if (get_flags () & O_PATH)
 return fhandler_base::dup (child, flags);
 
   if (fhandler_base::dup (child, flags))
-goto out;
+goto err;
 
   fhf = (fhandler_fifo *) child;
   if (!DuplicateHandle (GetCurrentProcess (), read_ready,
GetCurrentProcess (), >read_ready,
-   0, true, DUPLICATE_SAME_ACCESS))
+   0, !(flags & O_CLOEXEC), DUPLICATE_SAME_ACCESS))
 {
-  fhf->close ();
   __seterrno ();
-  goto out;
+  goto err;
 }
   if (!DuplicateHandle (GetCurrentProcess (), write_ready,
GetCurrentProcess (), >write_ready,
-   0, true, DUPLICATE_SAME_ACCESS))
+   0, !(flags & O_CLOEXEC), DUPLICATE_SAME_ACCESS))
 {
-  NtClose (fhf->read_ready);
-  fhf->close ();
   __seterrno ();
-  goto out;
+  goto err_close_read_ready;
 }
-  fifo_client_lock ();
-  for (int i = 0; i < nhandlers; i++)
+  if (reader)
 {
-  if (!DuplicateHandle (GetCurrentProcess (), fc_handler[i].h,
-   GetCurrentProcess (),
-   >fc_handler[i].h,
-   0, true, DUPLICATE_SAME_ACCESS))
+  fifo_client_lock ();
+  for (i = 0; i < nhandlers; i++)
+   {
+ if (!DuplicateHandle (GetCurrentProcess (), fc_handler[i].h,
+   GetCurrentProcess (), >fc_handler[i].h,
+   0, !(flags & O_CLOEXEC), DUPLICATE_SAME_ACCESS))
+   {
+ __seterrno ();
+ break;
+   }
+   }
+  if (i < nhandlers)
{
  fifo_client_unlock ();
- NtClose (fhf->read_ready);
- NtClose (fhf->write_ready);
- fhf->close ();
- __seterrno ();
- goto out;
+ goto err_close_handlers;
}
+  fifo_client_unlock ();
+  if (!fhf->listen_client ())
+   goto err_close_handlers;
+  fhf->init_fixup_before ();
 }
-  fifo_client_unlock ();
-  if (!reader || fhf->listen_client ())
-ret = 0;
-  if (reader)
-fhf->init_fixup_before ();
-out:
-  return ret;
+  return 0;
+err_close_handlers:
+  for (int j = 0; j < i; j++)
+fhf->fc_handler[j].close ();
+  NtClose (fhf->write_ready);
+err_close_read_ready:
+  NtClose (fhf->read_ready);
+err:
+  return -1;
 }
 
 void
-- 
2.21.0

[PATCH 07/21] Cygwin: FIFO: dup/fork/exec: make sure child starts unlocked

2020-05-07 Thread Ken Brown via Cygwin-patches

There can be deadlocks if the child starts with its fifo_client_lock
in the locked state.
---
 winsup/cygwin/fhandler_fifo.cc | 31 +++
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index f61e2fe72..4904a535d 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -981,6 +981,9 @@ fhandler_fifo::dup (fhandler_base *child, int flags)
 }
   if (reader)
 {
+  /* Make sure the child starts unlocked. */
+  fhf->fifo_client_unlock ();
+
   fifo_client_lock ();
   for (i = 0; i < nhandlers; i++)
{
@@ -1025,20 +1028,32 @@ fhandler_fifo::fixup_after_fork (HANDLE parent)
   fhandler_base::fixup_after_fork (parent);
   fork_fixup (parent, read_ready, "read_ready");
   fork_fixup (parent, write_ready, "write_ready");
-  fifo_client_lock ();
-  for (int i = 0; i < nhandlers; i++)
-  fork_fixup (parent, fc_handler[i].h, "fc_handler[].h");
-  fifo_client_unlock ();
-  if (reader && !listen_client ())
-debug_printf ("failed to start lct, %E");
+  if (reader)
+{
+  /* Make sure the child starts unlocked. */
+  fifo_client_unlock ();
+
+  fifo_client_lock ();
+  for (int i = 0; i < nhandlers; i++)
+   fork_fixup (parent, fc_handler[i].h, "fc_handler[].h");
+  fifo_client_unlock ();
+  if (!listen_client ())
+   debug_printf ("failed to start lct, %E");
+}
 }
 
 void
 fhandler_fifo::fixup_after_exec ()
 {
   fhandler_base::fixup_after_exec ();
-  if (reader && !listen_client ())
-debug_printf ("failed to start lct, %E");
+  if (reader && !close_on_exec ())
+{
+  /* Make sure the child starts unlocked. */
+  fifo_client_unlock ();
+
+  if (!listen_client ())
+   debug_printf ("failed to start lct, %E");
+}
 }
 
 void
-- 
2.21.0

[PATCH 03/21] Cygwin: FIFO: change the fifo_client_connect_state enum

2020-05-07 Thread Ken Brown via Cygwin-patches

Make the values correspond to the possible return values of
fifo_client_handler::pipe_state().

When cleaning up the fc_handler list in listen_client_thread(), don't
delete handlers in the fc_closing state.  I think the pipe might still
have input to be read in that case.

Set the state to fc_closing later in the same function if a connection
is made and the status returned by NtFsControlFile is
STATUS_PIPE_CLOSING.

In raw_read, don't error out if NtReadFile returns an unexpected
status; just set the state of that handler to fc_error.  One writer in
a bad state doesn't justify giving up on reading.
---
 winsup/cygwin/fhandler.h   | 10 --
 winsup/cygwin/fhandler_fifo.cc | 29 ++---
 2 files changed, 22 insertions(+), 17 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index e841f96ac..c1f47025a 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1270,11 +1270,16 @@ public:
 #define CYGWIN_FIFO_PIPE_NAME_LEN 47
 #define MAX_CLIENTS 64
 
+/* The last three are the ones we try to read from. */
 enum fifo_client_connect_state
 {
   fc_unknown,
+  fc_error,
+  fc_disconnected,
+  fc_listening,
   fc_connected,
-  fc_invalid
+  fc_closing,
+  fc_input_avail,
 };
 
 enum
@@ -1316,7 +1321,8 @@ class fhandler_fifo: public fhandler_base
   bool listen_client ();
   int stop_listen_client ();
   int check_listen_client_thread ();
-  void record_connection (fifo_client_handler&);
+  void record_connection (fifo_client_handler&,
+ fifo_client_connect_state = fc_connected);
 public:
   fhandler_fifo ();
   bool hit_eof ();
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index 6b71dd950..ba3dbb124 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -267,6 +267,7 @@ fhandler_fifo::add_client_handler ()
 {
   ret = 0;
   fc.h = ph;
+  fc.state = fc_listening;
   fc_handler[nhandlers++] = fc;
 }
 out:
@@ -311,10 +312,11 @@ fhandler_fifo::listen_client ()
 }
 
 void
-fhandler_fifo::record_connection (fifo_client_handler& fc)
+fhandler_fifo::record_connection (fifo_client_handler& fc,
+ fifo_client_connect_state s)
 {
   SetEvent (write_ready);
-  fc.state = fc_connected;
+  fc.state = s;
   nconnected++;
   set_pipe_non_blocking (fc.h, true);
 }
@@ -330,15 +332,12 @@ fhandler_fifo::listen_client_thread ()
 
   while (1)
 {
-  /* At the beginning of the loop, all client handlers are
-in the fc_connected or fc_invalid state. */
-
-  /* Delete any invalid clients. */
+  /* Cleanup the fc_handler list. */
   fifo_client_lock ();
   int i = 0;
   while (i < nhandlers)
{
- if (fc_handler[i].state == fc_invalid)
+ if (fc_handler[i].state < fc_connected)
delete_client_handler (i);
  else
i++;
@@ -393,6 +392,10 @@ fhandler_fifo::listen_client_thread ()
  record_connection (fc);
  ResetEvent (evt);
  break;
+   case STATUS_PIPE_CLOSING:
+ record_connection (fc, fc_closing);
+ ResetEvent (evt);
+ break;
case STATUS_THREAD_IS_TERMINATING:
  /* Force NtFsControlFile to complete.  Otherwise the next
 writer to connect might not be recorded in the client
@@ -835,7 +838,7 @@ fhandler_fifo::raw_read (void *in_ptr, size_t& len)
   /* Poll the connected clients for input. */
   fifo_client_lock ();
   for (int i = 0; i < nhandlers; i++)
-   if (fc_handler[i].state == fc_connected)
+   if (fc_handler[i].state >= fc_connected)
  {
NTSTATUS status;
IO_STATUS_BLOCK io;
@@ -859,18 +862,14 @@ fhandler_fifo::raw_read (void *in_ptr, size_t& len)
  case STATUS_PIPE_EMPTY:
break;
  case STATUS_PIPE_BROKEN:
-   /* Client has disconnected.  Mark the client handler
-  to be deleted when it's safe to do that. */
-   fc_handler[i].state = fc_invalid;
+   fc_handler[i].state = fc_disconnected;
nconnected--;
break;
  default:
debug_printf ("NtReadFile status %y", status);
-   __seterrno_from_nt_status (status);
-   fc_handler[i].state = fc_invalid;
+   fc_handler[i].state = fc_error;
nconnected--;
-   fifo_client_unlock ();
-   goto errout;
+   break;
  }
  }
   fifo_client_unlock ();
-- 
2.21.0

[PATCH 02/21] Cygwin: FIFO: simplify the fifo_client_handler structure

2020-05-07 Thread Ken Brown via Cygwin-patches

Replace the 'fhandler_base *' member by a HANDLE to the server side of
the Windows named pipe instance.  Make the corresponding
simplifications throughout.
---
 winsup/cygwin/fhandler.h   | 19 +++---
 winsup/cygwin/fhandler_fifo.cc | 65 --
 2 files changed, 19 insertions(+), 65 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 1c7336370..e841f96ac 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1284,10 +1284,10 @@ enum
 
 struct fifo_client_handler
 {
-  fhandler_base *fh;
+  HANDLE h;
   fifo_client_connect_state state;
-  fifo_client_handler () : fh (NULL), state (fc_unknown) {}
-  int close ();
+  fifo_client_handler () : h (NULL), state (fc_unknown) {}
+  void close () { NtClose (h); }
 /* Returns FILE_PIPE_DISCONNECTED_STATE, FILE_PIPE_LISTENING_STATE,
FILE_PIPE_CONNECTED_STATE, FILE_PIPE_CLOSING_STATE,
FILE_PIPE_INPUT_AVAILABLE_STATE, or -1 on error. */
@@ -1312,7 +1312,7 @@ class fhandler_fifo: public fhandler_base
   HANDLE create_pipe_instance (bool);
   NTSTATUS open_pipe (HANDLE&);
   int add_client_handler ();
-  int delete_client_handler (int);
+  void delete_client_handler (int);
   bool listen_client ();
   int stop_listen_client ();
   int check_listen_client_thread ();
@@ -1321,8 +1321,7 @@ public:
   fhandler_fifo ();
   bool hit_eof ();
   int get_nhandlers () const { return nhandlers; }
-  HANDLE get_fc_handle (int i) const
-  { return fc_handler[i].fh->get_handle (); }
+  HANDLE get_fc_handle (int i) const { return fc_handler[i].h; }
   bool is_connected (int i) const
   { return fc_handler[i].state == fc_connected; }
   PUNICODE_STRING get_pipe_name ();
@@ -1345,12 +1344,6 @@ public:
   void fixup_after_fork (HANDLE);
   void fixup_after_exec ();
   int __reg2 fstatvfs (struct statvfs *buf);
-  void clear_readahead ()
-  {
-fhandler_base::clear_readahead ();
-for (int i = 0; i < nhandlers; i++)
-  fc_handler[i].fh->clear_readahead ();
-  }
   select_record *select_read (select_stuff *);
   select_record *select_write (select_stuff *);
   select_record *select_except (select_stuff *);
@@ -1374,8 +1367,6 @@ public:
 /* fhf->pipe_name_buf is a *copy* of this->pipe_name_buf, but
fhf->pipe_name.Buffer == this->pipe_name_buf. */
 fhf->pipe_name.Buffer = fhf->pipe_name_buf;
-for (int i = 0; i < nhandlers; i++)
-  fhf->fc_handler[i].fh = fc_handler[i].fh->clone ();
 return fhf;
   }
 };
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index c091b0add..6b71dd950 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -252,7 +252,6 @@ fhandler_fifo::add_client_handler ()
 {
   int ret = -1;
   fifo_client_handler fc;
-  fhandler_base *fh;
   HANDLE ph = NULL;
   bool first = (nhandlers == 0);
 
@@ -261,40 +260,26 @@ fhandler_fifo::add_client_handler ()
   set_errno (EMFILE);
   goto out;
 }
-  if (!(fh = build_fh_dev (dev (
-{
-  set_errno (EMFILE);
-  goto out;
-}
   ph = create_pipe_instance (first);
   if (!ph)
-{
-  delete fh;
-  goto out;
-}
+goto out;
   else
 {
-  fh->set_handle (ph);
-  fh->set_flags ((openflags & ~O_ACCMODE) | O_RDONLY);
-  fh->set_nonblocking (false);
   ret = 0;
-  fc.fh = fh;
-  fifo_client_lock ();
+  fc.h = ph;
   fc_handler[nhandlers++] = fc;
-  fifo_client_unlock ();
 }
 out:
   return ret;
 }
 
-int
+void
 fhandler_fifo::delete_client_handler (int i)
 {
-  int ret = fc_handler[i].close ();
+  fc_handler[i].close ();
   if (i < --nhandlers)
 memmove (fc_handler + i, fc_handler + i + 1,
 (nhandlers - i) * sizeof (fc_handler[i]));
-  return ret;
 }
 
 /* Just hop to the listen_client_thread method. */
@@ -331,8 +316,7 @@ fhandler_fifo::record_connection (fifo_client_handler& fc)
   SetEvent (write_ready);
   fc.state = fc_connected;
   nconnected++;
-  fc.fh->set_nonblocking (true);
-  set_pipe_non_blocking (fc.fh->get_handle (), true);
+  set_pipe_non_blocking (fc.h, true);
 }
 
 DWORD
@@ -355,13 +339,7 @@ fhandler_fifo::listen_client_thread ()
   while (i < nhandlers)
{
  if (fc_handler[i].state == fc_invalid)
-   {
- if (delete_client_handler (i) < 0)
-   {
- fifo_client_unlock ();
- goto out;
-   }
-   }
+   delete_client_handler (i);
  else
i++;
}
@@ -383,7 +361,7 @@ fhandler_fifo::listen_client_thread ()
   NTSTATUS status;
   IO_STATUS_BLOCK io;
 
-  status = NtFsControlFile (fc.fh->get_handle (), evt, NULL, NULL, ,
+  status = NtFsControlFile (fc.h, evt, NULL, NULL, ,
FSCTL_PIPE_LISTEN, NULL, 0, NULL, 0);
   if (status == STATUS_PENDING)
{
@@ -424,8 +402,7 @@ fhandler_fifo::listen_client_thread ()
  && (NT_SUCCESS (io.Status) ||

[PATCH 15/21] Cygwin: FIFO: allow fc_handler list to grow dynamically

2020-05-07 Thread Ken Brown via Cygwin-patches

Make fc_handler a pointer to malloc'd memory instead of a fixed-size
array.  The size is now a new data member 'shandlers'.  Call realloc
in add_client_handler if we need to grow the array.

free fc_handler in close.  As long as we're touching that code, also
remove an unneeded lock.
---
 winsup/cygwin/fhandler.h   |  6 ++---
 winsup/cygwin/fhandler_fifo.cc | 41 +++---
 2 files changed, 26 insertions(+), 21 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index bd44da5cd..4f42cf1b8 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1268,7 +1268,6 @@ public:
 };
 
 #define CYGWIN_FIFO_PIPE_NAME_LEN 47
-#define MAX_CLIENTS 64
 
 /* The last three are the ones we try to read from. */
 enum fifo_client_connect_state
@@ -1351,8 +1350,9 @@ class fhandler_fifo: public fhandler_base
   UNICODE_STRING pipe_name;
   WCHAR pipe_name_buf[CYGWIN_FIFO_PIPE_NAME_LEN + 1];
   bool _maybe_eof;
-  fifo_client_handler fc_handler[MAX_CLIENTS];
-  int nhandlers;
+  fifo_client_handler *fc_handler; /* Dynamically growing array. */
+  int shandlers;   /* Size (capacity) of the array. */
+  int nhandlers;   /* Number of elements in the array. */
   af_unix_spinlock_t _fifo_client_lock;
   bool reader, writer, duplexer;
   size_t max_atomic_write;
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index 0b9b33785..595e55ad9 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -70,7 +70,8 @@ static NO_COPY fifo_reader_id_t null_fr_id = { .winpid = 0, 
.fh = NULL };
 fhandler_fifo::fhandler_fifo ():
   fhandler_base (),
   read_ready (NULL), write_ready (NULL), writer_opening (NULL),
-  cancel_evt (NULL), thr_sync_evt (NULL), _maybe_eof (false), nhandlers (0),
+  cancel_evt (NULL), thr_sync_evt (NULL), _maybe_eof (false),
+  fc_handler (NULL), shandlers (0), nhandlers (0),
   reader (false), writer (false), duplexer (false),
   max_atomic_write (DEFAULT_PIPEBUFSIZE),
   me (null_fr_id), shmem_handle (NULL), shmem (NULL)
@@ -287,27 +288,28 @@ fhandler_fifo::wait_open_pipe (HANDLE& ph)
 int
 fhandler_fifo::add_client_handler ()
 {
-  int ret = -1;
   fifo_client_handler fc;
   HANDLE ph = NULL;
 
-  if (nhandlers == MAX_CLIENTS)
+  if (nhandlers >= shandlers)
 {
-  set_errno (EMFILE);
-  goto out;
+  void *temp = realloc (fc_handler,
+   (shandlers += 64) * sizeof (fc_handler[0]));
+  if (!temp)
+   {
+ shandlers -= 64;
+ set_errno (ENOMEM);
+ return -1;
+   }
+  fc_handler = (fifo_client_handler *) temp;
 }
   ph = create_pipe_instance ();
   if (!ph)
-goto out;
-  else
-{
-  ret = 0;
-  fc.h = ph;
-  fc.state = fc_listening;
-  fc_handler[nhandlers++] = fc;
-}
-out:
-  return ret;
+return -1;
+  fc.h = ph;
+  fc.state = fc_listening;
+  fc_handler[nhandlers++] = fc;
+  return 0;
 }
 
 void
@@ -1067,10 +1069,10 @@ fhandler_fifo::close ()
 NtClose (write_ready);
   if (writer_opening)
 NtClose (writer_opening);
-  fifo_client_lock ();
   for (int i = 0; i < nhandlers; i++)
 fc_handler[i].close ();
-  fifo_client_unlock ();
+  if (fc_handler)
+free (fc_handler);
   return fhandler_base::close ();
 }
 
@@ -1130,7 +1132,8 @@ fhandler_fifo::dup (fhandler_base *child, int flags)
   fhf->fifo_client_unlock ();
 
   /* Clear fc_handler list; the child never starts as owner. */
-  fhf->nhandlers = 0;
+  fhf->nhandlers = fhf->shandlers = 0;
+  fhf->fc_handler = NULL;
 
   if (!DuplicateHandle (GetCurrentProcess (), shmem_handle,
GetCurrentProcess (), >shmem_handle,
@@ -1206,6 +1209,8 @@ fhandler_fifo::fixup_after_exec ()
 
   if (reopen_shmem () < 0)
api_fatal ("Can't reopen shared memory during exec, %E");
+  fc_handler = NULL;
+  nhandlers = shandlers = 0;
   me.winpid = GetCurrentProcessId ();
   if (!(cancel_evt = create_event ()))
api_fatal ("Can't create reader thread cancel event during exec, %E");
-- 
2.21.0

[PATCH 00/21] FIFO: Support multiple readers

2020-05-07 Thread Ken Brown via Cygwin-patches

This project began as a an attempt to allow a FIFO to be opened
multiple times for reading.  The initial motivation was that Midnight
Commander running under tcsh does this (unsuccessfully on Cygwin).
See

   https://sourceware.org/pipermail/cygwin/2019-December/243317.html

It quickly became apparent, however, that the current code doesn't
even properly handle the case where the FIFO is *explicitly* opened
only once for reading, but additional readers are created via
dup/fork/exec.

This explained some of the bugs reported by Kristian Ivarsson.  See,
for example, the thread starting here:

  https://sourceware.org/pipermail/cygwin/2020-March/000206.html

as well as later similar threads.

[The discussion continued in private email, with many bug reports and
test programs by Kristian.  I'm very grateful to him for his reports
and testing.]

The first 10 patches in this series make some improvements and bug
fixes that came up along the way and don't specifically relate to
multiple readers.  The next 10 patches, with the exception of "allow
fc_handler list to grow dynamically", add the support for multiple
readers.  The last one updates the commentary at the beginning of
fhandler_fifo.cc that tries to explain how it all works.

The key ideas in these patches are:

1. Use shared memory, so that all readers have the necessary
information about the writers that are open.

2. Designate one reader as the "owner".  This reader runs a thread
that listens for connections and keeps track of the writers.

3. Use a second shared memory block to be used for transfer of
ownership.  Ownership must be transferred when the owner closes or
execs.  And a reader that wants to read or run select must take
ownership in order to be able to poll the writers for input.

The patches in this series have been applied to the topic/fifo branch
in case it's easier to review/test them there.

Ken Brown (21):
  Cygwin: FIFO: minor change - use NtClose
  Cygwin: FIFO: simplify the fifo_client_handler structure
  Cygwin: FIFO: change the fifo_client_connect_state enum
  Cygwin: FIFO: simplify the listen_client_thread code
  Cygwin: FIFO: remove the arm method
  Cygwin: FIFO: honor the flags argument in dup
  Cygwin: FIFO: dup/fork/exec: make sure child starts unlocked
  Cygwin: FIFO: fix hit_eof
  Cygwin: FIFO: make opening a writer more robust
  Cygwin: FIFO: use a cygthread instead of a homemade thread
  Cygwin: FIFO: add shared memory
  Cygwin: FIFO: keep track of the number of readers
  Cygwin: FIFO: introduce a new type, fifo_reader_id_t
  Cygwin: FIFO: designate one reader as owner
  Cygwin: FIFO: allow fc_handler list to grow dynamically
  Cygwin: FIFO: add a shared fifo_client_handler list
  Cygwin: FIFO: take ownership on exec
  Cygwin: FIFO: find a new owner when closing
  Cygwin: FIFO: allow any reader to take ownership
  Cygwin: FIFO: support opening multiple readers
  Cygwin: FIFO: update commentary

 winsup/cygwin/fhandler.h   |  208 -
 winsup/cygwin/fhandler_fifo.cc | 1564 ++--
 winsup/cygwin/select.cc|   48 +-
 3 files changed, 1311 insertions(+), 509 deletions(-)

-- 
2.21.0

[PATCH 11/21] Cygwin: FIFO: add shared memory

2020-05-07 Thread Ken Brown via Cygwin-patches

Even though we currently allow a FIFO to be opened for reading only
once, we can still have more than one reader open because of dup and
fork.  Add a named shared memory section accessible to all readers of
a given FIFO.  In future commits we will add information needed by all
readers to this section

Add a class fifo_shmem_t that lets us access this information.

Add a method create_shmem that is called when a reader opens, and add
a method reopen_shmem that is called by dup, fork, and exec.  (Each
new reader needs its own view of the shared memory.)
---
 winsup/cygwin/fhandler.h   | 13 +
 winsup/cygwin/fhandler_fifo.cc | 97 --
 2 files changed, 106 insertions(+), 4 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 5e6a1d1db..8d6b94021 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1300,6 +1300,11 @@ struct fifo_client_handler
   int pipe_state ();
 };
 
+/* Info needed by all readers of a FIFO, stored in named shared memory. */
+class fifo_shmem_t
+{
+};
+
 class fhandler_fifo: public fhandler_base
 {
   /* Handles to named events shared by all fhandlers for a given FIFO. */
@@ -1319,6 +1324,10 @@ class fhandler_fifo: public fhandler_base
   af_unix_spinlock_t _fifo_client_lock;
   bool reader, writer, duplexer;
   size_t max_atomic_write;
+
+  HANDLE shmem_handle;
+  fifo_shmem_t *shmem;
+
   bool __reg2 wait (HANDLE);
   static NTSTATUS npfs_handle (HANDLE &);
   HANDLE create_pipe_instance (bool);
@@ -1330,6 +1339,9 @@ class fhandler_fifo: public fhandler_base
   void record_connection (fifo_client_handler&,
  fifo_client_connect_state = fc_connected);
 
+  int create_shmem ();
+  int reopen_shmem ();
+
 public:
   fhandler_fifo ();
   bool hit_eof ();
@@ -1341,6 +1353,7 @@ public:
   DWORD fifo_reader_thread_func ();
   void fifo_client_lock () { _fifo_client_lock.lock (); }
   void fifo_client_unlock () { _fifo_client_lock.unlock (); }
+
   int open (int, mode_t);
   off_t lseek (off_t offset, int whence);
   int close ();
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index 09a7eb321..9a0db3f33 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -70,7 +70,8 @@ fhandler_fifo::fhandler_fifo ():
   read_ready (NULL), write_ready (NULL), writer_opening (NULL),
   cancel_evt (NULL), thr_sync_evt (NULL), _maybe_eof (false), nhandlers (0),
   reader (false), writer (false), duplexer (false),
-  max_atomic_write (DEFAULT_PIPEBUFSIZE)
+  max_atomic_write (DEFAULT_PIPEBUFSIZE),
+  shmem_handle (NULL), shmem (NULL)
 {
   pipe_name_buf[0] = L'\0';
   need_fork_fixup (true);
@@ -441,6 +442,67 @@ canceled:
   return 0;
 }
 
+int
+fhandler_fifo::create_shmem ()
+{
+  HANDLE sect;
+  OBJECT_ATTRIBUTES attr;
+  NTSTATUS status;
+  LARGE_INTEGER size = { .QuadPart = sizeof (fifo_shmem_t) };
+  SIZE_T viewsize = sizeof (fifo_shmem_t);
+  PVOID addr = NULL;
+  UNICODE_STRING uname;
+  WCHAR shmem_name[MAX_PATH];
+
+  __small_swprintf (shmem_name, L"fifo-shmem.%08x.%016X", get_dev (),
+   get_ino ());
+  RtlInitUnicodeString (, shmem_name);
+  InitializeObjectAttributes (, , OBJ_INHERIT,
+ get_shared_parent_dir (), NULL);
+  status = NtCreateSection (, STANDARD_RIGHTS_REQUIRED | SECTION_QUERY
+   | SECTION_MAP_READ | SECTION_MAP_WRITE,
+   , , PAGE_READWRITE, SEC_COMMIT, NULL);
+  if (status == STATUS_OBJECT_NAME_COLLISION)
+status = NtOpenSection (, STANDARD_RIGHTS_REQUIRED | SECTION_QUERY
+   | SECTION_MAP_READ | SECTION_MAP_WRITE, );
+  if (!NT_SUCCESS (status))
+{
+  __seterrno_from_nt_status (status);
+  return -1;
+}
+  status = NtMapViewOfSection (sect, NtCurrentProcess (), , 0, viewsize,
+  NULL, , ViewShare, 0, PAGE_READWRITE);
+  if (!NT_SUCCESS (status))
+{
+  NtClose (sect);
+  __seterrno_from_nt_status (status);
+  return -1;
+}
+  shmem_handle = sect;
+  shmem = (fifo_shmem_t *) addr;
+  return 0;
+}
+
+/* shmem_handle must be valid when this is called. */
+int
+fhandler_fifo::reopen_shmem ()
+{
+  NTSTATUS status;
+  SIZE_T viewsize = sizeof (fifo_shmem_t);
+  PVOID addr = NULL;
+
+  status = NtMapViewOfSection (shmem_handle, NtCurrentProcess (), ,
+  0, viewsize, NULL, , ViewShare,
+  0, PAGE_READWRITE);
+  if (!NT_SUCCESS (status))
+{
+  __seterrno_from_nt_status (status);
+  return -1;
+}
+  shmem = (fifo_shmem_t *) addr;
+  return 0;
+}
+
 int
 fhandler_fifo::open (int flags, mode_t)
 {
@@ -501,12 +563,15 @@ fhandler_fifo::open (int flags, mode_t)
   goto err_close_write_ready;
 }
 
-  /* If we're reading, signal read_ready and start the fifo_reader_thread. */
+  /* If we're reading, signal read_ready, create the shared memory,
+ and start the

[PATCH 19/21] Cygwin: FIFO: allow any reader to take ownership

2020-05-07 Thread Ken Brown via Cygwin-patches

Add a take_ownership method, used by raw_read and select.cc:peek_fifo.
It wakes up all fifo_reader_threads and allows the caller to become
owner.  The work is done by the fifo_reader_threads.

For synchronization we introduce several new fhandler_fifo data
members and methods:

- update_needed_evt signals the current owner to stop listening for
  writer connections and update its fc_handler list.

- shared_fc_handler() gets and sets the status of the fc_handler
  update process.

- get_pending_owner() and set_pending_owner() get and set the reader
  that is requesting ownership.

Finally, a new 'reading_lock' prevents two readers from trying to take
ownership simultaneously.
---
 winsup/cygwin/fhandler.h   |  28 -
 winsup/cygwin/fhandler_fifo.cc | 106 +
 winsup/cygwin/select.cc|   4 ++
 3 files changed, 122 insertions(+), 16 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index f8c1b52a4..31c65866e 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1323,12 +1323,13 @@ struct fifo_reader_id_t
 class fifo_shmem_t
 {
   LONG _nreaders;
-  fifo_reader_id_t _owner, _prev_owner;
-  af_unix_spinlock_t _owner_lock;
+  fifo_reader_id_t _owner, _prev_owner, _pending_owner;
+  af_unix_spinlock_t _owner_lock, _reading_lock;
 
   /* Info about shared memory block used for temporary storage of the
  owner's fc_handler list. */
-  LONG _sh_nhandlers, _sh_shandlers, _sh_fc_handler_committed;
+  LONG _sh_nhandlers, _sh_shandlers, _sh_fc_handler_committed,
+_sh_fc_handler_updated;
 
 public:
   int inc_nreaders () { return (int) InterlockedIncrement (&_nreaders); }
@@ -1338,9 +1339,13 @@ public:
   void set_owner (fifo_reader_id_t fr_id) { _owner = fr_id; }
   fifo_reader_id_t get_prev_owner () const { return _prev_owner; }
   void set_prev_owner (fifo_reader_id_t fr_id) { _prev_owner = fr_id; }
+  fifo_reader_id_t get_pending_owner () const { return _pending_owner; }
+  void set_pending_owner (fifo_reader_id_t fr_id) { _pending_owner = fr_id; }
 
   void owner_lock () { _owner_lock.lock (); }
   void owner_unlock () { _owner_lock.unlock (); }
+  void reading_lock () { _reading_lock.lock (); }
+  void reading_unlock () { _reading_lock.unlock (); }
 
   int get_shared_nhandlers () const { return (int) _sh_nhandlers; }
   void set_shared_nhandlers (int n) { InterlockedExchange (&_sh_nhandlers, n); 
}
@@ -1350,6 +1355,9 @@ public:
   { return (size_t) _sh_fc_handler_committed; }
   void set_shared_fc_handler_committed (size_t n)
   { InterlockedExchange (&_sh_fc_handler_committed, (LONG) n); }
+  bool shared_fc_handler_updated () const { return _sh_fc_handler_updated; }
+  void shared_fc_handler_updated (bool val)
+  { InterlockedExchange (&_sh_fc_handler_updated, val); }
 };
 
 class fhandler_fifo: public fhandler_base
@@ -1362,6 +1370,7 @@ class fhandler_fifo: public fhandler_base
   /* Handles to named events needed by all readers of a given FIFO. */
   HANDLE owner_needed_evt;  /* The owner is closing. */
   HANDLE owner_found_evt;   /* A new owner has taken over. */
+  HANDLE update_needed_evt; /* shared_fc_handler needs updating. */
 
   /* Handles to non-shared events needed for fifo_reader_threads. */
   HANDLE cancel_evt;/* Signal thread to terminate. */
@@ -1409,6 +1418,11 @@ class fhandler_fifo: public fhandler_base
   fifo_reader_id_t get_prev_owner () const { return shmem->get_prev_owner (); }
   void set_prev_owner (fifo_reader_id_t fr_id)
   { shmem->set_prev_owner (fr_id); }
+  fifo_reader_id_t get_pending_owner () const
+  { return shmem->get_pending_owner (); }
+  void set_pending_owner (fifo_reader_id_t fr_id)
+  { shmem->set_pending_owner (fr_id); }
+
   void owner_needed ()
   {
 ResetEvent (owner_found_evt);
@@ -1430,6 +1444,10 @@ class fhandler_fifo: public fhandler_base
   { shmem->set_shared_fc_handler_committed (n); }
   int update_my_handlers (bool from_exec = false);
   int update_shared_handlers ();
+  bool shared_fc_handler_updated () const
+  { return shmem->shared_fc_handler_updated (); }
+  void shared_fc_handler_updated (bool val)
+  { shmem->shared_fc_handler_updated (val); }
 
 public:
   fhandler_fifo ();
@@ -1449,6 +1467,10 @@ public:
   void owner_lock () { shmem->owner_lock (); }
   void owner_unlock () { shmem->owner_unlock (); }
 
+  void take_ownership ();
+  void reading_lock () { shmem->reading_lock (); }
+  void reading_unlock () { shmem->reading_unlock (); }
+
   int open (int, mode_t);
   off_t lseek (off_t offset, int whence);
   int close ();
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index bf33a52d6..81473015e 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -74,7 +74,7 @@ static NO_COPY fifo_reader_id_t null_fr_id = { .winpid = 0, 
.fh = NULL };
 fhandler_fifo::fhandler_fifo ():
   fhandler_base (),
   read_ready (NULL), write_ready (NULL), writer_opening (NULL),
-

[PATCH 13/21] Cygwin: FIFO: introduce a new type, fifo_reader_id_t

2020-05-07 Thread Ken Brown via Cygwin-patches

This uniquely identifies an fhandler_fifo open for reading in any
process.

Add a new data member 'me' of this type, which is set in open, dup,
fork, and exec.
---
 winsup/cygwin/fhandler.h   | 23 +++
 winsup/cygwin/fhandler_fifo.cc |  9 -
 2 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index b2ee7e6b6..65aab4da3 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1300,6 +1300,26 @@ struct fifo_client_handler
   int pipe_state ();
 };
 
+class fhandler_fifo;
+
+struct fifo_reader_id_t
+{
+  DWORD winpid;
+  fhandler_fifo *fh;
+
+  operator bool () const { return winpid != 0 || fh != NULL; }
+
+  friend operator == (const fifo_reader_id_t , const fifo_reader_id_t )
+  {
+return l.winpid == r.winpid && l.fh == r.fh;
+  }
+
+  friend operator != (const fifo_reader_id_t , const fifo_reader_id_t )
+  {
+return l.winpid != r.winpid || l.fh != r.fh;
+  }
+};
+
 /* Info needed by all readers of a FIFO, stored in named shared memory. */
 class fifo_shmem_t
 {
@@ -1329,6 +1349,7 @@ class fhandler_fifo: public fhandler_base
   af_unix_spinlock_t _fifo_client_lock;
   bool reader, writer, duplexer;
   size_t max_atomic_write;
+  fifo_reader_id_t me;
 
   HANDLE shmem_handle;
   fifo_shmem_t *shmem;
@@ -1362,6 +1383,8 @@ public:
   void fifo_client_lock () { _fifo_client_lock.lock (); }
   void fifo_client_unlock () { _fifo_client_lock.unlock (); }
 
+  fifo_reader_id_t get_me () const { return me; }
+
   int open (int, mode_t);
   off_t lseek (off_t offset, int whence);
   int close ();
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index d87070ac7..5676a2c97 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -65,13 +65,15 @@ STATUS_PIPE_EMPTY simply means there's no data to be read. 
*/
   || _s == STATUS_PIPE_NOT_AVAILABLE \
   || _s == STATUS_PIPE_BUSY; })
 
+static NO_COPY fifo_reader_id_t null_fr_id = { .winpid = 0, .fh = NULL };
+
 fhandler_fifo::fhandler_fifo ():
   fhandler_base (),
   read_ready (NULL), write_ready (NULL), writer_opening (NULL),
   cancel_evt (NULL), thr_sync_evt (NULL), _maybe_eof (false), nhandlers (0),
   reader (false), writer (false), duplexer (false),
   max_atomic_write (DEFAULT_PIPEBUFSIZE),
-  shmem_handle (NULL), shmem (NULL)
+  me (null_fr_id), shmem_handle (NULL), shmem (NULL)
 {
   pipe_name_buf[0] = L'\0';
   need_fork_fixup (true);
@@ -575,6 +577,8 @@ fhandler_fifo::open (int flags, mode_t)
goto err_dec_nreaders;
   if (!(thr_sync_evt = create_event ()))
goto err_close_cancel_evt;
+  me.winpid = GetCurrentProcessId ();
+  me.fh = this;
   new cygthread (fifo_reader_thread, this, "fifo_reader", thr_sync_evt);
 
   /* If we're a duplexer, we need a handle for writing. */
@@ -1119,6 +1123,7 @@ fhandler_fifo::dup (fhandler_base *child, int flags)
   if (!(fhf->thr_sync_evt = create_event ()))
goto err_close_cancel_evt;
   inc_nreaders ();
+  fhf->me.fh = fhf;
   new cygthread (fifo_reader_thread, fhf, "fifo_reader", 
fhf->thr_sync_evt);
 }
   return 0;
@@ -1164,6 +1169,7 @@ fhandler_fifo::fixup_after_fork (HANDLE parent)
   if (!(thr_sync_evt = create_event ()))
api_fatal ("Can't create reader thread sync event during fork, %E");
   inc_nreaders ();
+  me.winpid = GetCurrentProcessId ();
   new cygthread (fifo_reader_thread, this, "fifo_reader", thr_sync_evt);
 }
 }
@@ -1179,6 +1185,7 @@ fhandler_fifo::fixup_after_exec ()
 
   if (reopen_shmem () < 0)
api_fatal ("Can't reopen shared memory during exec, %E");
+  me.winpid = GetCurrentProcessId ();
   if (!(cancel_evt = create_event ()))
api_fatal ("Can't create reader thread cancel event during exec, %E");
   if (!(thr_sync_evt = create_event ()))
-- 
2.21.0

[PATCH 14/21] Cygwin: FIFO: designate one reader as owner

2020-05-07 Thread Ken Brown via Cygwin-patches

Among all the open readers of a FIFO, one is declared to be the owner.
This is the only reader that listens for client connections, and it is
the only one that has an accurate fc_handler list.

Add shared data and methods for getting and setting the owner, as well
as a lock to prevent more than one reader from accessing these data
simultaneously.

Modify the fifo_reader_thread so that it checks the owner at the
beginning of its loop.  If there is no owner, it takes ownership.  If
there is an owner but it is a different reader, the thread just waits
to be canceled.  Otherwise, it listens for client connections as
before.

Remove the 'first' argument from create_pipe_instance.  It is not
needed, and it may be confusing in the future since only the owner
knows whether a pipe instance is the first.

When opening a reader, don't return until the fifo_reader_thread has
time to set an owner.

If the owner closes, indicate that there is no longer an owner.

Clear the child's fc_handler list in dup, and don't bother duplicating
the handles.  The child never starts out as owner, so it can't use
those handles.

Do the same thing in fixup_after_fork in the close-on-exec case.  In
the non-close-on-exec case, the child inherits an fc_handler list that
it can't use, but we can just leave it alone; the handles will be
closed when the child is closed.
---
 winsup/cygwin/fhandler.h   |  13 +-
 winsup/cygwin/fhandler_fifo.cc | 237 ++---
 2 files changed, 141 insertions(+), 109 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 65aab4da3..bd44da5cd 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1324,10 +1324,17 @@ struct fifo_reader_id_t
 class fifo_shmem_t
 {
   LONG _nreaders;
+  fifo_reader_id_t _owner;
+  af_unix_spinlock_t _owner_lock;
 
 public:
   int inc_nreaders () { return (int) InterlockedIncrement (&_nreaders); }
   int dec_nreaders () { return (int) InterlockedDecrement (&_nreaders); }
+
+  fifo_reader_id_t get_owner () const { return _owner; }
+  void set_owner (fifo_reader_id_t fr_id) { _owner = fr_id; }
+  void owner_lock () { _owner_lock.lock (); }
+  void owner_unlock () { _owner_lock.unlock (); }
 };
 
 class fhandler_fifo: public fhandler_base
@@ -1356,7 +1363,7 @@ class fhandler_fifo: public fhandler_base
 
   bool __reg2 wait (HANDLE);
   static NTSTATUS npfs_handle (HANDLE &);
-  HANDLE create_pipe_instance (bool);
+  HANDLE create_pipe_instance ();
   NTSTATUS open_pipe (HANDLE&);
   NTSTATUS wait_open_pipe (HANDLE&);
   int add_client_handler ();
@@ -1384,6 +1391,10 @@ public:
   void fifo_client_unlock () { _fifo_client_lock.unlock (); }
 
   fifo_reader_id_t get_me () const { return me; }
+  fifo_reader_id_t get_owner () const { return shmem->get_owner (); }
+  void set_owner (fifo_reader_id_t fr_id) { shmem->set_owner (fr_id); }
+  void owner_lock () { shmem->owner_lock (); }
+  void owner_unlock () { shmem->owner_unlock (); }
 
   int open (int, mode_t);
   off_t lseek (off_t offset, int whence);
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index 5676a2c97..0b9b33785 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -164,7 +164,7 @@ fhandler_fifo::npfs_handle (HANDLE )
blocking mode so that we can easily wait for a connection.  After
it is connected, it is put in nonblocking mode. */
 HANDLE
-fhandler_fifo::create_pipe_instance (bool first)
+fhandler_fifo::create_pipe_instance ()
 {
   NTSTATUS status;
   HANDLE npfsh;
@@ -187,14 +187,12 @@ fhandler_fifo::create_pipe_instance (bool first)
   access = GENERIC_READ | FILE_READ_ATTRIBUTES | FILE_WRITE_ATTRIBUTES
 | SYNCHRONIZE;
   sharing = FILE_SHARE_READ | FILE_SHARE_WRITE;
-  hattr = openflags & O_CLOEXEC ? 0 : OBJ_INHERIT;
-  if (first)
-hattr |= OBJ_CASE_INSENSITIVE;
+  hattr = (openflags & O_CLOEXEC ? 0 : OBJ_INHERIT) | OBJ_CASE_INSENSITIVE;
   InitializeObjectAttributes (, get_pipe_name (),
  hattr, npfsh, NULL);
   timeout.QuadPart = -50;
   status = NtCreateNamedPipeFile (, access, , , sharing,
- first ? FILE_CREATE : FILE_OPEN, 0,
+ FILE_OPEN_IF, 0,
  FILE_PIPE_MESSAGE_TYPE
| FILE_PIPE_REJECT_REMOTE_CLIENTS,
  FILE_PIPE_MESSAGE_MODE,
@@ -292,14 +290,13 @@ fhandler_fifo::add_client_handler ()
   int ret = -1;
   fifo_client_handler fc;
   HANDLE ph = NULL;
-  bool first = (nhandlers == 0);
 
   if (nhandlers == MAX_CLIENTS)
 {
   set_errno (EMFILE);
   goto out;
 }
-  ph = create_pipe_instance (first);
+  ph = create_pipe_instance ();
   if (!ph)
 goto out;
   else
@@ -349,92 +346,120 @@ fhandler_fifo::fifo_reader_thread_func ()
 
   while (1)
 {
-  /* Cleanup the fc_handler list. */
-  fifo_client_lock ();
-  int i = 0;
-  while (i <

[PATCH 21/21] Cygwin: FIFO: update commentary

2020-05-07 Thread Ken Brown via Cygwin-patches

The beginning of fhandler_fifo.cc contains a long comment giving an
overview of the FIFO implementation.  This is now updated to describe
the support for multiple readers.
---
 winsup/cygwin/fhandler_fifo.cc | 58 --
 1 file changed, 35 insertions(+), 23 deletions(-)

diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index cc51c449a..7290b4655 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -26,29 +26,41 @@
 /*
Overview:
 
- Currently a FIFO can be opened once for reading and multiple
- times for writing.  Any attempt to open the FIFO a second time
- for reading fails with EACCES (from STATUS_ACCESS_DENIED).
-
- When a FIFO is opened for reading,
- fhandler_fifo::create_pipe_instance is called to create the first
- instance of a Windows named pipe server (Windows terminology).  A
- "fifo_reader" thread is also started; it waits for pipe clients
- (Windows terminology again) to connect.  This happens every time
- a process opens the FIFO for writing.
-
- The fifo_reader thread creates new instances of the pipe server
- as needed, so that there is always an instance available for a
- writer to connect to.
-
- The reader maintains a list of "fifo_client_handlers", one for
- each pipe instance.  A fifo_client_handler manages the connection
- between the pipe instance and a writer connected to that pipe
- instance.
-
- TODO: Allow a FIFO to be opened multiple times for reading.
- Maybe this could be done by using shared memory, so that all
- readers could have access to the same list of writers.
+ FIFOs are implemented via Windows named pipes.  The server end of
+ the pipe corresponds to an fhandler_fifo open for reading (a.k.a,
+ a "reader"), and the client end corresponds to an fhandler_fifo
+ open for writing (a.k.a, a "writer").
+
+ The server can have multiple instances.  The reader (assuming for
+ the moment that there is only one) creates a pipe instance for
+ each writer that opens.  The reader maintains a list of
+ "fifo_client_handler" structures, one for each writer.  A
+ fifo_client_handler contains the handle for the pipe server
+ instance and information about the state of the connection with
+ the writer.  Each writer holds the pipe instance's client handle.
+
+ The reader runs a "fifo_reader_thread" that creates new pipe
+ instances as needed and listens for client connections.
+
+ If there are multiple readers open, only one of them, called the
+ "owner", maintains the fifo_client_handler list.  The owner is
+ therefore the only reader that can read at any given time.  If a
+ different reader wants to read, it has to take ownership and
+ duplicate the fifo_client_handler list.
+
+ A reader that is not an owner also runs a fifo_reader_thread,
+ which is mostly idle.  The thread wakes up if that reader might
+ need to take ownership.
+
+ There is a block of shared memory, accessible to all readers,
+ that contains information needed for the owner change process.
+ It also contains some locks to prevent races and deadlocks
+ between the various threads.
+
+ At this writing, I know of only one application (Midnight
+ Commander when running under tcsh) that *explicitly* opens two
+ readers of a FIFO.  But many applications will have multiple
+ readers open via dup/fork/exec.
 */
 
 
-- 
2.21.0

Re: [PATCH 00/21] FIFO: Support multiple readers

2020-05-19 Thread Ken Brown via Cygwin-patches

On 5/19/2020 8:51 AM, Ken Brown via Cygwin-patches wrote:

On 5/19/2020 2:15 AM, Takashi Yano via Cygwin-patches wrote:

On Tue, 19 May 2020 10:26:09 +0900
Takashi Yano via Cygwin-patches  wrote:

Hi Ken,

On Mon, 18 May 2020 13:42:19 -0400
Ken Brown via Cygwin-patches  wrote:

Hi Takashi,

On 5/18/2020 12:03 PM, Ken Brown via Cygwin-patches wrote:

On 5/18/2020 1:36 AM, Takashi Yano via Cygwin-patches wrote:

On Mon, 18 May 2020 14:25:19 +0900
Takashi Yano via Cygwin-patches  wrote:

However, mc hangs by several operations.

To reproduce this:
1. Start mc with 'env SHELL=tcsh mc -a'

I mean 'env SHELL=/bin/tcsh mc -a'

2. Select a file using up/down cursor keys.
3. Press F3 (View) key.

Thanks for the report.  I can reproduce the problem and will look into it.

I'm not convinced that this is a FIFO bug.  I tried two things.

1. I attached gdb to mc while it was hanging and got the following backtrace
(abbreviated):

#1  0x7ff901638037 in WaitForMultipleObjectsEx ()
#2  0x7ff901637f1e in WaitForMultipleObjects ()
#3  0x000180048df5 in cygwait () at ...winsup/cygwin/cygwait.cc:75
#4  0x00018019b1c0 in wait4 () at ...winsup/cygwin/wait.cc:80
#5  0x00018019afea in waitpid () at ...winsup/cygwin/wait.cc:28
#6  0x00018017d2d8 in pclose () at ...winsup/cygwin/syscalls.cc:4627
#7  0x00018015943b in _sigfe () at sigfe.s:35
#8  0x00010040d002 in get_popen_information () at filemanager/ext.c:561
[...]

So pclose is blocking after calling waitpid.  As far as I can tell from looking
at backtraces of all threads, there are no FIFOs open.

2. I ran mc under strace (after exporting SHELL=/bin/tcsh), and I didn't see
anything suspicious involving FIFOs.  But I saw many EBADF errors from fstat 
and

close that don't appear to be related to FIFOs.

So my best guess at this point is that the FIFO changes just exposed some
unrelated bug(s).

Prior to the FIFO changes, mc would get an error when it tried to open 
tcsh_fifo

the second time, and it would then set

    mc_global.tty.use_subshell = FALSE;

see the mc source file subshell/common.c:1087.

I looked into this problem and found pclose() stucks if FIFO
is opened.

Attached is a simple test case. It works under cygwin 3.1.4,
but stucks at pclose() under cygwin git head.

Isn't this a FIFO related issue?

In the test case, fhandler_fifo::close() called from /bin/true
seems to get into infinite loop at:

do
...
while (inc_nreaders () > 0 && !found);

Thank you!  I see the problem.  After the popen call, the original 
fhandler_fifo's fifo_reader_thread was no longer running, so it was unable to 
take ownership.

It might take a little while for me to figure out how to fix this.

Actually, I think it's easy.  Please try the two attached patches.  The second 
one is the crucial one for the mc problem, but the first is something I noticed 
while working on this.

I just did a quick and dirty patch for testing purposes.  I still have to do a 
lot of cleanup and make sure the fix doesn't break something else.

Ken
>From bdc0bd172b09d386a2f15c41e7a764f48748b90c Mon Sep 17 00:00:00 2001
From: Ken Brown 
Date: Tue, 19 May 2020 09:03:59 -0400
Subject: [PATCH 1/2] Cygwin: FIFO: add missing reader_opening_unlock

---
 winsup/cygwin/fhandler_fifo.cc | 1 +
 1 file changed, 1 insertion(+)

diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index e8a05dfbf..0dc356dfe 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -1047,6 +1047,7 @@ err_close_reader:
   saved_errno = get_errno ();
   close ();
   set_errno (saved_errno);
+  reader_opening_unlock ();
   return 0;
 err_close_cancel_evt:
   NtClose (cancel_evt);
-- 
2.21.0

>From ce60aa56a573e712ce5d212ca6aa0ddd9781be29 Mon Sep 17 00:00:00 2001
From: Ken Brown 
Date: Tue, 19 May 2020 09:28:30 -0400
Subject: [PATCH 2/2] Cygwin: FIFO: don't take ownership after exec

Proof of concept only.  Needs cleanup and further testing.

There's no need to transfer ownership after exec.  This will happen
automatically, if necessary, when the parent closes.  And canceling
the parent's fifo_reader_thread can cause problems, as reported here
for example:

https://cygwin.com/pipermail/cygwin-patches/2020q2/010235.html
---
 winsup/cygwin/fhandler_fifo.cc | 35 +-
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index 0dc356dfe..79f83177f 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -1727,23 +1727,23 @@ fhandler_fifo::fixup_after_exec ()
   fc_handler = NULL;
   nhandlers = shandlers = 0;

-  /* Cancel parent's reader thread */
-  if (cancel_evt)
-   SetEvent (cancel_evt);
-  if (thr_sync_evt)
-   WaitForSingleObject (thr_sync_evt, INFINITE);
-
-  /* Take ownership if parent is owner. */
-  fifo_reader_id_t parent_fr = me;
-  me.winpid = Ge

Re: [PATCH] Cygwin: pty: Make system_printf() work after closing pty slave.

2020-05-19 Thread Ken Brown via Cygwin-patches


Hi Takashi,

On 5/19/2020 7:35 AM, Takashi Yano via Cygwin-patches wrote:

- Current pty cannot show system_printf() output after closing pty
   slave. This patch fixes the issue.


Sorry to be returning the favor so soon, but this patch causes 'make check' in 
the texinfo source tree to hang.  I don't have time at the moment to try to 
produce a simple test case, so here's a complicated way to reproduce the problem:


1. Clone the texinfo git repo:

  $ git clone https://git.savannah.gnu.org/git/texinfo.git

2. Build texinfo:

  $ cd texinfo
  $ ./autogen.sh && ./configure # Maybe CFLAGS='-g -O0' for debugging
  $ make

3. Test the standalone info reader:

  $ cd info
  $ make check

It hangs while running the test t/malformed-split.sh, leaving a ginfo process 
and a pseudotty process running, with ginfo trying to close a pty slave.


Note that this test uses both ptys and fifos, so there's always a chance that 
this is another fifo bug.  But reverting your patch fixes the problem, so I 
think it's probably a pty bug.


Ken

Re: [PATCH 00/21] FIFO: Support multiple readers

2020-05-22 Thread Ken Brown via Cygwin-patches


On 5/19/2020 10:07 AM, Takashi Yano wrote:

On Tue, 19 May 2020 09:37:17 -0400
Ken Brown via Cygwin-patches  wrote:

On 5/19/2020 8:51 AM, Ken Brown via Cygwin-patches wrote:

On 5/19/2020 2:15 AM, Takashi Yano via Cygwin-patches wrote:

On Tue, 19 May 2020 10:26:09 +0900
Takashi Yano via Cygwin-patches  wrote:

Hi Ken,

On Mon, 18 May 2020 13:42:19 -0400
Ken Brown via Cygwin-patches  wrote:

Hi Takashi,

On 5/18/2020 12:03 PM, Ken Brown via Cygwin-patches wrote:

On 5/18/2020 1:36 AM, Takashi Yano via Cygwin-patches wrote:

On Mon, 18 May 2020 14:25:19 +0900
Takashi Yano via Cygwin-patches  wrote:

However, mc hangs by several operations.

To reproduce this:
1. Start mc with 'env SHELL=tcsh mc -a'


I mean 'env SHELL=/bin/tcsh mc -a'


2. Select a file using up/down cursor keys.
3. Press F3 (View) key.


Thanks for the report.  I can reproduce the problem and will look into it.


I'm not convinced that this is a FIFO bug.  I tried two things.

1. I attached gdb to mc while it was hanging and got the following backtrace
(abbreviated):

#1  0x7ff901638037 in WaitForMultipleObjectsEx ()
#2  0x7ff901637f1e in WaitForMultipleObjects ()
#3  0x000180048df5 in cygwait () at ...winsup/cygwin/cygwait.cc:75
#4  0x00018019b1c0 in wait4 () at ...winsup/cygwin/wait.cc:80
#5  0x00018019afea in waitpid () at ...winsup/cygwin/wait.cc:28
#6  0x00018017d2d8 in pclose () at ...winsup/cygwin/syscalls.cc:4627
#7  0x00018015943b in _sigfe () at sigfe.s:35
#8  0x00010040d002 in get_popen_information () at filemanager/ext.c:561
[...]

So pclose is blocking after calling waitpid.  As far as I can tell from looking
at backtraces of all threads, there are no FIFOs open.

2. I ran mc under strace (after exporting SHELL=/bin/tcsh), and I didn't see
anything suspicious involving FIFOs.  But I saw many EBADF errors from fstat
and
close that don't appear to be related to FIFOs.

So my best guess at this point is that the FIFO changes just exposed some
unrelated bug(s).

Prior to the FIFO changes, mc would get an error when it tried to open
tcsh_fifo
the second time, and it would then set

     mc_global.tty.use_subshell = FALSE;

see the mc source file subshell/common.c:1087.


I looked into this problem and found pclose() stucks if FIFO
is opened.

Attached is a simple test case. It works under cygwin 3.1.4,
but stucks at pclose() under cygwin git head.

Isn't this a FIFO related issue?


In the test case, fhandler_fifo::close() called from /bin/true
seems to get into infinite loop at:

do
...
while (inc_nreaders () > 0 && !found);


Thank you!  I see the problem.  After the popen call, the original
fhandler_fifo's fifo_reader_thread was no longer running, so it was unable to
take ownership.

It might take a little while for me to figure out how to fix this.


Actually, I think it's easy.  Please try the two attached patches.  The second
one is the crucial one for the mc problem, but the first is something I noticed
while working on this.

I just did a quick and dirty patch for testing purposes.  I still have to do a
lot of cleanup and make sure the fix doesn't break something else.


For a shor time, I tested these patches, and confirmed
that this fixes the problem.

Thanks for the quick response.


I've just pushed cleaned-up versions of the patches.

Ken

Re: [PATCH] Cygwin: pty: Revise code to make system_printf() work after close.

2020-05-21 Thread Ken Brown via Cygwin-patches


On 5/21/2020 4:25 AM, Takashi Yano via Cygwin-patches wrote:

- After commit 0365031ce1347600d854a23f30f1355745a1765c, the issue
   https://cygwin.com/pipermail/cygwin-patches/2020q2/010259.html
   occurs. This patch fixes the issue.


Thanks.  I can confirm that it's fixed.

Ken

[PATCH 0/2] Fix problems with /proc//fd checking

2020-09-08 Thread Ken Brown via Cygwin-patches

This fixes the assertion failure reported here:

  https://sourceware.org/pipermail/cygwin/2020-September/246160.html

with a simple test case here:

  https://sourceware.org/pipermail/cygwin/2020-September/246188.html

That test case now fails as follows, as on Linux:

$ ./proc_bug.exe
open: Not a directory

I'm not completely sure about the second patch.  The path_conv::check
code is so complicated that I could easily have missed something.  But
I think it's correct.

Ken Brown (2):
  Cygwin: format_process_fd: add directory check
  Cygwin: path_conv::check: handle error from fhandler_process::exists

 winsup/cygwin/fhandler_process.cc | 15 +++
 winsup/cygwin/path.cc |  6 ++
 2 files changed, 21 insertions(+)

-- 
2.28.0

[PATCH 1/2] Cygwin: format_process_fd: add directory check

2020-09-08 Thread Ken Brown via Cygwin-patches

The incoming path is allowed to have the form "$PID/fd/[0-9]*/.*"
provided the descriptor symlink points to a directory.  Check that
this is indeed the case.
---
 winsup/cygwin/fhandler_process.cc | 15 +++
 1 file changed, 15 insertions(+)

diff --git a/winsup/cygwin/fhandler_process.cc 
b/winsup/cygwin/fhandler_process.cc
index a6c358217..888604e3d 100644
--- a/winsup/cygwin/fhandler_process.cc
+++ b/winsup/cygwin/fhandler_process.cc
@@ -398,6 +398,21 @@ format_process_fd (void *data, char *)
*((process_fd_t *) data)->fd_type = virt_fdsymlink;
   else /* trailing path */
{
+ /* Does the descriptor link point to a directory? */
+ bool dir;
+ if (*destbuf != '/')  /* e.g., "pipe:[" or "socket:[" */
+   dir = false;
+ else
+   {
+ path_conv pc (destbuf);
+ dir = pc.isdir ();
+   }
+ if (!dir)
+   {
+ set_errno (ENOTDIR);
+ cfree (destbuf);
+ return -1;
+   }
  char *newbuf = (char *) cmalloc_abort (HEAP_STR, strlen (destbuf)
   + strlen (e) + 1);
  stpcpy (stpcpy (newbuf, destbuf), e);
-- 
2.28.0

[PATCH 2/2] Cygwin: path_conv::check: handle error from fhandler_process::exists

2020-09-08 Thread Ken Brown via Cygwin-patches

fhandler_process::exists is called when we are checking a path
starting with "/proc//fd".  If it returns virt_none and sets an
errno, there is no need for further checking.  Just set 'error' and
return.
---
 winsup/cygwin/path.cc | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/winsup/cygwin/path.cc b/winsup/cygwin/path.cc
index 95faf8ca7..092261939 100644
--- a/winsup/cygwin/path.cc
+++ b/winsup/cygwin/path.cc
@@ -809,6 +809,12 @@ path_conv::check (const char *src, unsigned opt,
  delete fh;
  goto retry_fs_via_processfd;
}
+ else if (file_type == virt_none && dev == FH_PROCESSFD)
+   {
+ error = get_errno ();
+ if (error)
+   return;
+   }
  delete fh;
}
  switch (file_type)
-- 
2.28.0

Re: [PATCH 2/2] Cygwin: path_conv::check: handle error from fhandler_process::exists

2020-09-08 Thread Ken Brown via Cygwin-patches


On 9/8/2020 12:50 PM, Ken Brown via Cygwin-patches wrote:

fhandler_process::exists is called when we are checking a path
starting with "/proc//fd".  If it returns virt_none and sets an
errno, there is no need for further checking.  Just set 'error' and
return.
---
  winsup/cygwin/path.cc | 6 ++
  1 file changed, 6 insertions(+)

diff --git a/winsup/cygwin/path.cc b/winsup/cygwin/path.cc
index 95faf8ca7..092261939 100644
--- a/winsup/cygwin/path.cc
+++ b/winsup/cygwin/path.cc
@@ -809,6 +809,12 @@ path_conv::check (const char *src, unsigned opt,
  delete fh;
  goto retry_fs_via_processfd;
}
+ else if (file_type == virt_none && dev == FH_PROCESSFD)
+   {
+ error = get_errno ();
+ if (error)
+   return;
+   }
  delete fh;
}
  switch (file_type)



There's a missing 'delete fh' above.  I'll send v2 shortly.

Ken

Re: [PATCH v2 2/3] Cygwin: path_conv::check: handle error from fhandler_process::exists

2020-09-08 Thread Ken Brown via Cygwin-patches


On 9/8/2020 3:02 PM, Ken Brown via Cygwin-patches wrote:

fhandler_process::exists is called when we are checking a path
starting with "/proc//fd".  If it returns virt_none and sets an
errno, there is no need for further checking.  Just set 'error' and
return.
---
  winsup/cygwin/path.cc | 9 +
  1 file changed, 9 insertions(+)

diff --git a/winsup/cygwin/path.cc b/winsup/cygwin/path.cc
index 95faf8ca7..1d0c38a20 100644
--- a/winsup/cygwin/path.cc
+++ b/winsup/cygwin/path.cc
@@ -809,6 +809,15 @@ path_conv::check (const char *src, unsigned opt,
  delete fh;
  goto retry_fs_via_processfd;
}
+ else if (file_type == virt_none && dev == FH_PROCESSFD)
+   {
+ error = get_errno ();
+ if (error)
+   {
+ delete fh;
+ return;
+   }
+   }
  delete fh;
}
  switch (file_type)



The subject should say "2/2", not "2/3".  I have a local third patch documenting 
the bug fix, which I didn't bother to send.


Ken

[PATCH v2 2/3] Cygwin: path_conv::check: handle error from fhandler_process::exists

2020-09-08 Thread Ken Brown via Cygwin-patches

fhandler_process::exists is called when we are checking a path
starting with "/proc//fd".  If it returns virt_none and sets an
errno, there is no need for further checking.  Just set 'error' and
return.
---
 winsup/cygwin/path.cc | 9 +
 1 file changed, 9 insertions(+)

diff --git a/winsup/cygwin/path.cc b/winsup/cygwin/path.cc
index 95faf8ca7..1d0c38a20 100644
--- a/winsup/cygwin/path.cc
+++ b/winsup/cygwin/path.cc
@@ -809,6 +809,15 @@ path_conv::check (const char *src, unsigned opt,
  delete fh;
  goto retry_fs_via_processfd;
}
+ else if (file_type == virt_none && dev == FH_PROCESSFD)
+   {
+ error = get_errno ();
+ if (error)
+   {
+ delete fh;
+ return;
+   }
+   }
  delete fh;
}
  switch (file_type)
-- 
2.28.0

Re: [PATCH v2 0/6] Some AF_UNIX fixes

2020-10-08 Thread Ken Brown via Cygwin-patches


On 10/4/2020 12:49 PM, Ken Brown via Cygwin-patches wrote:

I'm about to push these.  Corinna, please check them when you return.
The only difference between v2 and v1 is that there are a few more
fixes.

I'm trying to help get the AF_UNIX development going again.  I'm
mostly working on the topic/af_unix branch.  But when I find bugs that
exist on master, I'll push those to master and then merge master to
topic/af_unix.


FYI to Corinna and anyone else interested in AF_UNIX development.  After pushing 
a few patches to the topic/af_unix branch I did some cleanup (locally) and 
merged master into the topic branch.  I don't want to do a forced push and risk 
messing up the branch, so I've created a new branch, topic/af_unix_new, and will 
do all further work there until Corinna returns and decides how we should proceed.


Ken

[PATCH v2 0/6] Some AF_UNIX fixes

2020-10-04 Thread Ken Brown via Cygwin-patches

I'm about to push these.  Corinna, please check them when you return.
The only difference between v2 and v1 is that there are a few more
fixes.

I'm trying to help get the AF_UNIX development going again.  I'm
mostly working on the topic/af_unix branch.  But when I find bugs that
exist on master, I'll push those to master and then merge master to
topic/af_unix.

So far what I have on that branch locally (to be pushed shortly) is an
implementation of fhandler_socket_unix::read, which I've tested by
running the srver/client programs from Section 57.2 of Kerrisk's book,
"The Linux Programming Interface".

Ken Brown (6):
  Cygwin: AF_UNIX: use FILE_OPEN_REPARSE_POINT when needed
  Cygwin: fix handling of known reparse points that are not symlinks
  Cygwin: always recognize AF_UNIX sockets as reparse points
  Cygwin: AF_UNIX: socket: set the O_RDWR flag
  Cygwin: AF_UNIX: listen_pipe: check for STATUS_SUCCESS
  Cygwin: AF_UNIX: open_pipe: call recv_peer_info

 winsup/cygwin/fhandler.cc | 11 --
 winsup/cygwin/fhandler_socket_unix.cc | 31 +--
 winsup/cygwin/path.cc | 27 +++
 winsup/cygwin/security.cc |  8 +--
 4 files changed, 53 insertions(+), 24 deletions(-)

-- 
2.28.0

[PATCH v2 3/6] Cygwin: always recognize AF_UNIX sockets as reparse points

2020-10-04 Thread Ken Brown via Cygwin-patches

If __WITH_AF_UNIX is defined when Cygwin is built, then a named
AF_UNIX socket is represented by a reparse point with a
Cygwin-specific tag and GUID.  Make such files recognizable as reparse
points (but not as sockets) even if __WITH_AF_UNIX is not defined.
That way utilities such as 'ls' and 'rm' still behave reasonably.

This requires two changes:

- Define the GUID __cygwin_socket_guid unconditionally.

- Make check_reparse_point_target return PATH_REP on a reparse point
  of this type if __WITH_AF_UNIX is not defined.
---
 winsup/cygwin/fhandler_socket_unix.cc | 17 +
 winsup/cygwin/path.cc | 10 ++
 2 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/winsup/cygwin/fhandler_socket_unix.cc 
b/winsup/cygwin/fhandler_socket_unix.cc
index d7bb1090e..429aa8a90 100644
--- a/winsup/cygwin/fhandler_socket_unix.cc
+++ b/winsup/cygwin/fhandler_socket_unix.cc
@@ -8,9 +8,17 @@
Cygwin license.  Please consult the file "CYGWIN_LICENSE" for
details. */
 
+#include "winsup.h"
+
+GUID __cygwin_socket_guid = {
+  .Data1 = 0xefc1714d,
+  .Data2 = 0x7b19,
+  .Data3 = 0x4407,
+  .Data4 = { 0xba, 0xb3, 0xc5, 0xb1, 0xf9, 0x2c, 0xb8, 0x8c }
+};
+
 #ifdef __WITH_AF_UNIX
 
-#include "winsup.h"
 #include 
 #include 
 #include 
@@ -124,13 +132,6 @@ class af_unix_pkt_hdr_t
   (void *)(((PBYTE)(_p)) + AF_UNIX_PKT_OFFSETOF_DATA (_p)); \
})
 
-GUID __cygwin_socket_guid = {
-  .Data1 = 0xefc1714d,
-  .Data2 = 0x7b19,
-  .Data3 = 0x4407,
-  .Data4 = { 0xba, 0xb3, 0xc5, 0xb1, 0xf9, 0x2c, 0xb8, 0x8c }
-};
-
 /* Some error conditions on pipes have multiple status codes, unfortunately. */
 #define STATUS_PIPE_NO_INSTANCE_AVAILABLE(status)  \
({ NTSTATUS _s = (status); \
diff --git a/winsup/cygwin/path.cc b/winsup/cygwin/path.cc
index 2e3208d2d..4f5f03a76 100644
--- a/winsup/cygwin/path.cc
+++ b/winsup/cygwin/path.cc
@@ -2476,8 +2476,7 @@ check_reparse_point_string (PUNICODE_STRING subst)
 /* Return values:
 <0: Negative errno.
  0: Not a reparse point recognized by us.
->0: PATH_SYMLINK | PATH_REP for symlink or directory mount point,
-PATH_SOCKET | PATH_REP for AF_UNIX socket.
+>0: Path flags for a recognized reparse point, always including PATH_REP.
 */
 int
 check_reparse_point_target (HANDLE h, bool remote, PREPARSE_DATA_BUFFER rp,
@@ -2618,15 +2617,18 @@ check_reparse_point_target (HANDLE h, bool remote, 
PREPARSE_DATA_BUFFER rp,
}
   return -EIO;
 }
-#ifdef __WITH_AF_UNIX
   else if (rp->ReparseTag == IO_REPARSE_TAG_CYGUNIX)
 {
   PREPARSE_GUID_DATA_BUFFER rgp = (PREPARSE_GUID_DATA_BUFFER) rp;
 
   if (memcmp (CYGWIN_SOCKET_GUID, >ReparseGuid, sizeof (GUID)) == 0)
+#ifdef __WITH_AF_UNIX
return PATH_SOCKET | PATH_REP;
+#else
+/* Recognize this as a reparse point but not as a socket.  */
+return PATH_REP;
+#endif
 }
-#endif /* __WITH_AF_UNIX */
   return 0;
 }
 
-- 
2.28.0

[PATCH v2 2/6] Cygwin: fix handling of known reparse points that are not symlinks

2020-10-04 Thread Ken Brown via Cygwin-patches

Commit aa467e6e, "Cygwin: add AF_UNIX reparse points to path
handling", changed check_reparse_point_target so that it could return
a positive value on a known reparse point that is not a symlink.  But
some of the code in check_reparse_point that handles this positive
return value was executed unconditionally, when it should have been
executed only for symlinks.

As a result, posixify could be called on a buffer containing garbage,
and check_reparse_point could erroneously return a positive value on a
non-symlink.  This is now fixed so that posixify is only called if the
reparse point is a symlink, and check_reparse_point returns 0 if the
reparse point is not a symlink.

Also fix symlink_info::check to handle this last case, in which
check_reparse_point returns 0 on a known reparse point.
---
 winsup/cygwin/path.cc | 17 -
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/winsup/cygwin/path.cc b/winsup/cygwin/path.cc
index 638f1adce..2e3208d2d 100644
--- a/winsup/cygwin/path.cc
+++ b/winsup/cygwin/path.cc
@@ -2655,11 +2655,15 @@ symlink_info::check_reparse_point (HANDLE h, bool 
remote)
   /* ret is > 0, so it's a known reparse point, path in symbuf. */
   path_flags |= ret;
   if (ret & PATH_SYMLINK)
-sys_wcstombs (srcbuf, SYMLINK_MAX + 7, symbuf.Buffer,
- symbuf.Length / sizeof (WCHAR));
-  /* A symlink is never a directory. */
-  fileattr &= ~FILE_ATTRIBUTE_DIRECTORY;
-  return posixify (srcbuf);
+{
+  sys_wcstombs (srcbuf, SYMLINK_MAX + 7, symbuf.Buffer,
+   symbuf.Length / sizeof (WCHAR));
+  /* A symlink is never a directory. */
+  fileattr &= ~FILE_ATTRIBUTE_DIRECTORY;
+  return posixify (srcbuf);
+}
+  else
+return 0;
 }
 
 int
@@ -3274,6 +3278,9 @@ restart:
&= ~FILE_ATTRIBUTE_DIRECTORY;
  break;
}
+ else if (res == 0 && (path_flags & PATH_REP))
+   /* Known reparse point but not a symlink. */
+   goto file_not_symlink;
  else
{
  /* Volume moint point or unrecognized reparse point type.
-- 
2.28.0

[PATCH v2 1/6] Cygwin: AF_UNIX: use FILE_OPEN_REPARSE_POINT when needed

2020-10-04 Thread Ken Brown via Cygwin-patches

The following Windows system calls currently fail with
STATUS_IO_REPARSE_TAG_NOT_HANDLED when called on an AF_UNIX socket:

- NtOpenFile in get_file_sd

- NtOpenFile in set_file_sd

- NtCreateFile in fhandler_base::open

Fix this by adding the FILE_OPEN_REPARSE_POINT flag to those calls
when the file is a known reparse point.
---
 winsup/cygwin/fhandler.cc | 11 +--
 winsup/cygwin/security.cc |  8 ++--
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/winsup/cygwin/fhandler.cc b/winsup/cygwin/fhandler.cc
index 82b21aff4..5dbbd4068 100644
--- a/winsup/cygwin/fhandler.cc
+++ b/winsup/cygwin/fhandler.cc
@@ -620,13 +620,20 @@ fhandler_base::open (int flags, mode_t mode)
   else
 create_disposition = (flags & O_CREAT) ? FILE_OPEN_IF : FILE_OPEN;
 
-  if (get_device () == FH_FS)
+  if (get_device () == FH_FS
+#ifdef __WITH_AF_UNIX
+  || get_device () == FH_UNIX
+#endif
+  )
 {
-  /* Add the reparse point flag to known repares points, otherwise we
+  /* Add the reparse point flag to known reparse points, otherwise we
 open the target, not the reparse point.  This would break lstat. */
   if (pc.is_known_reparse_point ())
options |= FILE_OPEN_REPARSE_POINT;
+}
 
+  if (get_device () == FH_FS)
+{
   /* O_TMPFILE files are created with delete-on-close semantics, as well
 as with FILE_ATTRIBUTE_TEMPORARY.  The latter speeds up file access,
 because the OS tries to keep the file in memory as much as possible.
diff --git a/winsup/cygwin/security.cc b/winsup/cygwin/security.cc
index 468b05164..d48526619 100644
--- a/winsup/cygwin/security.cc
+++ b/winsup/cygwin/security.cc
@@ -65,7 +65,9 @@ get_file_sd (HANDLE fh, path_conv , security_descriptor 
,
   fh ? pc.init_reopen_attr (attr, fh)
  : pc.get_object_attr (attr, sec_none_nih),
   , FILE_SHARE_VALID_FLAGS,
-  FILE_OPEN_FOR_BACKUP_INTENT);
+  FILE_OPEN_FOR_BACKUP_INTENT
+  | pc.is_known_reparse_point ()
+  ? FILE_OPEN_REPARSE_POINT : 0);
   if (!NT_SUCCESS (status))
{
  sd.free ();
@@ -232,7 +234,9 @@ set_file_sd (HANDLE fh, path_conv , security_descriptor 
, bool is_chown)
  : pc.get_object_attr (attr, sec_none_nih),
   ,
   FILE_SHARE_VALID_FLAGS,
-  FILE_OPEN_FOR_BACKUP_INTENT);
+  FILE_OPEN_FOR_BACKUP_INTENT
+  | pc.is_known_reparse_point ()
+  ? FILE_OPEN_REPARSE_POINT : 0);
  if (!NT_SUCCESS (status))
{
  fh = NULL;
-- 
2.28.0

[PATCH v2 5/6] Cygwin: AF_UNIX: listen_pipe: check for STATUS_SUCCESS

2020-10-04 Thread Ken Brown via Cygwin-patches

A successful connection can be indicated by STATUS_SUCCESS or
STATUS_PIPE_CONNECTED.  Previously we were checking only for the
latter.
---
 winsup/cygwin/fhandler_socket_unix.cc | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/winsup/cygwin/fhandler_socket_unix.cc 
b/winsup/cygwin/fhandler_socket_unix.cc
index 0ae7fe125..1a9532fe5 100644
--- a/winsup/cygwin/fhandler_socket_unix.cc
+++ b/winsup/cygwin/fhandler_socket_unix.cc
@@ -1064,6 +1064,7 @@ fhandler_socket_unix::listen_pipe ()
   IO_STATUS_BLOCK io;
   HANDLE evt = NULL;
   DWORD waitret = WAIT_OBJECT_0;
+  int ret = -1;
 
   io.Status = STATUS_PENDING;
   if (!is_nonblocking () && !(evt = create_event ()))
@@ -1085,9 +1086,11 @@ fhandler_socket_unix::listen_pipe ()
 set_errno (EINTR);
   else if (status == STATUS_PIPE_LISTENING)
 set_errno (EAGAIN);
-  else if (status != STATUS_PIPE_CONNECTED)
+  else if (status == STATUS_SUCCESS || status == STATUS_PIPE_CONNECTED)
+ret = 0;
+  else
 __seterrno_from_nt_status (status);
-  return (status == STATUS_PIPE_CONNECTED) ? 0 : -1;
+  return ret;
 }
 
 ULONG
-- 
2.28.0

[PATCH v2 4/6] Cygwin: AF_UNIX: socket: set the O_RDWR flag

2020-10-04 Thread Ken Brown via Cygwin-patches

---
 winsup/cygwin/fhandler_socket_unix.cc | 1 +
 1 file changed, 1 insertion(+)

diff --git a/winsup/cygwin/fhandler_socket_unix.cc 
b/winsup/cygwin/fhandler_socket_unix.cc
index 429aa8a90..0ae7fe125 100644
--- a/winsup/cygwin/fhandler_socket_unix.cc
+++ b/winsup/cygwin/fhandler_socket_unix.cc
@@ -1366,6 +1366,7 @@ fhandler_socket_unix::socket (int af, int type, int 
protocol, int flags)
   wmem (262144);
   set_addr_family (AF_UNIX);
   set_socket_type (type);
+  set_flags (O_RDWR | O_BINARY);
   if (flags & SOCK_NONBLOCK)
 set_nonblocking (true);
   if (flags & SOCK_CLOEXEC)
-- 
2.28.0

[PATCH v2 6/6] Cygwin: AF_UNIX: open_pipe: call recv_peer_info

2020-10-04 Thread Ken Brown via Cygwin-patches

If open_pipe is called with xchg_sock_info true, call recv_peer_info
in addition to send_sock_info.
---
 winsup/cygwin/fhandler_socket_unix.cc | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/winsup/cygwin/fhandler_socket_unix.cc 
b/winsup/cygwin/fhandler_socket_unix.cc
index 1a9532fe5..9f7f86c47 100644
--- a/winsup/cygwin/fhandler_socket_unix.cc
+++ b/winsup/cygwin/fhandler_socket_unix.cc
@@ -941,7 +941,11 @@ fhandler_socket_unix::open_pipe (PUNICODE_STRING 
pipe_name, bool xchg_sock_info)
 {
   set_handle (ph);
   if (xchg_sock_info)
-   send_sock_info (false);
+   {
+ /* FIXME: Should we check for errors? */
+ send_sock_info (false);
+ recv_peer_info ();
+   }
 }
   return status;
 }
-- 
2.28.0

[PATCH 1/1] Cygwin: AF_INET and AF_LOCAL: recv_internal: fix MSG_WAITALL support

2020-10-12 Thread Ken Brown via Cygwin-patches

If MSG_WAITALL is set, recv_internal calls WSARecv or WSARecvFrom in a
loop, in an effort to fill all the scatter-gather buffers.  The test
for whether all the buffers are full was previously incorrect.
---
 winsup/cygwin/fhandler_socket_inet.cc  | 2 +-
 winsup/cygwin/fhandler_socket_local.cc | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/winsup/cygwin/fhandler_socket_inet.cc 
b/winsup/cygwin/fhandler_socket_inet.cc
index 71e92e341..bc08d3cf1 100644
--- a/winsup/cygwin/fhandler_socket_inet.cc
+++ b/winsup/cygwin/fhandler_socket_inet.cc
@@ -1208,7 +1208,7 @@ fhandler_socket_inet::recv_internal (LPWSAMSG wsamsg, 
bool use_recvmsg)
  --wsacnt;
}
}
- if (!wret)
+ if (!wsacnt)
break;
}
   else if (WSAGetLastError () != WSAEWOULDBLOCK)
diff --git a/winsup/cygwin/fhandler_socket_local.cc 
b/winsup/cygwin/fhandler_socket_local.cc
index 8bfba225a..c94bf828f 100644
--- a/winsup/cygwin/fhandler_socket_local.cc
+++ b/winsup/cygwin/fhandler_socket_local.cc
@@ -1212,7 +1212,7 @@ fhandler_socket_local::recv_internal (LPWSAMSG wsamsg, 
bool use_recvmsg)
  --wsacnt;
}
}
- if (!wret)
+ if (!wsacnt)
break;
}
   else if (WSAGetLastError () != WSAEWOULDBLOCK)
-- 
2.28.0

[PATCH 0/1] Fix MSG_WAITALL support

2020-10-12 Thread Ken Brown via Cygwin-patches

It looks to me like there's been a bug in the MSG_WAITALL support for
AF_INET and AF_LOCAL sockets ever since that support was first
introduced 13 years ago in commit 023a2fa7.  If I'm right, MSG_WAITALL
has never worked.

This patch fixes it.  I'll push it in a few days if no one sees
anything wrong with it.

In a followup email I'll show how I tested it.

Ken Brown (1):
  Cygwin: AF_INET and AF_LOCAL: recv_internal: fix MSG_WAITALL support

 winsup/cygwin/fhandler_socket_inet.cc  | 2 +-
 winsup/cygwin/fhandler_socket_local.cc | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

-- 
2.28.0

Re: [PATCH 0/1] Fix MSG_WAITALL support

2020-10-12 Thread Ken Brown via Cygwin-patches


On 10/12/2020 2:02 PM, Ken Brown via Cygwin-patches wrote:

It looks to me like there's been a bug in the MSG_WAITALL support for
AF_INET and AF_LOCAL sockets ever since that support was first
introduced 13 years ago in commit 023a2fa7.  If I'm right, MSG_WAITALL
has never worked.

This patch fixes it.  I'll push it in a few days if no one sees
anything wrong with it.

In a followup email I'll show how I tested it.


Attached are slight variants of the server/client programs from Section 57.2 of 
Kerrisk's book, "The Linux Programming Interface".  The only essential 
difference is that I've changed the server program to (a) use a small buffer 
(size 10 instead of 100) and (b) use 'recv' with the MSG_WAITALL flag instead of 
'read'.  The 'recv' call shouldn't return until it reads 10 bytes.


To test, run waitall_sv in one terminal and waitall_cl in a second.  Type 
something in the second terminal (followed by RET), and it should be echoed in 
the first.  But because of the MSG_WAITALL flag, the echoing shouldn't occur 
until 10 bytes have been written.  For example, if I type "abcd" in the 
second terminal and then do it again, I should see the following:


# Terminal 2:
$ ./waitall_cl
abcd
abcd

# Terminal 1:
$ ./waitall_sv
abcd
abcd

Here the echoing in Terminal 1 shouldn't occur until I've typed both "abcd" 
lines in Terminal 2.


[Note that there is a newline character after each "abcd", so "abcd" is 5 
bytes long, and the two lines together are 10 bytes long.]


Before I apply my patch, each line typed in Terminal 2 is immediately echoed in 
Terminal 1.  After I apply the patch, the echoing doesn't occur until I've typed 
both lines.


Ken
#include 
#include 
#include 
#include 
#include 
#include 

#define SV_SOCK_PATH "/tmp/waitall"

#define BUF_SIZE 10

#define BACKLOG 5

int
main ()
{
struct sockaddr_un addr;
int sfd, cfd;
ssize_t nread;
char buf[BUF_SIZE];

if ((sfd = socket (AF_UNIX, SOCK_STREAM, 0)) < 0)
  {
perror ("socket");
exit (1);
  }
if (remove (SV_SOCK_PATH) < 0 && errno != ENOENT)
  {
perror ("remove");
exit (1);
  }

memset (, 0, sizeof (struct sockaddr_un));
addr.sun_family = AF_UNIX;
strncpy (addr.sun_path, SV_SOCK_PATH, sizeof (addr.sun_path) - 1);

if (bind (sfd, (struct sockaddr *) , sizeof (struct sockaddr_un)) < 0)
  {
perror ("bind");
exit (1);
  }

if (listen (sfd, BACKLOG) < 0)
  {
perror ("listen");
exit (1);
  }

while (1)
  {
cfd = accept (sfd, NULL, NULL);
if (cfd < 0)
  {
perror ("accept");
exit (1);
  }

/* Transfer data from connected socket to stdout until EOF. */
while ((nread = recv (cfd, buf, BUF_SIZE, MSG_WAITALL)) > 0)
  if (write (STDOUT_FILENO, buf, nread) != nread)
{
  perror ("partial/failed write");
  exit (1);
}

if (nread < 0)
  {
perror ("read");
exit (1);
  }

if (close (cfd) < 0)
  {
perror ("close");
exit (1);
  }
  }
}
#include 
#include 
#include 
#include 
#include 
#include 

#define SV_SOCK_PATH "/tmp/waitall"

#define BUF_SIZE 100

int
main ()
{
struct sockaddr_un addr;
int sfd;
ssize_t nread;
char buf[BUF_SIZE];

if ((sfd = socket (AF_UNIX, SOCK_STREAM, 0)) < 0)
  {
perror ("socket");
exit (1);
  }

memset (, 0, sizeof(struct sockaddr_un));
addr.sun_family = AF_UNIX;
strncpy (addr.sun_path, SV_SOCK_PATH, sizeof(addr.sun_path) - 1);

if (connect (sfd, (struct sockaddr *) ,
sizeof (struct sockaddr_un)) < 0)
  {
perror ("connect");
exit (1);
  }

/* Copy stdin to socket. */
while ((nread = read (STDIN_FILENO, buf, BUF_SIZE)) > 0)
  if (write (sfd, buf, nread) != nread)
{
  perror ("partial/failed write");
  exit (1);
}

if (nread < 0)
  {
perror ("read");
exit (1);
  }
}

Re: [PATCH v2 5/6] Cygwin: AF_UNIX: listen_pipe: check for STATUS_SUCCESS

2020-10-13 Thread Ken Brown via Cygwin-patches

On 10/13/2020 7:28 AM, Corinna Vinschen wrote:
> On Oct  4 12:49, Ken Brown via Cygwin-patches wrote:
>> A successful connection can be indicated by STATUS_SUCCESS or
>> STATUS_PIPE_CONNECTED.
> 
> THanks for catching but... huh?  How does Windows generate two different
> status codes for the same result from the same function?

I think (but I'd have to recheck) that if the pipe is already connected when 
NtFsControlFile is called, then the latter returns STATUS_PIPE_CONNECTED.  But 
if you first get STATUS_PENDING and then set status = io.Status after 
completion, then the result is STATUS_SUCCESS.

I might not have that exactly right.  All I remember for sure is that I was 
debugging a listen_pipe failure, and it turned out to be due to STATUS_SUCCESS 
being treated as an error condition.

Ken

Re: [PATCH v2 0/6] Some AF_UNIX fixes

2020-10-14 Thread Ken Brown via Cygwin-patches


On 10/13/2020 7:49 AM, Corinna Vinschen wrote:

On Oct  8 17:36, Ken Brown via Cygwin-patches wrote:

On 10/4/2020 12:49 PM, Ken Brown via Cygwin-patches wrote:

I'm about to push these.  Corinna, please check them when you return.
The only difference between v2 and v1 is that there are a few more
fixes.

I'm trying to help get the AF_UNIX development going again.  I'm
mostly working on the topic/af_unix branch.  But when I find bugs that
exist on master, I'll push those to master and then merge master to
topic/af_unix.


FYI to Corinna and anyone else interested in AF_UNIX development.  After
pushing a few patches to the topic/af_unix branch I did some cleanup
(locally) and merged master into the topic branch.  I don't want to do a
forced push and risk messing up the branch, so I've created a new branch,
topic/af_unix_new, and will do all further work there until Corinna returns
and decides how we should proceed.


No, that's ok, just force push.


OK, I've done that now.  The branch contains a few sendmsg fixes, a first cut of 
a recvmsg implementation, and a merge from master.  I've done some testing of 
recvmsg, but many things are not yet tested.  I'll work on continued testing next.


Are you aware of any test suite that I could run?  I've been using examples from 
Kerrisk's book, because that's what I read to learn the basics of sockets.  But 
those are just examples and are not meant to be comprehensive.


Ken

[PATCH] Cygwin: main exception handler (64-bit): continue GCC exceptions

2020-08-17 Thread Ken Brown via Cygwin-patches

This is necessary in order to be consistent with the following comment
in the definition of _Unwind_RaiseException() in the GCC source file
libgcc/unwind-seh.c:

 The exception handler installed in crt0 will continue any GCC
 exception that reaches there (and isn't marked non-continuable).

Previously we failed to do this and, as a consequence, the C++ runtime
didn't call std::terminate after an unhandled exception.

This fixes the problem reported here:

  https://sourceware.org/pipermail/cygwin/2020-August/245897.html
---
 winsup/cygwin/exceptions.cc | 21 +
 1 file changed, 21 insertions(+)

diff --git a/winsup/cygwin/exceptions.cc b/winsup/cygwin/exceptions.cc
index 0d9e8f70e..f8da7529b 100644
--- a/winsup/cygwin/exceptions.cc
+++ b/winsup/cygwin/exceptions.cc
@@ -647,6 +647,17 @@ exception::handle (EXCEPTION_RECORD *e, exception_list 
*frame, CONTEXT *in,
   if (exit_state || e->ExceptionFlags)
 return ExceptionContinueSearch;
 
+/* From the GCC source file libgcc/unwind-seh.c. */
+#ifdef __x86_64__
+#define STATUS_USER_DEFINED(1U << 29)
+#define GCC_MAGIC  (('G' << 16) | ('C' << 8) | 'C')
+#define GCC_EXCEPTION(TYPE)\
+   (STATUS_USER_DEFINED | ((TYPE) << 24) | GCC_MAGIC)
+#define STATUS_GCC_THROW   GCC_EXCEPTION (0)
+#define STATUS_GCC_UNWIND  GCC_EXCEPTION (1)
+#define STATUS_GCC_FORCED  GCC_EXCEPTION (2)
+#endif
+
   siginfo_t si = {};
   si.si_code = SI_KERNEL;
   /* Coerce win32 value to posix value.  */
@@ -762,6 +773,16 @@ exception::handle (EXCEPTION_RECORD *e, exception_list 
*frame, CONTEXT *in,
 handling.  */
   return ExceptionContinueExecution;
 
+#ifdef __x86_64__
+  /* According to a comment in the GCC function
+_Unwind_RaiseException(), GCC expects us to continue all the
+(continuable) GCC exceptions that reach us. */
+case STATUS_GCC_THROW:
+case STATUS_GCC_UNWIND:
+case STATUS_GCC_FORCED:
+  return ExceptionContinueExecution;
+#endif
+
 default:
   /* If we don't recognize the exception, we have to assume that
 we are doing structured exception handling, and we let
-- 
2.28.0

[PATCH] Cygwin: sigproc.cc: add comment

2020-08-29 Thread Ken Brown via Cygwin-patches

---
 winsup/cygwin/sigproc.cc | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/winsup/cygwin/sigproc.cc b/winsup/cygwin/sigproc.cc
index 2a9734f00..47352c213 100644
--- a/winsup/cygwin/sigproc.cc
+++ b/winsup/cygwin/sigproc.cc
@@ -44,7 +44,11 @@ char NO_COPY myself_nowait_dummy[1] = {'0'};
 
 #define Static static NO_COPY
 
-/* All my children info.  Avoid expensive constructor ops at DLL startup */
+/* All my children info.  Avoid expensive constructor ops at DLL
+   startup.
+
+   This class can allocate memory.  But there's no need to free it
+   because only one instance of the class is created per process. */
 class child_procs {
 #ifdef __i386__
 static const int _NPROCS = 256;
-- 
2.28.0

Re: [PATCH] Cygwin: strace: ignore GCC exceptions

2020-08-20 Thread Ken Brown via Cygwin-patches


On 8/20/2020 10:06 AM, Corinna Vinschen wrote:

Wouldn't it make sense to create a header defining the GCC status values
as in libgcc/unwind-seh.c and share this between exceptions.cc and
strace.cc?


Yes, thanks for the suggestion.  New patch(es) on the way.  I called the header 
"gcc_seh.h".  Feel free to suggest a better name.


Ken

[PATCH] Cygwin: strace: ignore GCC exceptions

2020-08-20 Thread Ken Brown via Cygwin-patches

Any C++ app that calls 'throw' on 64-bit Cygwin results in an
exception of type STATUS_GCC_THROW (0x20474343) generated by the C++
runtime.  Don't pollute the strace output by printing information
about this and other GCC exceptions.
---
 winsup/utils/strace.cc | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/winsup/utils/strace.cc b/winsup/utils/strace.cc
index 9b6569a70..2b317012d 100644
--- a/winsup/utils/strace.cc
+++ b/winsup/utils/strace.cc
@@ -790,6 +790,13 @@ proc_child (unsigned mask, FILE *ofile, pid_t pid)
case STATUS_BREAKPOINT:
case 0x406d1388:/* SetThreadName exception. */
  break;
+#ifdef __x86_64__
+   case 0x20474343:/* STATUS_GCC_THROW. */
+   case 0x21474343:/* STATUS_GCC_UNWIND. */
+   case 0x22474343:/* STATUS_GCC_FORCED. */
+ status = DBG_EXCEPTION_NOT_HANDLED;
+ break;
+#endif
default:
  status = DBG_EXCEPTION_NOT_HANDLED;
  if (ev.u.Exception.dwFirstChance)
-- 
2.28.0

[PATCH 1/2] Cygwin: add header defining GCC exception codes

2020-08-20 Thread Ken Brown via Cygwin-patches

Include it in exceptions.cc instead of defining the exception codes
there.
---
 winsup/cygwin/exceptions.cc | 10 +-
 winsup/cygwin/gcc_seh.h | 19 +++
 2 files changed, 20 insertions(+), 9 deletions(-)
 create mode 100644 winsup/cygwin/gcc_seh.h

diff --git a/winsup/cygwin/exceptions.cc b/winsup/cygwin/exceptions.cc
index 61406e5d1..bb7704f94 100644
--- a/winsup/cygwin/exceptions.cc
+++ b/winsup/cygwin/exceptions.cc
@@ -28,6 +28,7 @@ details. */
 #include "ntdll.h"
 #include "exception.h"
 #include "posix_timer.h"
+#include "gcc_seh.h"
 
 /* Definitions for code simplification */
 #ifdef __x86_64__
@@ -763,15 +764,6 @@ exception::handle (EXCEPTION_RECORD *e, exception_list 
*frame, CONTEXT *in,
   return ExceptionContinueExecution;
 
 #ifdef __x86_64__
-/* From the GCC source file libgcc/unwind-seh.c. */
-#define STATUS_USER_DEFINED(1U << 29)
-#define GCC_MAGIC  (('G' << 16) | ('C' << 8) | 'C')
-#define GCC_EXCEPTION(TYPE)\
-   (STATUS_USER_DEFINED | ((TYPE) << 24) | GCC_MAGIC)
-#define STATUS_GCC_THROW   GCC_EXCEPTION (0)
-#define STATUS_GCC_UNWIND  GCC_EXCEPTION (1)
-#define STATUS_GCC_FORCED  GCC_EXCEPTION (2)
-
 case STATUS_GCC_THROW:
 case STATUS_GCC_UNWIND:
 case STATUS_GCC_FORCED:
diff --git a/winsup/cygwin/gcc_seh.h b/winsup/cygwin/gcc_seh.h
new file mode 100644
index 0..fb779ef73
--- /dev/null
+++ b/winsup/cygwin/gcc_seh.h
@@ -0,0 +1,19 @@
+/* gcc_seh.h: GCC exception codes.
+
+This software is a copyrighted work licensed under the terms of the
+Cygwin license.  Please consult the file "CYGWIN_LICENSE" for
+details. */
+
+#pragma once
+
+/* From the GCC source file libgcc/unwind-seh.c. */
+
+#ifdef __x86_64__
+#define STATUS_USER_DEFINED(1U << 29)
+#define GCC_MAGIC  (('G' << 16) | ('C' << 8) | 'C')
+#define GCC_EXCEPTION(TYPE)\
+   (STATUS_USER_DEFINED | ((TYPE) << 24) | GCC_MAGIC)
+#define STATUS_GCC_THROW   GCC_EXCEPTION (0)
+#define STATUS_GCC_UNWIND  GCC_EXCEPTION (1)
+#define STATUS_GCC_FORCED  GCC_EXCEPTION (2)
+#endif
-- 
2.28.0

[PATCH 0/2] GCC exception codes

2020-08-20 Thread Ken Brown via Cygwin-patches

Ken Brown (2):
  Cygwin: add header defining GCC exception codes
  Cygwin: strace: ignore GCC exceptions

 winsup/cygwin/exceptions.cc | 10 +-
 winsup/cygwin/gcc_seh.h | 19 +++
 winsup/utils/strace.cc  |  8 
 3 files changed, 28 insertions(+), 9 deletions(-)
 create mode 100644 winsup/cygwin/gcc_seh.h

-- 
2.28.0

[PATCH 2/2] Cygwin: strace: ignore GCC exceptions

2020-08-20 Thread Ken Brown via Cygwin-patches

Any C++ app that calls 'throw' on 64-bit Cygwin results in an
exception of type STATUS_GCC_THROW (0x20474343) generated by the C++
runtime.  Don't pollute the strace output by printing information
about this and other GCC exceptions.
---
 winsup/utils/strace.cc | 8 
 1 file changed, 8 insertions(+)

diff --git a/winsup/utils/strace.cc b/winsup/utils/strace.cc
index 9b6569a70..b96ad40c1 100644
--- a/winsup/utils/strace.cc
+++ b/winsup/utils/strace.cc
@@ -25,6 +25,7 @@ details. */
 #include "../cygwin/include/sys/cygwin.h"
 #include "../cygwin/include/cygwin/version.h"
 #include "../cygwin/cygtls_padsize.h"
+#include "../cygwin/gcc_seh.h"
 #include "path.h"
 #undef cygwin_internal
 #include "loadlib.h"
@@ -790,6 +791,13 @@ proc_child (unsigned mask, FILE *ofile, pid_t pid)
case STATUS_BREAKPOINT:
case 0x406d1388:/* SetThreadName exception. */
  break;
+#ifdef __x86_64__
+   case STATUS_GCC_THROW:
+   case STATUS_GCC_UNWIND:
+   case STATUS_GCC_FORCED:
+ status = DBG_EXCEPTION_NOT_HANDLED;
+ break;
+#endif
default:
  status = DBG_EXCEPTION_NOT_HANDLED;
  if (ev.u.Exception.dwFirstChance)
-- 
2.28.0

[PATCH] Cygwin: fhandler_fifo::delete_client_handler: improve efficiency

2020-08-26 Thread Ken Brown via Cygwin-patches

Delete a client handler by swapping it with the last one in the list
instead of calling memmove.
---
 winsup/cygwin/fhandler_fifo.cc | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index b3c4c4a25..75c8406fe 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -377,14 +377,14 @@ fhandler_fifo::add_client_handler (bool new_pipe_instance)
   return 0;
 }
 
-/* Always called with fifo_client_lock in place. */
+/* Always called with fifo_client_lock in place.  Delete a
+   client_handler by swapping it with the last one in the list. */
 void
 fhandler_fifo::delete_client_handler (int i)
 {
   fc_handler[i].close ();
   if (i < --nhandlers)
-memmove (fc_handler + i, fc_handler + i + 1,
-(nhandlers - i) * sizeof (fc_handler[i]));
+fc_handler[i] = fc_handler[nhandlers];
 }
 
 /* Delete handlers that we will never read from.  Always called with
-- 
2.28.0

[PATCH] Cygwin: sigproc.cc: fix typo in comment describing nprocs

2020-08-27 Thread Ken Brown via Cygwin-patches

nprocs is the number of children, not the number of deceased children.
The incorrect comment used to apply to a variable nzombies.  The
latter was removed in commit 8cb359d9 in 2004, but the comment was
never updated.
---
 winsup/cygwin/sigproc.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/winsup/cygwin/sigproc.cc b/winsup/cygwin/sigproc.cc
index a5cf73bde..30c799f8c 100644
--- a/winsup/cygwin/sigproc.cc
+++ b/winsup/cygwin/sigproc.cc
@@ -44,7 +44,7 @@ char NO_COPY myself_nowait_dummy[1] = {'0'};// Flag to 
sig_send that signal goes
 #define Static static NO_COPY
 
 
-Static int nprocs; // Number of deceased children
+Static int nprocs; // Number of children
 Static char cprocs[(NPROCS + 1) * sizeof (pinfo)];// All my children info
 #define procs ((pinfo *) cprocs)   // All this just to avoid expensive
// constructor operation  at DLL startup
-- 
2.28.0

[PATCH] Cygwin: cwdstuff::get: clean up debug_printf output

2020-08-23 Thread Ken Brown via Cygwin-patches

Set errno = 0 at the beginning so that the debug_printf call at the
end doesn't report a nonzero errno left over from some other function
call.
---
 winsup/cygwin/path.cc | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/winsup/cygwin/path.cc b/winsup/cygwin/path.cc
index f3b9913bd..95faf8ca7 100644
--- a/winsup/cygwin/path.cc
+++ b/winsup/cygwin/path.cc
@@ -5022,6 +5022,8 @@ char *
 cwdstuff::get (char *buf, int need_posix, int with_chroot, unsigned ulen)
 {
   tmp_pathbuf tp;
+
+  errno = 0;
   if (ulen)
 /* nothing */;
   else if (buf == NULL)
-- 
2.28.0

[PATCH 3/3] Cygwin: always recognize AF_UNIX sockets as reparse points

2020-09-29 Thread Ken Brown via Cygwin-patches

If __WITH_AF_UNIX is defined when Cygwin is built, then a named
AF_UNIX socket is represented by a reparse point with a
Cygwin-specific tag and GUID.  Make such files recognizable as reparse
points (but not as sockets) even if __WITH_AF_UNIX is not defined.
That way utilities such as 'ls' and 'rm' still behave reasonably.

This requires two changes:

- Define the GUID __cygwin_socket_guid unconditionally.

- Make check_reparse_point_target return PATH_REP on a reparse point
  of this type if __WITH_AF_UNIX is not defined.
---
 winsup/cygwin/fhandler_socket_unix.cc | 17 +
 winsup/cygwin/path.cc | 10 ++
 2 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/winsup/cygwin/fhandler_socket_unix.cc 
b/winsup/cygwin/fhandler_socket_unix.cc
index d7bb1090e..429aa8a90 100644
--- a/winsup/cygwin/fhandler_socket_unix.cc
+++ b/winsup/cygwin/fhandler_socket_unix.cc
@@ -8,9 +8,17 @@
Cygwin license.  Please consult the file "CYGWIN_LICENSE" for
details. */
 
+#include "winsup.h"
+
+GUID __cygwin_socket_guid = {
+  .Data1 = 0xefc1714d,
+  .Data2 = 0x7b19,
+  .Data3 = 0x4407,
+  .Data4 = { 0xba, 0xb3, 0xc5, 0xb1, 0xf9, 0x2c, 0xb8, 0x8c }
+};
+
 #ifdef __WITH_AF_UNIX
 
-#include "winsup.h"
 #include 
 #include 
 #include 
@@ -124,13 +132,6 @@ class af_unix_pkt_hdr_t
   (void *)(((PBYTE)(_p)) + AF_UNIX_PKT_OFFSETOF_DATA (_p)); \
})
 
-GUID __cygwin_socket_guid = {
-  .Data1 = 0xefc1714d,
-  .Data2 = 0x7b19,
-  .Data3 = 0x4407,
-  .Data4 = { 0xba, 0xb3, 0xc5, 0xb1, 0xf9, 0x2c, 0xb8, 0x8c }
-};
-
 /* Some error conditions on pipes have multiple status codes, unfortunately. */
 #define STATUS_PIPE_NO_INSTANCE_AVAILABLE(status)  \
({ NTSTATUS _s = (status); \
diff --git a/winsup/cygwin/path.cc b/winsup/cygwin/path.cc
index 2e3208d2d..4f5f03a76 100644
--- a/winsup/cygwin/path.cc
+++ b/winsup/cygwin/path.cc
@@ -2476,8 +2476,7 @@ check_reparse_point_string (PUNICODE_STRING subst)
 /* Return values:
 <0: Negative errno.
  0: Not a reparse point recognized by us.
->0: PATH_SYMLINK | PATH_REP for symlink or directory mount point,
-PATH_SOCKET | PATH_REP for AF_UNIX socket.
+>0: Path flags for a recognized reparse point, always including PATH_REP.
 */
 int
 check_reparse_point_target (HANDLE h, bool remote, PREPARSE_DATA_BUFFER rp,
@@ -2618,15 +2617,18 @@ check_reparse_point_target (HANDLE h, bool remote, 
PREPARSE_DATA_BUFFER rp,
}
   return -EIO;
 }
-#ifdef __WITH_AF_UNIX
   else if (rp->ReparseTag == IO_REPARSE_TAG_CYGUNIX)
 {
   PREPARSE_GUID_DATA_BUFFER rgp = (PREPARSE_GUID_DATA_BUFFER) rp;
 
   if (memcmp (CYGWIN_SOCKET_GUID, >ReparseGuid, sizeof (GUID)) == 0)
+#ifdef __WITH_AF_UNIX
return PATH_SOCKET | PATH_REP;
+#else
+/* Recognize this as a reparse point but not as a socket.  */
+return PATH_REP;
+#endif
 }
-#endif /* __WITH_AF_UNIX */
   return 0;
 }
 
-- 
2.28.0

[PATCH 2/3] Cygwin: fix handling of known reparse points that are not symlinks

2020-09-29 Thread Ken Brown via Cygwin-patches

Commit aa467e6e, "Cygwin: add AF_UNIX reparse points to path
handling", changed check_reparse_point_target so that it could return
a positive value on a known reparse point that is not a symlink.  But
some of the code in check_reparse_point that handles this positive
return value was executed unconditionally, when it should have been
executed only for symlinks.

As a result, posixify could be called on a buffer containing garbage,
and check_reparse_point could erroneously return a positive value on a
non-symlink.  This is now fixed so that posixify is only called if the
reparse point is a symlink, and check_reparse_point returns 0 if the
reparse point is not a symlink.

Also fix symlink_info::check to handle this last case, in which
check_reparse_point returns 0 on a known reparse point.
---
 winsup/cygwin/path.cc | 17 -
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/winsup/cygwin/path.cc b/winsup/cygwin/path.cc
index 638f1adce..2e3208d2d 100644
--- a/winsup/cygwin/path.cc
+++ b/winsup/cygwin/path.cc
@@ -2655,11 +2655,15 @@ symlink_info::check_reparse_point (HANDLE h, bool 
remote)
   /* ret is > 0, so it's a known reparse point, path in symbuf. */
   path_flags |= ret;
   if (ret & PATH_SYMLINK)
-sys_wcstombs (srcbuf, SYMLINK_MAX + 7, symbuf.Buffer,
- symbuf.Length / sizeof (WCHAR));
-  /* A symlink is never a directory. */
-  fileattr &= ~FILE_ATTRIBUTE_DIRECTORY;
-  return posixify (srcbuf);
+{
+  sys_wcstombs (srcbuf, SYMLINK_MAX + 7, symbuf.Buffer,
+   symbuf.Length / sizeof (WCHAR));
+  /* A symlink is never a directory. */
+  fileattr &= ~FILE_ATTRIBUTE_DIRECTORY;
+  return posixify (srcbuf);
+}
+  else
+return 0;
 }
 
 int
@@ -3274,6 +3278,9 @@ restart:
&= ~FILE_ATTRIBUTE_DIRECTORY;
  break;
}
+ else if (res == 0 && (path_flags & PATH_REP))
+   /* Known reparse point but not a symlink. */
+   goto file_not_symlink;
  else
{
  /* Volume moint point or unrecognized reparse point type.
-- 
2.28.0

[PATCH 1/3] Cygwin: AF_UNIX: use FILE_OPEN_REPARSE_POINT when needed

2020-09-29 Thread Ken Brown via Cygwin-patches

There are two Windows system calls that currently fail with
STATUS_IO_REPARSE_TAG_NOT_HANDLED when called on an AF_UNIX socket: a
call to NtOpenFile in get_file_sd and a call to NtCreateFile in
fhandler_base::open.

Fix this by adding the FILE_OPEN_REPARSE_POINT flag to those calls
when the file is a known reparse point.
---
 winsup/cygwin/fhandler.cc | 11 +--
 winsup/cygwin/security.cc |  4 +++-
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/winsup/cygwin/fhandler.cc b/winsup/cygwin/fhandler.cc
index 82b21aff4..5dbbd4068 100644
--- a/winsup/cygwin/fhandler.cc
+++ b/winsup/cygwin/fhandler.cc
@@ -620,13 +620,20 @@ fhandler_base::open (int flags, mode_t mode)
   else
 create_disposition = (flags & O_CREAT) ? FILE_OPEN_IF : FILE_OPEN;
 
-  if (get_device () == FH_FS)
+  if (get_device () == FH_FS
+#ifdef __WITH_AF_UNIX
+  || get_device () == FH_UNIX
+#endif
+  )
 {
-  /* Add the reparse point flag to known repares points, otherwise we
+  /* Add the reparse point flag to known reparse points, otherwise we
 open the target, not the reparse point.  This would break lstat. */
   if (pc.is_known_reparse_point ())
options |= FILE_OPEN_REPARSE_POINT;
+}
 
+  if (get_device () == FH_FS)
+{
   /* O_TMPFILE files are created with delete-on-close semantics, as well
 as with FILE_ATTRIBUTE_TEMPORARY.  The latter speeds up file access,
 because the OS tries to keep the file in memory as much as possible.
diff --git a/winsup/cygwin/security.cc b/winsup/cygwin/security.cc
index 468b05164..91fdc1e42 100644
--- a/winsup/cygwin/security.cc
+++ b/winsup/cygwin/security.cc
@@ -65,7 +65,9 @@ get_file_sd (HANDLE fh, path_conv , security_descriptor 
,
   fh ? pc.init_reopen_attr (attr, fh)
  : pc.get_object_attr (attr, sec_none_nih),
   , FILE_SHARE_VALID_FLAGS,
-  FILE_OPEN_FOR_BACKUP_INTENT);
+  FILE_OPEN_FOR_BACKUP_INTENT
+  | pc.is_known_reparse_point ()
+  ? FILE_OPEN_REPARSE_POINT : 0);
   if (!NT_SUCCESS (status))
{
  sd.free ();
-- 
2.28.0

[PATCH 0/3] Some AF_UNIX fixes

2020-09-29 Thread Ken Brown via Cygwin-patches

I'll push these in a few days if no one sees anything wrong.  Corinna,
please check them when you return.

Ken Brown (3):
  Cygwin: AF_UNIX: use FILE_OPEN_REPARSE_POINT when needed
  Cygwin: fix handling of known reparse points that are not symlinks
  Cygwin: always recognize AF_UNIX sockets as reparse points

 winsup/cygwin/fhandler.cc | 11 +--
 winsup/cygwin/fhandler_socket_unix.cc | 19 ++-
 winsup/cygwin/path.cc | 27 ++-
 winsup/cygwin/security.cc |  4 +++-
 4 files changed, 40 insertions(+), 21 deletions(-)

-- 
2.28.0

Re: [PATCH 0/3] Warning fixes for gcc 10.2

2020-09-21 Thread Ken Brown via Cygwin-patches


On 9/21/2020 3:25 PM, Jon Turney wrote:

Jon Turney (3):
   Cygwin: avoid GCC 10 error with -Werror=parentheses
   Cygwin: avoid GCC 10 error with -Werror=narrowing
   Cygwin: avoid GCC 10 error with -Werror=narrowing

  winsup/cygwin/fhandler_console.cc | 4 ++--
  winsup/cygwin/fhandler_socket_inet.cc | 2 +-
  winsup/cygwin/ntdll.h | 2 +-
  winsup/cygwin/pseudo-reloc.cc | 2 --
  4 files changed, 4 insertions(+), 6 deletions(-)


LGTM.

Ken

[PATCH] Cygwin: winlean.h: remove most of extended memory API

2020-09-23 Thread Ken Brown via Cygwin-patches

This was added as a temporary measure in commit e18f7f99 because it
wasn't yet in the mingw-w64 headers.  With one exception, it is now in
the current release of the headers (version 8.0.0), so we don't need
it in winlean.h.  The exception is that VirtualAlloc2 is only declared
conditionally in , so retain it in winlean.h.  Add
"WINAPI" to its declaration for consistency with the delaration in
memoryapi.h.

Also revert commit 3d136011, which was a related temporary workaround.
---
 winsup/cygwin/winlean.h | 44 +++--
 winsup/utils/cygpath.cc |  1 -
 winsup/utils/ps.cc  |  1 -
 3 files changed, 3 insertions(+), 43 deletions(-)

diff --git a/winsup/cygwin/winlean.h b/winsup/cygwin/winlean.h
index 2ee4aaff4..4a6a46355 100644
--- a/winsup/cygwin/winlean.h
+++ b/winsup/cygwin/winlean.h
@@ -99,47 +99,9 @@ details. */
 extern "C" {
 #endif
 
-/* Define extended memory API here as long as not available from mingw-w64. */
-
-typedef struct _MEM_ADDRESS_REQUIREMENTS
-{
-  PVOID LowestStartingAddress;
-  PVOID HighestEndingAddress;
-  SIZE_T Alignment;
-} MEM_ADDRESS_REQUIREMENTS, *PMEM_ADDRESS_REQUIREMENTS;
-
-typedef enum MEM_EXTENDED_PARAMETER_TYPE
-{
-  MemExtendedParameterInvalidType = 0,
-  MemExtendedParameterAddressRequirements,
-  MemExtendedParameterNumaNode,
-  MemExtendedParameterPartitionHandle,
-  MemExtendedParameterUserPhysicalHandle,
-  MemExtendedParameterAttributeFlags,
-  MemExtendedParameterMax
-} MEM_EXTENDED_PARAMETER_TYPE, *PMEM_EXTENDED_PARAMETER_TYPE;
-
-#define MEM_EXTENDED_PARAMETER_TYPE_BITS 8
-
-typedef struct DECLSPEC_ALIGN(8) MEM_EXTENDED_PARAMETER
-{
-  struct
-  {
-  DWORD64 Type : MEM_EXTENDED_PARAMETER_TYPE_BITS;
-  DWORD64 Reserved : 64 - MEM_EXTENDED_PARAMETER_TYPE_BITS;
-  };
-  union
-  {
-  DWORD64 ULong64;
-  PVOID Pointer;
-  SIZE_T Size;
-  HANDLE Handle;
-  DWORD ULong;
-  };
-} MEM_EXTENDED_PARAMETER, *PMEM_EXTENDED_PARAMETER;
-
-PVOID VirtualAlloc2 (HANDLE, PVOID, SIZE_T, ULONG, ULONG,
-PMEM_EXTENDED_PARAMETER, ULONG);
+
+PVOID WINAPI VirtualAlloc2 (HANDLE, PVOID, SIZE_T, ULONG, ULONG,
+   PMEM_EXTENDED_PARAMETER, ULONG);
 
 #ifdef __cplusplus
 }
diff --git a/winsup/utils/cygpath.cc b/winsup/utils/cygpath.cc
index aa9df3a21..bc5f11dd0 100644
--- a/winsup/utils/cygpath.cc
+++ b/winsup/utils/cygpath.cc
@@ -24,7 +24,6 @@ details. */
 #define _WIN32_WINNT 0x0a00
 #define WINVER 0x0a00
 #define NOCOMATTRIBUTE
-#define PMEM_EXTENDED_PARAMETER PVOID
 #include 
 #include 
 #include 
diff --git a/winsup/utils/ps.cc b/winsup/utils/ps.cc
index f3eb9e847..478ed8efd 100644
--- a/winsup/utils/ps.cc
+++ b/winsup/utils/ps.cc
@@ -6,7 +6,6 @@ This software is a copyrighted work licensed under the terms of 
the
 Cygwin license.  Please consult the file "CYGWIN_LICENSE" for
 details. */
 
-#define PMEM_EXTENDED_PARAMETER PVOID
 #include 
 #include 
 #include 
-- 
2.28.0

Re: [PATCH] Cygwin: winlean.h: remove most of extended memory API

2020-09-24 Thread Ken Brown via Cygwin-patches


On 9/24/2020 10:04 AM, Jon Turney wrote:

On 24/09/2020 00:52, Ken Brown via Cygwin-patches wrote:

This was added as a temporary measure in commit e18f7f99 because it
wasn't yet in the mingw-w64 headers.  With one exception, it is now in
the current release of the headers (version 8.0.0), so we don't need
it in winlean.h.  The exception is that VirtualAlloc2 is only declared
conditionally in , so retain it in winlean.h.  Add


I assume it's conditional on the windows version targetted, but it might help to 
mention that in a comment.


Done.


"WINAPI" to its declaration for consistency with the delaration in
memoryapi.h.

Also revert commit 3d136011, which was a related temporary workaround.


Looks good to me.

I think this isn't going work any more with older win32api, but we probably 
don't care about that.  It would perhaps be nice to explicitly complain about 
that (checking __MINGW64_VERSION_MAJOR somehow), rather than exploding 
incomprehensibly if the w32api is too old?


We probably don't care much, since anyone building Cygwin should have the latest 
tools installed.  On the other hand, it's simple to add that check, so I've done it.


In particular, I'd like to know if my handling of the declaration of 
VirtualAlloc2 seems reasonable.  Among other things, I'm puzzled by the 
apparent need to add WINAPI.  If it's really needed, I don't know how the 
calls of that function could have worked before.  Can anyone enlighten me?


I believe that WINAPI only does something (stdcall) on x86, so it might well be 
that it's never worked on Windows 10 =>1803 x86?


OK, I feel better now.

Thanks for the review.

Ken

Re: [PATCH v2] winsup/doc/faq-what.xml: FAQ 1.2 Windows versions supported

2020-09-21 Thread Ken Brown via Cygwin-patches


On 9/21/2020 3:22 PM, Jon Turney wrote:

On 18/09/2020 22:17, Ken Brown via Cygwin-patches wrote:

Do you have to run something to regen the docs, FAQ.html, and push to the web
site, or does it run periodically, so I can follow up to the OP and get feed
back from the responder?


No, sorry.  I don't know how/when that's done.


I believe all the built cygwin documentation files are present in the 
cygwin-htdocs git repo, so updating them in that would deploy to the website.


This is usually only done when a cygwin release is made.


Thanks, that's good to know.

Ken

Re: [PATCH v2] winsup/doc/faq-what.xml: FAQ 1.2 Windows versions supported

2020-09-18 Thread Ken Brown via Cygwin-patches


On 9/18/2020 11:29 AM, Brian Inglis wrote:

On 2020-09-18 05:59, Ken Brown via Cygwin-patches wrote:

On 9/17/2020 10:53 PM, Brian Inglis wrote:

enumerate Vista, 7, 8, 10 progression to be clear, and earliest server 2008;
add 8.1, exclude S mode, add Cygwin32 on ARM, specify 64 bit only AMD/Intel
---
   winsup/doc/faq-what.xml | 10 +-
   1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/winsup/doc/faq-what.xml b/winsup/doc/faq-what.xml
index ea8496ccbc65..77ba1c5fdd9c 100644
--- a/winsup/doc/faq-what.xml
+++ b/winsup/doc/faq-what.xml
@@ -30,12 +30,12 @@ They can be used from one of the provided Unix shells like
bash, tcsh or zsh.
   What versions of Windows are supported?
   
   -Cygwin can be expected to run on all modern, released versions of
Windows.
-State January 2016 this includes Windows Vista, Windows Server 2008 and all
-later versions of Windows up to Windows 10 and Windows Server 2016.
+Cygwin can be expected to run on all modern, released versions of 
Windows,
+from Windows Vista, 7, 8, 8.1, 10, Windows Server 2008 and all
+later versions of Windows, except Windows S mode due to its limitations.
   The 32 bit version of Cygwin also runs in the WOW64 32 bit environment on
-released 64 bit versions of Windows, the 64 bit version of course only on
-64 bit Windows.
+released 64 bit versions of Windows including ARM PCs,
+the 64 bit version of course only on 64 bit AMD/Intel compatible PCs.
   
   Keep in mind that Cygwin can only do as much as the underlying OS
   supports.  Because of this, Cygwin will behave differently, and


Pushed.  Thanks.

Ken


Thanks Ken,
Do you have to run something to regen the docs, FAQ.html, and push to the web
site, or does it run periodically, so I can follow up to the OP and get feed
back from the responder?


No, sorry.  I don't know how/when that's done.

Ken

Re: [PATCH] fhandler_proc.cc(format_proc_cpuinfo): add tsxldtrk, sev_es flags

2020-09-17 Thread Ken Brown via Cygwin-patches


On 9/17/2020 2:51 PM, Brian Inglis wrote:

Add linux-next cpuinfo flags for Intel TSX suspend load address tracking
instructions and AMD Secure Encrypted Virtualization with Encrypted State
---
  winsup/cygwin/fhandler_proc.cc | 8 
  1 file changed, 8 insertions(+)

diff --git a/winsup/cygwin/fhandler_proc.cc b/winsup/cygwin/fhandler_proc.cc
index 196bafd18993..6f6e8291a0ca 100644
--- a/winsup/cygwin/fhandler_proc.cc
+++ b/winsup/cygwin/fhandler_proc.cc
@@ -1376,6 +1376,7 @@ format_proc_cpuinfo (void *, char *)
  cpuid (, , , , 0x801f);
  
  	  ftcprint (features2,  1, "sev");	/* secure encrypted virt */

+   /*ftcprint (features2,  3, "sev_es"); - print below */
}
/* cpuid 0x8008 ebx */
if (maxe >= 0x8008)
@@ -1400,6 +1401,12 @@ format_proc_cpuinfo (void *, char *)
  /*  ftcprint (features1, 26, "ssb_no"); *//* ssb fixed in hardware */
  }
  
+  /* cpuid 0x801f eax - set above */

+  if (maxe >= 0x801f)
+   {
+ ftcprint (features2,  3, "sev_es"); /* AMD SEV encrypted state */
+   }
+
/* cpuid 0x0007 ebx */
if (maxf >= 0x0007)
{
@@ -1579,6 +1586,7 @@ format_proc_cpuinfo (void *, char *)
ftcprint (features1,  8, "avx512_vp2intersect"); /* vec intcpt d/q 
*/
ftcprint (features1, 10, "md_clear");/* verw clear buf 
*/
ftcprint (features1, 14, "serialize");   /* SERIALIZE 
instruction */
+  ftcprint (features1, 16, "tsxldtrk"); /* TSX Susp Ld 
Addr Track */
ftcprint (features1, 18, "pconfig"); /* platform 
config */
ftcprint (features1, 19, "arch_lbr");/* last branch 
records */
ftcprint (features1, 28, "flush_l1d");   /* flush l1d cache */


Pushed with a trivial change (added a period at the end of the commit message).

Thanks.

Ken

Re: [PATCH] winsup/doc/faq-what.xml: FAQ 1.2 Windows versions supported

2020-09-17 Thread Ken Brown via Cygwin-patches


On 9/17/2020 2:29 PM, Brian Inglis wrote:

Based on thread https://cygwin.com/pipermail/cygwin/2020-September/246318.html
enumerate Vista, 7, 8, 10 progression to be clear, and earliest server 2008
---
  winsup/doc/faq-what.xml | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/winsup/doc/faq-what.xml b/winsup/doc/faq-what.xml
index ea8496ccbc65..09747532c2e8 100644
--- a/winsup/doc/faq-what.xml
+++ b/winsup/doc/faq-what.xml
@@ -30,9 +30,9 @@ They can be used from one of the provided Unix shells like 
bash, tcsh or zsh.
  What versions of Windows are supported?
  
  
-Cygwin can be expected to run on all modern, released versions of Windows.

-State January 2016 this includes Windows Vista, Windows Server 2008 and all
-later versions of Windows up to Windows 10 and Windows Server 2016.
+Cygwin can be expected to run on all modern, released versions of 
Windows,
+from Windows Vista, 7, 8, 10, Windows Server 2008, and all
+later versions of Windows.
  The 32 bit version of Cygwin also runs in the WOW64 32 bit environment on
  released 64 bit versions of Windows, the 64 bit version of course only on
  64 bit Windows.


Since this is something that changes over time, I don't think you should drop 
the date completely, though I see no reason to retain "January 2016".  What 
would you think of revising your patch so that the text says something like this:


"Cygwin can be expected to run on all modern, released versions of Windows.  As 
of September 2020 this includes Windows Vista, 7, 8, 8.1, and 10, Windows Server 
2008, and all later versions of Windows Server."


Ken

Re: [PATCH v2] winsup/doc/faq-what.xml: FAQ 1.2 Windows versions supported

2020-09-18 Thread Ken Brown via Cygwin-patches


On 9/17/2020 10:53 PM, Brian Inglis wrote:

enumerate Vista, 7, 8, 10 progression to be clear, and earliest server 2008;
add 8.1, exclude S mode, add Cygwin32 on ARM, specify 64 bit only AMD/Intel
---
  winsup/doc/faq-what.xml | 10 +-
  1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/winsup/doc/faq-what.xml b/winsup/doc/faq-what.xml
index ea8496ccbc65..77ba1c5fdd9c 100644
--- a/winsup/doc/faq-what.xml
+++ b/winsup/doc/faq-what.xml
@@ -30,12 +30,12 @@ They can be used from one of the provided Unix shells like 
bash, tcsh or zsh.
  What versions of Windows are supported?
  
  
-Cygwin can be expected to run on all modern, released versions of Windows.

-State January 2016 this includes Windows Vista, Windows Server 2008 and all
-later versions of Windows up to Windows 10 and Windows Server 2016.
+Cygwin can be expected to run on all modern, released versions of 
Windows,
+from Windows Vista, 7, 8, 8.1, 10, Windows Server 2008 and all
+later versions of Windows, except Windows S mode due to its limitations.
  The 32 bit version of Cygwin also runs in the WOW64 32 bit environment on
-released 64 bit versions of Windows, the 64 bit version of course only on
-64 bit Windows.
+released 64 bit versions of Windows including ARM PCs,
+the 64 bit version of course only on 64 bit AMD/Intel compatible PCs.
  
  Keep in mind that Cygwin can only do as much as the underlying OS
  supports.  Because of this, Cygwin will behave differently, and


Pushed.  Thanks.

Ken

[PATCH] Cygwin: fix return value of sqrtl on negative infinity

2020-10-27 Thread Ken Brown via Cygwin-patches

The return value is now -NaN.

This fixes a bug in the mingw-w64 code that was imported into Cygwin.
The fix is consistent with Posix and Linux.  It is also consistent
with the current mingw-w64 code, with one exception: The mingw-w64
code sets errno to EDOM if the input is -NaN, but this appears to
differ from Posix and Linux.

Addresses: https://cygwin.com/pipermail/cygwin/2020-October/246606.html
---
 winsup/cygwin/math/sqrt.def.h | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/winsup/cygwin/math/sqrt.def.h b/winsup/cygwin/math/sqrt.def.h
index cf8b5cbe6..3d1a00908 100644
--- a/winsup/cygwin/math/sqrt.def.h
+++ b/winsup/cygwin/math/sqrt.def.h
@@ -73,8 +73,11 @@ __FLT_ABI (sqrt) (__FLT_TYPE x)
   if (x_class == FP_ZERO)
return __FLT_CST (-0.0);
 
+  if (x_class == FP_NAN)
+   return x;
+
   errno = EDOM;
-  return x;
+  return -__FLT_NAN;
 }
   else if (x_class == FP_ZERO)
 return __FLT_CST (0.0);
-- 
2.28.0

Re: [PATCH] Cygwin: mmap: Remove AT_ROUND_TO_PAGE workaround

2020-07-20 Thread Ken Brown via Cygwin-patches


Hi Corinna,

On 7/20/2020 2:55 PM, Corinna Vinschen wrote:

From: Corinna Vinschen 

It's working on 32 bit OSes only anyway. It even fails on WOW64.

Signed-off-by: Corinna Vinschen 
---

Notes:
 Hi Ken,
 
 can you please review this patch and check if it doesn't break

 your testcase again?
 
 Thanks,

 Corinna

  winsup/cygwin/mmap.cc | 117 --
  1 file changed, 34 insertions(+), 83 deletions(-)

diff --git a/winsup/cygwin/mmap.cc b/winsup/cygwin/mmap.cc
index 1fccc6c58ee9..8ac96606c2e6 100644
--- a/winsup/cygwin/mmap.cc
+++ b/winsup/cygwin/mmap.cc
@@ -195,12 +195,7 @@ MapView (HANDLE h, void *addr, size_t len, DWORD openflags,
DWORD protect = gen_create_protect (openflags, flags);
void *base = addr;
SIZE_T viewsize = len;
-#ifdef __x86_64__ /* AT_ROUND_TO_PAGE isn't supported on 64 bit systems. */
ULONG alloc_type = MEM_TOP_DOWN;
-#else
-  ULONG alloc_type = (base && !wincap.is_wow64 () ? AT_ROUND_TO_PAGE : 0)
-| MEM_TOP_DOWN;
-#endif
  
  #ifdef __x86_64__

/* Don't call NtMapViewOfSectionEx during fork.  It requires autoloading
@@ -878,6 +873,10 @@ mmap64 (void *addr, size_t len, int prot, int flags, int 
fd, off_t off)
  
if (!anonymous (flags) && fd != -1)

  {
+  UNICODE_STRING fname;
+  IO_STATUS_BLOCK io;
+  FILE_STANDARD_INFORMATION fsi;
+
/* Ensure that fd is open */
cygheap_fdget cfd (fd);
if (cfd < 0)
@@ -896,19 +895,16 @@ mmap64 (void *addr, size_t len, int prot, int flags, int 
fd, off_t off)
  
/* The autoconf mmap test maps a file of size 1 byte.  It then tests

 every byte of the entire mapped page of 64K for 0-bytes since that's
-what POSIX requires.  The problem is, we can't create that mapping on
-64 bit systems.  The file mapping will be only a single page, 4K, and
-since 64 bit systems don't support the AT_ROUND_TO_PAGE flag, the
-remainder of the 64K slot will result in a SEGV when accessed.
-
-So, what we do here is cheating for the sake of the autoconf test
-on 64 bit systems.  The justification is that there's very likely
-no application actually utilizing the map beyond EOF, and we know that
-all bytes beyond EOF are set to 0 anyway.  If this test doesn't work
-on 64 bit systems, it will result in not using mmap at all in a
-package.  But we want that mmap is treated as usable by autoconf,
-regardless whether the autoconf test runs on a 32 bit or a 64 bit
-system.
+what POSIX requires.  The problem is, we can't create that mapping.
+The file mapping will be only a single page, 4K, and the remainder
+of the 64K slot will result in a SEGV when accessed.
+
+So, what we do here is cheating for the sake of the autoconf test.
+The justification is that there's very likely no application actually
+utilizing the map beyond EOF, and we know that all bytes beyond EOF
+are set to 0 anyway.  If this test doesn't work, it will result in
+not using mmap at all in a package.  But we want mmap being treated
+as usable by autoconf.
  
  	 Ok, so we know exactly what autoconf is doing.  The file is called

 "conftest.txt", it has a size of 1 byte, the mapping size is the
@@ -916,31 +912,19 @@ mmap64 (void *addr, size_t len, int prot, int flags, int 
fd, off_t off)
 mapping is MAP_SHARED, the offset is 0.
  
  	 If all these requirements are given, we just return an anonymous map.

-This will help to get over the autoconf test even on 64 bit systems.
 The tests are ordered for speed. */
-#ifdef __x86_64__
-  if (1)
-#else
-  if (wincap.is_wow64 ())
-#endif
-   {
- UNICODE_STRING fname;
- IO_STATUS_BLOCK io;
- FILE_STANDARD_INFORMATION fsi;
-
- if (len == pagesize
- && prot == (PROT_READ | PROT_WRITE)
- && flags == MAP_SHARED
- && off == 0
- && (RtlSplitUnicodePath (fh->pc.get_nt_native_path (), NULL,
-  ),
- wcscmp (fname.Buffer, L"conftest.txt") == 0)
- && NT_SUCCESS (NtQueryInformationFile (fh->get_handle (), ,
-, sizeof fsi,
-FileStandardInformation))
- && fsi.EndOfFile.QuadPart == 1LL)
-   flags |= MAP_ANONYMOUS;
-   }
+  if (len == pagesize
+ && prot == (PROT_READ | PROT_WRITE)
+ && flags == MAP_SHARED
+ && off == 0
+ && (RtlSplitUnicodePath (fh->pc.get_nt_native_path (), NULL,
+  ),
+ wcscmp (fname.Buffer, L"conftest.txt") == 0)
+ && NT_SUCCESS (NtQueryInformationFile (fh->get_handle (), ,
+, sizeof fsi,
+

Re: [PATCH] Cygwin: mmap: fix mapping beyond EOF on 64 bit

2020-07-20 Thread Ken Brown via Cygwin-patches


On 7/20/2020 4:29 PM, Brian Inglis wrote:

On 2020-07-20 09:41, Corinna Vinschen wrote:

Ultimately, I wonder if we really should keep all the 32 bit OS stuff
in.  The number of real 32 bit systems (not WOW64) is dwindling fast.
Keeping all the AT_ROUND_TO_PAGE stuff in just for what? 2%? of the
systems is really not worth it, I guess.

[...]

If you don't want to ask and perhaps reconsider the impact of your approach,
maybe give folks a heads up on the mailing list that the current release will be
the last to support 32 bit Windows?


Corinna didn't propose dropping support for 32 bit Windows.  She proposed 
dropping a feature that works *only* on 32 bit Windows.


Ken

[PATCH] Cygwin: cygserver: build with -Wimplicit-fallthrough=5

2020-08-07 Thread Ken Brown via Cygwin-patches

Define the pseudo keyword 'fallthrough' in woutsup.h to support this.
---
 winsup/cygserver/Makefile.in   | 2 +-
 winsup/cygserver/bsd_helper.cc | 2 +-
 winsup/cygserver/bsd_mutex.cc  | 2 +-
 winsup/cygserver/woutsup.h | 2 ++
 4 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/winsup/cygserver/Makefile.in b/winsup/cygserver/Makefile.in
index bbdfc25fb..70f38233c 100644
--- a/winsup/cygserver/Makefile.in
+++ b/winsup/cygserver/Makefile.in
@@ -16,7 +16,7 @@ export CXX:=@CXX@
 
 CFLAGS:=@CFLAGS@
 override CXXFLAGS=@CXXFLAGS@
-override CXXFLAGS+=-MMD -Wimplicit-fallthrough=4 -Werror -D__OUTSIDE_CYGWIN__ 
-DSYSCONFDIR="\"$(sysconfdir)\""
+override CXXFLAGS+=-MMD -Wimplicit-fallthrough=5 -Werror -D__OUTSIDE_CYGWIN__ 
-DSYSCONFDIR="\"$(sysconfdir)\""
 
 include ${srcdir}/../Makefile.common
 
diff --git a/winsup/cygserver/bsd_helper.cc b/winsup/cygserver/bsd_helper.cc
index ecc90e117..38639647e 100644
--- a/winsup/cygserver/bsd_helper.cc
+++ b/winsup/cygserver/bsd_helper.cc
@@ -120,7 +120,7 @@ ipcexit_hookthread (const LPVOID param)
 {
   case WAIT_OBJECT_0:
 /* Cygserver shutdown. */
-   /*FALLTHRU*/
+   fallthrough;
   case WAIT_OBJECT_0 + 1:
 /* Process exited.  Call semexit_myhook to handle SEM_UNDOs for the
   exiting process and shmexit_myhook to keep track of shared
diff --git a/winsup/cygserver/bsd_mutex.cc b/winsup/cygserver/bsd_mutex.cc
index 13c5f90e8..0cda87a5b 100644
--- a/winsup/cygserver/bsd_mutex.cc
+++ b/winsup/cygserver/bsd_mutex.cc
@@ -326,7 +326,7 @@ _msleep (void *ident, struct mtx *mtx, int priority,
 break;
   case WAIT_OBJECT_0 + 1:  /* Shutdown event (triggered by wakeup_all). */
 priority |= PDROP;
-   /*FALLTHRU*/
+   fallthrough;
   case WAIT_OBJECT_0 + 2:  /* The dependent process has exited. */
debug ("msleep process exit or shutdown for %d", td->td_proc->winpid);
ret = EIDRM;
diff --git a/winsup/cygserver/woutsup.h b/winsup/cygserver/woutsup.h
index 272f978c0..7b799f156 100644
--- a/winsup/cygserver/woutsup.h
+++ b/winsup/cygserver/woutsup.h
@@ -12,6 +12,8 @@ details. */
 #error "woutsup.h is not for code being compiled inside the dll"
 #endif
 
+#define fallthrough__attribute__((__fallthrough__))
+
 #ifndef _WIN32_WINNT
 #define _WIN32_WINNT 0x0500
 #endif
-- 
2.28.0

[PATCH 1/8] Cygwin: FIFO: lock fixes

2020-08-04 Thread Ken Brown via Cygwin-patches

Add some missing locks and remove one extra unlock.  Clarify for some
functions whether caller or callee acquires lock, and add appropriate
comments.
---
 winsup/cygwin/fhandler_fifo.cc | 23 +--
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index e9d0187d4..ee7f47c0c 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -316,6 +316,7 @@ fhandler_fifo::wait_open_pipe (HANDLE& ph)
   return status;
 }
 
+/* Always called with fifo_client_lock in place. */
 int
 fhandler_fifo::add_client_handler (bool new_pipe_instance)
 {
@@ -345,6 +346,7 @@ fhandler_fifo::add_client_handler (bool new_pipe_instance)
   return 0;
 }
 
+/* Always called with fifo_client_lock in place. */
 void
 fhandler_fifo::delete_client_handler (int i)
 {
@@ -354,7 +356,8 @@ fhandler_fifo::delete_client_handler (int i)
 (nhandlers - i) * sizeof (fc_handler[i]));
 }
 
-/* Delete handlers that we will never read from. */
+/* Delete handlers that we will never read from.  Always called with
+   fifo_client_lock in place. */
 void
 fhandler_fifo::cleanup_handlers ()
 {
@@ -369,6 +372,7 @@ fhandler_fifo::cleanup_handlers ()
 }
 }
 
+/* Always called with fifo_client_lock in place. */
 void
 fhandler_fifo::record_connection (fifo_client_handler& fc,
  fifo_client_connect_state s)
@@ -398,6 +402,7 @@ fhandler_fifo::update_my_handlers ()
   if (!prev_proc)
 api_fatal ("Can't open process of previous owner, %E");
 
+  fifo_client_lock ();
   for (int i = 0; i < get_shared_nhandlers (); i++)
 {
   if (add_client_handler (false) < 0)
@@ -419,10 +424,12 @@ fhandler_fifo::update_my_handlers ()
  fc.last_read = shared_fc_handler[i].last_read;
}
 }
+  fifo_client_unlock ();
   set_prev_owner (null_fr_id);
   return ret;
 }
 
+/* Always called with fifo_client_lock and owner_lock in place. */
 int
 fhandler_fifo::update_shared_handlers ()
 {
@@ -435,9 +442,7 @@ fhandler_fifo::update_shared_handlers ()
   set_shared_nhandlers (nhandlers);
   memcpy (shared_fc_handler, fc_handler, nhandlers * sizeof (fc_handler[0]));
   shared_fc_handler_updated (true);
-  owner_lock ();
   set_prev_owner (me);
-  owner_unlock ();
   return 0;
 }
 
@@ -509,7 +514,6 @@ fhandler_fifo::fifo_reader_thread_func ()
  if (update_my_handlers () < 0)
debug_printf ("error updating my handlers, %E");
  owner_found ();
- owner_unlock ();
  /* Fall through to owner_listen. */
}
}
@@ -602,8 +606,13 @@ owner_listen:
}
   if (ph)
NtClose (ph);
-  if (update && update_shared_handlers () < 0)
-   api_fatal ("Can't update shared handlers, %E");
+  if (update)
+   {
+ owner_lock ();
+ if (get_owner () == me && update_shared_handlers () < 0)
+   api_fatal ("Can't update shared handlers, %E");
+ owner_unlock ();
+   }
   fifo_client_unlock ();
   if (cancel)
goto canceled;
@@ -1402,9 +1411,11 @@ fhandler_fifo::fstatvfs (struct statvfs *sfs)
 void
 fhandler_fifo::close_all_handlers ()
 {
+  fifo_client_lock ();
   for (int i = 0; i < nhandlers; i++)
 fc_handler[i].close ();
   nhandlers = 0;
+  fifo_client_unlock ();
 }
 
 fifo_client_connect_state
-- 
2.28.0

[PATCH 2/8] Cygwin: FIFO: fix timing issue with owner change

2020-08-04 Thread Ken Brown via Cygwin-patches

fhandler_fifo::take_ownership() tacitly assumes that the current
owner's fifo_reader_thread will be woken up from WFMO when
update_needed_evt is signaled.  But it's possible that the the current
owner's fifo_reader_thread is at the beginning of its main loop rather
than in its WFMO call when that event is signaled.

In this case the owner will never see that the event has been
signaled, and it will never update the shared fifo_client_handlers.
The reader that wants to take ownership will then spin its wheels
forever.

Fix this by having the current owner call update_shared_handlers at
the beginning of its loop, if necessary.
---
 winsup/cygwin/fhandler_fifo.cc | 25 +
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index ee7f47c0c..7b87aab00 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -472,17 +472,34 @@ fhandler_fifo::fifo_reader_thread_func ()
 
   if (pending_owner)
{
- if (pending_owner != me)
+ if (pending_owner == me)
+   take_ownership = true;
+ else if (cur_owner != me)
idle = true;
  else
-   take_ownership = true;
+   {
+ /* I'm the owner but someone else wants to be.  Have I
+already seen and reacted to update_needed_evt? */
+ if (WaitForSingleObject (update_needed_evt, 0) == WAIT_OBJECT_0)
+   {
+ /* No, I haven't. */
+ fifo_client_lock ();
+ if (update_shared_handlers () < 0)
+   api_fatal ("Can't update shared handlers, %E");
+ fifo_client_unlock ();
+   }
+ owner_unlock ();
+ /* Yield to pending owner. */
+ Sleep (1);
+ continue;
+   }
}
   else if (!cur_owner)
take_ownership = true;
   else if (cur_owner != me)
idle = true;
   else
-   /* I'm the owner. */
+   /* I'm the owner and there's no pending owner. */
goto owner_listen;
   if (idle)
{
@@ -1212,7 +1229,7 @@ fhandler_fifo::take_ownership ()
   /* Wake up my fifo_reader_thread. */
   owner_needed ();
   if (get_owner ())
-/* Wake up owner's fifo_reader_thread. */
+/* Wake up the owner and request an update of the shared fc_handlers. */
 SetEvent (update_needed_evt);
   owner_unlock ();
   /* The reader threads should now do the transfer. */
-- 
2.28.0

[PATCH 0/8] FIFO: bug fixes and small improvements

2020-08-04 Thread Ken Brown via Cygwin-patches

The second patch in this series fixes a serious bug that has resulted
in observable hangs.  The other patches are minor bug fixes or minor
performance improvements.

Ken Brown (8):
  Cygwin: FIFO: lock fixes
  Cygwin: FIFO: fix timing issue with owner change
  Cygwin: FIFO: add a timeout to take_ownership
  Cygwin: FIFO: reorganize some fifo_client_handler methods
  Cygwin: FIFO: don't read from pipes that are closing
  Cygwin: FIFO: synchronize the fifo_reader and fifosel threads
  Cygwin: FIFO: fix indentation
  Cygwin: FIFO: add a third pass to raw_read

 winsup/cygwin/fhandler.h   |  24 +--
 winsup/cygwin/fhandler_fifo.cc | 358 ++---
 winsup/cygwin/select.cc|  38 ++--
 3 files changed, 268 insertions(+), 152 deletions(-)

-- 
2.28.0

[PATCH 5/8] Cygwin: FIFO: don't read from pipes that are closing

2020-08-04 Thread Ken Brown via Cygwin-patches

Don't try to read from fifo_client_handlers that are in the fc_closing
state.  Experiments have shown that this always yields
STATUS_PIPE_BROKEN, so it just wastes a Windows system call.

Re-order the values in enum fifo_client_connect_state to reflect the
new status of fc_closing.
---
 winsup/cygwin/fhandler.h   | 9 +
 winsup/cygwin/fhandler_fifo.cc | 6 +++---
 winsup/cygwin/select.cc| 2 +-
 3 files changed, 5 insertions(+), 12 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index f64eabda4..40e201b0f 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1276,20 +1276,13 @@ public:
 
 #define CYGWIN_FIFO_PIPE_NAME_LEN 47
 
-/* We view the fc_closing state as borderline between open and closed
-   for a writer at the other end of a fifo_client_handler.
-
-   We still attempt to read from the writer when the handler is in
-   this state, and we don't declare a reader to be at EOF if there's a
-   handler in this state.  But the existence of a handler in this
-   state is not sufficient to unblock a reader trying to open. */
 enum fifo_client_connect_state
 {
   fc_unknown,
   fc_error,
   fc_disconnected,
-  fc_listening,
   fc_closing,
+  fc_listening,
   fc_connected,
   fc_input_avail,
 };
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index c816c692a..1e1140f53 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -365,7 +365,7 @@ fhandler_fifo::cleanup_handlers ()
 
   while (i < nhandlers)
 {
-  if (fc_handler[i].get_state () < fc_closing)
+  if (fc_handler[i].get_state () < fc_connected)
delete_client_handler (i);
   else
i++;
@@ -1280,7 +1280,7 @@ fhandler_fifo::raw_read (void *in_ptr, size_t& len)
   for (j = 0; j < nhandlers; j++)
if (fc_handler[j].last_read)
  break;
-  if (j < nhandlers && fc_handler[j].get_state () >= fc_closing)
+  if (j < nhandlers && fc_handler[j].get_state () >= fc_connected)
{
  NTSTATUS status;
  IO_STATUS_BLOCK io;
@@ -1315,7 +1315,7 @@ fhandler_fifo::raw_read (void *in_ptr, size_t& len)
 
   /* Second pass. */
   for (int i = 0; i < nhandlers; i++)
-   if (fc_handler[i].get_state () >= fc_closing)
+   if (fc_handler[i].get_state () >= fc_connected)
  {
NTSTATUS status;
IO_STATUS_BLOCK io;
diff --git a/winsup/cygwin/select.cc b/winsup/cygwin/select.cc
index 0c94f6c45..9ee305f64 100644
--- a/winsup/cygwin/select.cc
+++ b/winsup/cygwin/select.cc
@@ -878,7 +878,7 @@ peek_fifo (select_record *s, bool from_select)
{
  fifo_client_handler  = fh->get_fc_handler (i);
  fc.query_and_set_state ();
- if (fc.get_state () >= fc_closing)
+ if (fc.get_state () >= fc_connected)
{
  nconnected++;
  if (fc.get_state () == fc_input_avail)
-- 
2.28.0

[PATCH 4/8] Cygwin: FIFO: reorganize some fifo_client_handler methods

2020-08-04 Thread Ken Brown via Cygwin-patches

Rename the existing set_state() to query_and_set_state() to reflect
what it really does.  (It queries the O/S for the pipe state.)  Add a
new set_state() method, which is a standard setter, and a
corresponding getter get_state().
---
 winsup/cygwin/fhandler.h   |  9 --
 winsup/cygwin/fhandler_fifo.cc | 50 +++---
 winsup/cygwin/select.cc| 28 +++
 3 files changed, 50 insertions(+), 37 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 5488327a2..f64eabda4 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1297,11 +1297,14 @@ enum fifo_client_connect_state
 struct fifo_client_handler
 {
   HANDLE h;
-  fifo_client_connect_state state;
+  fifo_client_connect_state _state;
   bool last_read;  /* true if our last successful read was from this client. */
-  fifo_client_handler () : h (NULL), state (fc_unknown), last_read (false) {}
+  fifo_client_handler () : h (NULL), _state (fc_unknown), last_read (false) {}
   void close () { NtClose (h); }
-  fifo_client_connect_state set_state ();
+  fifo_client_connect_state get_state () const { return _state; }
+  void set_state (fifo_client_connect_state s) { _state = s; }
+  /* Query O/S.  Return previous state. */
+  fifo_client_connect_state query_and_set_state ();
 };
 
 class fhandler_fifo;
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index b8a47f27f..c816c692a 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -340,7 +340,7 @@ fhandler_fifo::add_client_handler (bool new_pipe_instance)
   if (!ph)
return -1;
   fc.h = ph;
-  fc.state = fc_listening;
+  fc.set_state (fc_listening);
 }
   fc_handler[nhandlers++] = fc;
   return 0;
@@ -365,7 +365,7 @@ fhandler_fifo::cleanup_handlers ()
 
   while (i < nhandlers)
 {
-  if (fc_handler[i].state < fc_closing)
+  if (fc_handler[i].get_state () < fc_closing)
delete_client_handler (i);
   else
i++;
@@ -377,7 +377,7 @@ void
 fhandler_fifo::record_connection (fifo_client_handler& fc,
  fifo_client_connect_state s)
 {
-  fc.state = s;
+  fc.set_state (s);
   set_pipe_non_blocking (fc.h, true);
 }
 
@@ -414,13 +414,13 @@ fhandler_fifo::update_my_handlers ()
{
  debug_printf ("Can't duplicate handle of previous owner, %E");
  __seterrno ();
- fc.state = fc_error;
+ fc.set_state (fc_error);
  fc.last_read = false;
  ret = -1;
}
   else
{
- fc.state = shared_fc_handler[i].state;
+ fc.set_state (shared_fc_handler[i].get_state ());
  fc.last_read = shared_fc_handler[i].last_read;
}
 }
@@ -614,7 +614,7 @@ owner_listen:
break;
  default:
debug_printf ("NtFsControlFile status %y after failing to 
connect bogus client or real client", io.Status);
-   fc.state = fc_unknown;
+   fc.set_state (fc_unknown);
break;
  }
  break;
@@ -1280,7 +1280,7 @@ fhandler_fifo::raw_read (void *in_ptr, size_t& len)
   for (j = 0; j < nhandlers; j++)
if (fc_handler[j].last_read)
  break;
-  if (j < nhandlers && fc_handler[j].state >= fc_closing)
+  if (j < nhandlers && fc_handler[j].get_state () >= fc_closing)
{
  NTSTATUS status;
  IO_STATUS_BLOCK io;
@@ -1303,11 +1303,11 @@ fhandler_fifo::raw_read (void *in_ptr, size_t& len)
case STATUS_PIPE_EMPTY:
  break;
case STATUS_PIPE_BROKEN:
- fc_handler[j].state = fc_disconnected;
+ fc_handler[j].set_state (fc_disconnected);
  break;
default:
  debug_printf ("NtReadFile status %y", status);
- fc_handler[j].state = fc_error;
+ fc_handler[j].set_state (fc_error);
  break;
}
  fc_handler[j].last_read = false;
@@ -1315,7 +1315,7 @@ fhandler_fifo::raw_read (void *in_ptr, size_t& len)
 
   /* Second pass. */
   for (int i = 0; i < nhandlers; i++)
-   if (fc_handler[i].state >= fc_closing)
+   if (fc_handler[i].get_state () >= fc_closing)
  {
NTSTATUS status;
IO_STATUS_BLOCK io;
@@ -1339,12 +1339,12 @@ fhandler_fifo::raw_read (void *in_ptr, size_t& len)
  case STATUS_PIPE_EMPTY:
break;
  case STATUS_PIPE_BROKEN:
-   fc_handler[i].state = fc_disconnected;
+   fc_handler[i].set_state (fc_disconnected);
nconnected--;
break;
  default:
debug_printf ("NtReadFile status %y", status);
-   fc_handler[i].state = fc_error;
+   fc_handler[i].set_state (fc_error);
nconnected--;
break;
  }
@@ -1417,45 +1417,51 @@

[PATCH 3/8] Cygwin: FIFO: add a timeout to take_ownership

2020-08-04 Thread Ken Brown via Cygwin-patches

fhandler_fifo::take_ownership() is called from select.cc::peek_fifo
and fhandler_fifo::raw_read and could potentially block indefinitely
if something goes wrong.  This is always undesirable in peek_fifo, and
it is undesirable in a nonblocking read.  Fix this by adding a timeout
parameter to take_ownership.

Arbitrarily use a 1 ms timeout in peek_fifo and a 10 ms timeout in
raw_read.  These numbers may have to be tweaked based on experience.

Replace the call to cygwait in take_ownership by a call to WFSO.
There's no need to allow interruption now that we have a timeout.
---
 winsup/cygwin/fhandler.h   |  2 +-
 winsup/cygwin/fhandler_fifo.cc | 74 +-
 winsup/cygwin/select.cc|  7 +---
 3 files changed, 30 insertions(+), 53 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 60bd27e00..5488327a2 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1487,7 +1487,7 @@ public:
   void fifo_client_lock () { _fifo_client_lock.lock (); }
   void fifo_client_unlock () { _fifo_client_lock.unlock (); }
 
-  DWORD take_ownership ();
+  int take_ownership (DWORD timeout = INFINITE);
   void reading_lock () { shmem->reading_lock (); }
   void reading_unlock () { shmem->reading_unlock (); }
 
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index 7b87aab00..b8a47f27f 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -1214,16 +1214,17 @@ fhandler_fifo::raw_write (const void *ptr, size_t len)
   return ret;
 }
 
-/* Called from raw_read and select.cc:peek_fifo.  Return WAIT_OBJECT_0
-   on success.  */
-DWORD
-fhandler_fifo::take_ownership ()
+/* Called from raw_read and select.cc:peek_fifo. */
+int
+fhandler_fifo::take_ownership (DWORD timeout)
 {
+  int ret = 0;
+
   owner_lock ();
   if (get_owner () == me)
 {
   owner_unlock ();
-  return WAIT_OBJECT_0;
+  return 0;
 }
   set_pending_owner (me);
   /* Wake up my fifo_reader_thread. */
@@ -1233,18 +1234,25 @@ fhandler_fifo::take_ownership ()
 SetEvent (update_needed_evt);
   owner_unlock ();
   /* The reader threads should now do the transfer. */
-  DWORD waitret = cygwait (owner_found_evt, cw_cancel | cw_sig_eintr);
-  owner_lock ();
-  if (waitret == WAIT_OBJECT_0
-  && (get_owner () != me || get_pending_owner ()))
+  switch (WaitForSingleObject (owner_found_evt, timeout))
 {
-  /* Something went wrong.  Return WAIT_TIMEOUT, which can't be
-returned by the above cygwait call. */
-  set_pending_owner (null_fr_id);
-  waitret = WAIT_TIMEOUT;
+case WAIT_OBJECT_0:
+  owner_lock ();
+  if (get_owner () != me)
+   {
+ debug_printf ("owner_found_evt signaled, but I'm not the owner");
+ ret = -1;
+   }
+  owner_unlock ();
+  break;
+case WAIT_TIMEOUT:
+  debug_printf ("timed out");
+  ret = -1;
+default:
+  debug_printf ("WFSO failed, %E");
+  ret = -1;
 }
-  owner_unlock ();
-  return waitret;
+  return ret;
 }
 
 void __reg3
@@ -1255,38 +1263,12 @@ fhandler_fifo::raw_read (void *in_ptr, size_t& len)
 
   while (1)
 {
+  int nconnected = 0;
+
   /* No one else can take ownership while we hold the reading_lock. */
   reading_lock ();
-  switch (take_ownership ())
-   {
-   case WAIT_OBJECT_0:
- break;
-   case WAIT_SIGNALED:
- if (_my_tls.call_signal_handler ())
-   {
- reading_unlock ();
- continue;
-   }
- else
-   {
- set_errno (EINTR);
- reading_unlock ();
- goto errout;
-   }
- break;
-   case WAIT_CANCELED:
- reading_unlock ();
- pthread::static_cancel_self ();
- break;
-   case WAIT_TIMEOUT:
- reading_unlock ();
- debug_printf ("take_ownership returned an unexpected result; retry");
- continue;
-   default:
- reading_unlock ();
- debug_printf ("unknown error while trying to take ownership, %E");
- goto errout;
-   }
+  if (take_ownership (10) < 0)
+   goto maybe_retry;
 
   /* Poll the connected clients for input.  Make two passes.  On
 the first pass, just try to read from the client from which
@@ -1332,7 +1314,6 @@ fhandler_fifo::raw_read (void *in_ptr, size_t& len)
}
 
   /* Second pass. */
-  int nconnected = 0;
   for (int i = 0; i < nhandlers; i++)
if (fc_handler[i].state >= fc_closing)
  {
@@ -1375,6 +1356,7 @@ fhandler_fifo::raw_read (void *in_ptr, size_t& len)
  len = 0;
  return;
}
+maybe_retry:
   reading_unlock ();
   if (is_nonblocking ())
{
diff --git a/winsup/cygwin/select.cc b/winsup/cygwin/select.cc
index 3f3f33fb5..1ba76c817 100644
--- a/winsup/cygwin/select.cc
+++ b/winsup/cygwin/select.cc
@@ -867,16 +867,11 @@ peek_fifo (select_record *s,

[PATCH 6/8] Cygwin: FIFO: synchronize the fifo_reader and fifosel threads

2020-08-04 Thread Ken Brown via Cygwin-patches

The fifo_reader thread function and the function select.cc:peek_fifo()
can both change the state of a fifo_client_handler.  These changes are
made under fifo_client_lock, so there is no race, but the changes can
still be incompatible.

Add code to make sure that only one of these functions can change the
state from its initial fc_listening state.  Whichever function does
this calls the fhandler_fifo::record_connection method, which is now
public so that peek_fifo can call it.

Slightly modify that method to make it suitable for being called by
peek_fifo.

Make a few other small changes to the fifo_reader thread function to
change how it deals with the STATUS_PIPE_CLOSING value that can
(rarely) be returned by NtFsControlFile.

Add commentary to fhandler_fifo.cc to explain fifo_client connect
states and where they can be changed.
---
 winsup/cygwin/fhandler.h   |  4 +--
 winsup/cygwin/fhandler_fifo.cc | 60 ++
 winsup/cygwin/select.cc|  5 ++-
 3 files changed, 60 insertions(+), 9 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 40e201b0f..a577ca542 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1413,8 +1413,6 @@ class fhandler_fifo: public fhandler_base
   void cleanup_handlers ();
   void close_all_handlers ();
   void cancel_reader_thread ();
-  void record_connection (fifo_client_handler&,
- fifo_client_connect_state = fc_connected);
 
   int create_shmem (bool only_open = false);
   int reopen_shmem ();
@@ -1482,6 +1480,8 @@ public:
   DWORD fifo_reader_thread_func ();
   void fifo_client_lock () { _fifo_client_lock.lock (); }
   void fifo_client_unlock () { _fifo_client_lock.unlock (); }
+  void record_connection (fifo_client_handler&, bool = true,
+ fifo_client_connect_state = fc_connected);
 
   int take_ownership (DWORD timeout = INFINITE);
   void reading_lock () { shmem->reading_lock (); }
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index 1e1140f53..2b829eb6c 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -37,11 +37,42 @@
  "fifo_client_handler" structures, one for each writer.  A
  fifo_client_handler contains the handle for the pipe server
  instance and information about the state of the connection with
- the writer.  Each writer holds the pipe instance's client handle.
+ the writer.  Access to the list is controlled by a
+ "fifo_client_lock".
 
  The reader runs a "fifo_reader_thread" that creates new pipe
  instances as needed and listens for client connections.
 
+ The connection state of a fifo_client_handler has one of the
+ following values, in which order is important:
+
+   fc_unknown
+   fc_error
+   fc_disconnected
+   fc_closing
+   fc_listening
+   fc_connected
+   fc_input_avail
+
+ It can be changed in the following places:
+
+   - It is set to fc_listening when the pipe instance is created.
+
+   - It is set to fc_connected when the fifo_reader_thread detects
+ a connection.
+
+   - It is set to a value reported by the O/S when
+ query_and_set_state is called.  This can happen in
+ select.cc:peek_fifo and a couple other places.
+
+   - It is set to fc_disconnected by raw_read when an attempt to
+ read yields STATUS_PIPE_BROKEN.
+
+   - It is set to fc_error in various places when unexpected
+ things happen.
+
+ State changes are always guarded by fifo_client_lock.
+
  If there are multiple readers open, only one of them, called the
  "owner", maintains the fifo_client_handler list.  The owner is
  therefore the only reader that can read at any given time.  If a
@@ -374,10 +405,11 @@ fhandler_fifo::cleanup_handlers ()
 
 /* Always called with fifo_client_lock in place. */
 void
-fhandler_fifo::record_connection (fifo_client_handler& fc,
+fhandler_fifo::record_connection (fifo_client_handler& fc, bool set,
  fifo_client_connect_state s)
 {
-  fc.set_state (s);
+  if (set)
+fc.set_state (s);
   set_pipe_non_blocking (fc.h, true);
 }
 
@@ -583,6 +615,11 @@ owner_listen:
   NTSTATUS status1;
 
   fifo_client_lock ();
+  if (fc.get_state () != fc_listening)
+   /* select.cc:peek_fifo has already recorded a connection. */
+   ;
+  else
+  {
   switch (status)
{
case STATUS_SUCCESS:
@@ -590,7 +627,12 @@ owner_listen:
  record_connection (fc);
  break;
case STATUS_PIPE_CLOSING:
- record_connection (fc, fc_closing);
+ debug_printf ("NtFsControlFile got STATUS_PIPE_CLOSING...");
+ /* Maybe a writer already connected, wrote, and closed.
+Just query the O/S. */
+ fc.query_and_set_state ();
+ debug_printf ("...O/S reports state %d", fc.get_state ());
+

[PATCH 8/8] Cygwin: FIFO: add a third pass to raw_read

2020-08-04 Thread Ken Brown via Cygwin-patches

Currently raw_read makes two passes through the list of clients.  On
the first pass it tries to read from the client from which it last
read successfully.  On the second pass it tries to read from all
connected clients.

Add a new pass in between these two, in which raw_read tries to read
from all clients that are in the fc_input_avail case.  This should be
more efficient in case select was previously called and detected input
available.

Slightly tweak the first pass.  If a client is marked as having the
last successful read but reading from it now finds no input, don't
unmark it unless we successfully read from a different client on one
of the later passes.
---
 winsup/cygwin/fhandler_fifo.cc | 66 ++
 1 file changed, 60 insertions(+), 6 deletions(-)

diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index 017d44e54..a33c32b73 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -1318,17 +1318,30 @@ fhandler_fifo::raw_read (void *in_ptr, size_t& len)
   if (take_ownership (10) < 0)
goto maybe_retry;
 
-  /* Poll the connected clients for input.  Make two passes.  On
-the first pass, just try to read from the client from which
-we last read successfully.  This should minimize
-interleaving of writes from different clients. */
   fifo_client_lock ();
+  /* Poll the connected clients for input.  Make three passes.
+
+On the first pass, just try to read from the client from
+which we last read successfully.  This should minimize
+interleaving of writes from different clients.
+
+On the second pass, just try to read from the clients in the
+state fc_input_avail.  This should be more efficient if
+select has been called and detected input available.
+
+On the third pass, try to read from all connected clients. */
+
   /* First pass. */
   int j;
   for (j = 0; j < nhandlers; j++)
if (fc_handler[j].last_read)
  break;
-  if (j < nhandlers && fc_handler[j].get_state () >= fc_connected)
+  if (j < nhandlers && fc_handler[j].get_state () < fc_connected)
+   {
+ fc_handler[j].last_read = false;
+ j = nhandlers;
+   }
+  if (j < nhandlers)
{
  NTSTATUS status;
  IO_STATUS_BLOCK io;
@@ -1349,6 +1362,8 @@ fhandler_fifo::raw_read (void *in_ptr, size_t& len)
}
  break;
case STATUS_PIPE_EMPTY:
+ /* Update state in case it's fc_input_avail. */
+ fc_handler[j].set_state (fc_connected);
  break;
case STATUS_PIPE_BROKEN:
  fc_handler[j].set_state (fc_disconnected);
@@ -1358,10 +1373,47 @@ fhandler_fifo::raw_read (void *in_ptr, size_t& len)
  fc_handler[j].set_state (fc_error);
  break;
}
- fc_handler[j].last_read = false;
}
 
   /* Second pass. */
+  for (int i = 0; i < nhandlers; i++)
+   if (fc_handler[i].get_state () == fc_input_avail)
+ {
+   NTSTATUS status;
+   IO_STATUS_BLOCK io;
+
+   status = NtReadFile (fc_handler[i].h, NULL, NULL, NULL,
+, in_ptr, len, NULL, NULL);
+   switch (status)
+ {
+ case STATUS_SUCCESS:
+ case STATUS_BUFFER_OVERFLOW:
+   if (io.Information > 0)
+ {
+   len = io.Information;
+   if (j < nhandlers)
+ fc_handler[j].last_read = false;
+   fc_handler[i].last_read = true;
+   fifo_client_unlock ();
+   reading_unlock ();
+   return;
+ }
+   break;
+ case STATUS_PIPE_EMPTY:
+   /* No input available after all. */
+   fc_handler[i].set_state (fc_connected);
+   break;
+ case STATUS_PIPE_BROKEN:
+   fc_handler[i].set_state (fc_disconnected);
+   break;
+ default:
+   debug_printf ("NtReadFile status %y", status);
+   fc_handler[i].set_state (fc_error);
+   break;
+ }
+ }
+
+  /* Third pass. */
   for (int i = 0; i < nhandlers; i++)
if (fc_handler[i].get_state () >= fc_connected)
  {
@@ -1378,6 +1430,8 @@ fhandler_fifo::raw_read (void *in_ptr, size_t& len)
if (io.Information > 0)
  {
len = io.Information;
+   if (j < nhandlers)
+ fc_handler[j].last_read = false;
fc_handler[i].last_read = true;
fifo_client_unlock ();
reading_unlock ();
-- 
2.28.0

[PATCH 7/8] Cygwin: FIFO: fix indentation

2020-08-04 Thread Ken Brown via Cygwin-patches

---
 winsup/cygwin/fhandler_fifo.cc | 96 +-
 1 file changed, 48 insertions(+), 48 deletions(-)

diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index 2b829eb6c..017d44e54 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -619,56 +619,56 @@ owner_listen:
/* select.cc:peek_fifo has already recorded a connection. */
;
   else
-  {
-  switch (status)
{
-   case STATUS_SUCCESS:
-   case STATUS_PIPE_CONNECTED:
- record_connection (fc);
- break;
-   case STATUS_PIPE_CLOSING:
- debug_printf ("NtFsControlFile got STATUS_PIPE_CLOSING...");
- /* Maybe a writer already connected, wrote, and closed.
-Just query the O/S. */
- fc.query_and_set_state ();
- debug_printf ("...O/S reports state %d", fc.get_state ());
- record_connection (fc, false);
- break;
-   case STATUS_THREAD_IS_TERMINATING:
-   case STATUS_WAIT_1:
- /* Try to connect a bogus client.  Otherwise fc is still
-listening, and the next connection might not get recorded. */
- status1 = open_pipe (ph);
- WaitForSingleObject (conn_evt, INFINITE);
- if (NT_SUCCESS (status1))
-   /* Bogus cilent connected. */
-   delete_client_handler (nhandlers - 1);
- else
-   /* Did a real client connect? */
-   switch (io.Status)
- {
- case STATUS_SUCCESS:
- case STATUS_PIPE_CONNECTED:
-   record_connection (fc);
-   break;
- case STATUS_PIPE_CLOSING:
-   debug_printf ("got STATUS_PIPE_CLOSING when trying to connect 
bogus client...");
-   fc.query_and_set_state ();
-   debug_printf ("...O/S reports state %d", fc.get_state ());
-   record_connection (fc, false);
-   break;
- default:
-   debug_printf ("NtFsControlFile status %y after failing to 
connect bogus client or real client", io.Status);
-   fc.set_state (fc_error);
-   break;
- }
- break;
-   default:
- debug_printf ("NtFsControlFile got unexpected status %y", status);
- fc.set_state (fc_error);
- break;
+ switch (status)
+   {
+   case STATUS_SUCCESS:
+   case STATUS_PIPE_CONNECTED:
+ record_connection (fc);
+ break;
+   case STATUS_PIPE_CLOSING:
+ debug_printf ("NtFsControlFile got STATUS_PIPE_CLOSING...");
+ /* Maybe a writer already connected, wrote, and closed.
+Just query the O/S. */
+ fc.query_and_set_state ();
+ debug_printf ("...O/S reports state %d", fc.get_state ());
+ record_connection (fc, false);
+ break;
+   case STATUS_THREAD_IS_TERMINATING:
+   case STATUS_WAIT_1:
+ /* Try to connect a bogus client.  Otherwise fc is still
+listening, and the next connection might not get recorded. */
+ status1 = open_pipe (ph);
+ WaitForSingleObject (conn_evt, INFINITE);
+ if (NT_SUCCESS (status1))
+   /* Bogus cilent connected. */
+   delete_client_handler (nhandlers - 1);
+ else
+   /* Did a real client connect? */
+   switch (io.Status)
+ {
+ case STATUS_SUCCESS:
+ case STATUS_PIPE_CONNECTED:
+   record_connection (fc);
+   break;
+ case STATUS_PIPE_CLOSING:
+   debug_printf ("got STATUS_PIPE_CLOSING when trying to 
connect bogus client...");
+   fc.query_and_set_state ();
+   debug_printf ("...O/S reports state %d", fc.get_state ());
+   record_connection (fc, false);
+   break;
+ default:
+   debug_printf ("NtFsControlFile status %y after failing to 
connect bogus client or real client", io.Status);
+   fc.set_state (fc_error);
+   break;
+ }
+ break;
+   default:
+ debug_printf ("NtFsControlFile got unexpected status %y", status);
+ fc.set_state (fc_error);
+ break;
+   }
}
-  }
   if (ph)
NtClose (ph);
   if (update)
-- 
2.28.0

Re: Cygwin 3.1.6

2020-07-06 Thread Ken Brown via Cygwin-patches


On 7/6/2020 3:50 PM, Corinna Vinschen wrote:

Hi guys,

Do you have anything in the loop which should go into 3.1.6?

Given https://sourceware.org/git/?p=newlib-cygwin.git;a=commitdiff;h=bb96bd0,
I'd like to release 3.1.6 this week.


I'm working on some FIFO fixes, but it could be another week until they're done 
and thoroughly tested.  So I think you should go ahead, and the FIFO stuff can 
wait for 3.1.7.


Ken

Re: [PATCH v3] Cygwin: pty: Fix screen distortion after less for native apps again.

2020-06-09 Thread Ken Brown via Cygwin-patches


On 6/3/2020 9:43 PM, Takashi Yano via Cygwin-patches wrote:

- Commit c4b060e3fe3bed05b3a69ccbcc20993ad85e163d seems to be not
   enough. Moreover, it does not work as expected at all in Win10
   1809. This patch essentially reverts that commit and add another
   fix. After all, the cause of the problem was a race issue in
   switch_to_pcon_out flag. That is, this flag is set when native
   app starts, however, it is delayed by wait_pcon_fwd(). Since the
   flag is not set yet when less starts, the data which should go
   into the output_handle accidentally goes into output_handle_cyg.
   This patch fixes the problem more essentially for the cause of
   the problem than previous one.
---
  winsup/cygwin/fhandler.h  |  1 -
  winsup/cygwin/fhandler_tty.cc | 49 +++
  winsup/cygwin/tty.cc  |  7 -
  winsup/cygwin/tty.h   |  1 -
  4 files changed, 21 insertions(+), 37 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 4035c7e56..c6ce6d8e1 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -2354,7 +2354,6 @@ class fhandler_pty_slave: public fhandler_pty_common
void setup_locale (void);
void set_freeconsole_on_close (bool val);
void trigger_redraw_screen (void);
-  void wait_pcon_fwd (void);
void pull_pcon_input (void);
void update_pcon_input_state (bool need_lock);
  };
diff --git a/winsup/cygwin/fhandler_tty.cc b/winsup/cygwin/fhandler_tty.cc
index bcc7648f3..126249d9f 100644
--- a/winsup/cygwin/fhandler_tty.cc
+++ b/winsup/cygwin/fhandler_tty.cc
@@ -1277,6 +1277,7 @@ fhandler_pty_slave::set_switch_to_pcon (int fd_set)
  {
if (fd < 0)
  fd = fd_set;
+  acquire_output_mutex (INFINITE);
if (fd == 0 && !get_ttyp ()->switch_to_pcon_in)
  {
pull_pcon_input ();
@@ -1286,13 +1287,13 @@ fhandler_pty_slave::set_switch_to_pcon (int fd_set)
get_ttyp ()->switch_to_pcon_in = true;
if (isHybrid && !get_ttyp ()->switch_to_pcon_out)
{
- wait_pcon_fwd ();
+ get_ttyp ()->wait_pcon_fwd ();
  get_ttyp ()->switch_to_pcon_out = true;
}
  }
else if ((fd == 1 || fd == 2) && !get_ttyp ()->switch_to_pcon_out)
  {
-  wait_pcon_fwd ();
+  get_ttyp ()->wait_pcon_fwd ();
if (get_ttyp ()->pcon_pid == 0 ||
  !pinfo (get_ttyp ()->pcon_pid))
get_ttyp ()->pcon_pid = myself->pid;
@@ -1300,6 +1301,7 @@ fhandler_pty_slave::set_switch_to_pcon (int fd_set)
if (isHybrid)
get_ttyp ()->switch_to_pcon_in = true;
  }
+  release_output_mutex ();
  }
  
  void

@@ -1314,12 +1316,14 @@ fhandler_pty_slave::reset_switch_to_pcon (void)
  return;
if (do_not_reset_switch_to_pcon)
  return;
+  acquire_output_mutex (INFINITE);
if (get_ttyp ()->switch_to_pcon_out)
  /* Wait for pty_master_fwd_thread() */
-wait_pcon_fwd ();
+get_ttyp ()->wait_pcon_fwd ();
get_ttyp ()->pcon_pid = 0;
get_ttyp ()->switch_to_pcon_in = false;
get_ttyp ()->switch_to_pcon_out = false;
+  release_output_mutex ();
init_console_handler (true);
  }
  
@@ -1372,7 +1376,7 @@ fhandler_pty_slave::push_to_pcon_screenbuffer (const char *ptr, size_t len,

  p0 = (char *) memmem (p1, nlen - (p1-buf), "\033[?1049h", 8);
  if (p0)
{
- p0 += 8;
+ //p0 += 8;
  get_ttyp ()->screen_alternated = true;
  if (get_ttyp ()->switch_to_pcon_out)
do_not_reset_switch_to_pcon = true;
@@ -1384,7 +1388,7 @@ fhandler_pty_slave::push_to_pcon_screenbuffer (const char 
*ptr, size_t len,
  p1 = (char *) memmem (p0, nlen - (p0-buf), "\033[?1049l", 8);
  if (p1)
{
- //p1 += 8;
+ p1 += 8;
  get_ttyp ()->screen_alternated = false;
  do_not_reset_switch_to_pcon = false;
  memmove (p0, p1, buf+nlen - p1);
@@ -1504,8 +1508,9 @@ fhandler_pty_slave::write (const void *ptr, size_t len)
  
reset_switch_to_pcon ();
  
-  bool output_to_pcon =

-get_ttyp ()->switch_to_pcon_out && !get_ttyp ()->screen_alternated;
+  acquire_output_mutex (INFINITE);
+  bool output_to_pcon = get_ttyp ()->switch_to_pcon_out;
+  release_output_mutex ();
  
UINT target_code_page = output_to_pcon ?

  GetConsoleOutputCP () : get_ttyp ()->term_code_page;
@@ -2420,8 +2425,6 @@ fhandler_pty_master::close ()
}
  release_output_mutex ();
  master_fwd_thread->terminate_thread ();
- CloseHandle (get_ttyp ()->fwd_done);
- get_ttyp ()->fwd_done = NULL;
}
  }
  
@@ -2903,17 +2906,6 @@ fhandler_pty_slave::set_freeconsole_on_close (bool val)

freeconsole_on_close = val;
  }
  
-void

-fhandler_pty_slave::wait_pcon_fwd (void)
-{
-  acquire_output_mutex (INFINITE);
-  get_ttyp ()->pcon_last_time = GetTickCount ();
-  ResetEvent (get_ttyp ()->fwd_done);
-  release_output_mutex ();
-  while (get_ttyp

[PATCH 10/12] Cygwin: FIFO: allow take_ownership to be interrupted

2020-07-16 Thread Ken Brown via Cygwin-patches

Use cygwait in take_ownership to allow interruption while waiting to
become owner.  Return the cygwait return value or a suitable value to
indicate an error.

raw_read now checks the return value and acts accordingly.
---
 winsup/cygwin/fhandler.h   |  2 +-
 winsup/cygwin/fhandler_fifo.cc | 54 ++
 winsup/cygwin/select.cc| 11 ++-
 3 files changed, 59 insertions(+), 8 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 221c856e6..0e0cfbd71 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1489,7 +1489,7 @@ public:
   void owner_lock () { shmem->owner_lock (); }
   void owner_unlock () { shmem->owner_unlock (); }
 
-  void take_ownership ();
+  DWORD take_ownership ();
   void reading_lock () { shmem->reading_lock (); }
   void reading_unlock () { shmem->reading_unlock (); }
 
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index fd1695f40..30486304f 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -1175,15 +1175,16 @@ fhandler_fifo::raw_write (const void *ptr, size_t len)
   return ret;
 }
 
-/* Called from raw_read and select.cc:peek_fifo. */
-void
+/* Called from raw_read and select.cc:peek_fifo.  Return WAIT_OBJECT_0
+   on success.  */
+DWORD
 fhandler_fifo::take_ownership ()
 {
   owner_lock ();
   if (get_owner () == me)
 {
   owner_unlock ();
-  return;
+  return WAIT_OBJECT_0;
 }
   set_pending_owner (me);
   /* Wake up my fifo_reader_thread. */
@@ -1192,8 +1193,19 @@ fhandler_fifo::take_ownership ()
 /* Wake up owner's fifo_reader_thread. */
 SetEvent (update_needed_evt);
   owner_unlock ();
-  /* The reader threads should now do the transfer.  */
-  WaitForSingleObject (owner_found_evt, INFINITE);
+  /* The reader threads should now do the transfer. */
+  DWORD waitret = cygwait (owner_found_evt, cw_cancel | cw_sig_eintr);
+  owner_lock ();
+  if (waitret == WAIT_OBJECT_0
+  && (get_owner () != me || get_pending_owner ()))
+{
+  /* Something went wrong.  Return WAIT_TIMEOUT, which can't be
+returned by the above cygwait call. */
+  set_pending_owner (null_fr_id);
+  waitret = WAIT_TIMEOUT;
+}
+  owner_unlock ();
+  return waitret;
 }
 
 void __reg3
@@ -1206,7 +1218,37 @@ fhandler_fifo::raw_read (void *in_ptr, size_t& len)
 {
   /* No one else can take ownership while we hold the reading_lock. */
   reading_lock ();
-  take_ownership ();
+  switch (take_ownership ())
+   {
+   case WAIT_OBJECT_0:
+ break;
+   case WAIT_SIGNALED:
+ if (_my_tls.call_signal_handler ())
+   {
+ reading_unlock ();
+ continue;
+   }
+ else
+   {
+ set_errno (EINTR);
+ reading_unlock ();
+ goto errout;
+   }
+ break;
+   case WAIT_CANCELED:
+ reading_unlock ();
+ pthread::static_cancel_self ();
+ break;
+   case WAIT_TIMEOUT:
+ reading_unlock ();
+ debug_printf ("take_ownership returned an unexpected result; retry");
+ continue;
+   default:
+ reading_unlock ();
+ debug_printf ("unknown error while trying to take ownership, %E");
+ goto errout;
+   }
+
   /* Poll the connected clients for input.  Make two passes.  On
 the first pass, just try to read from the client from which
 we last read successfully.  This should minimize
diff --git a/winsup/cygwin/select.cc b/winsup/cygwin/select.cc
index 80d16f2a7..3f3f33fb5 100644
--- a/winsup/cygwin/select.cc
+++ b/winsup/cygwin/select.cc
@@ -867,7 +867,16 @@ peek_fifo (select_record *s, bool from_select)
}
 
   fh->reading_lock ();
-  fh->take_ownership ();
+  if (fh->take_ownership () != WAIT_OBJECT_0)
+   {
+ select_printf ("%s, unable to take ownership", fh->get_name ());
+ fh->reading_unlock ();
+ gotone += s->read_ready = true;
+ if (s->except_selected)
+   gotone += s->except_ready = true;
+ goto out;
+   }
+
   fh->fifo_client_lock ();
   int nconnected = 0;
   for (int i = 0; i < fh->get_nhandlers (); i++)
-- 
2.27.0

[PATCH 12/12] Cygwin: FIFO: update commentary

2020-07-16 Thread Ken Brown via Cygwin-patches

---
 winsup/cygwin/fhandler_fifo.cc | 21 +
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index 30486304f..e9d0187d4 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -52,10 +52,23 @@
  which is mostly idle.  The thread wakes up if that reader might
  need to take ownership.
 
- There is a block of shared memory, accessible to all readers,
- that contains information needed for the owner change process.
- It also contains some locks to prevent races and deadlocks
- between the various threads.
+ There is a block of named shared memory, accessible to all
+ fhandlers for a given FIFO.  It keeps track of the number of open
+ readers and writers; it contains information needed for the owner
+ change process; and it contains some locks to prevent races and
+ deadlocks between the various threads.
+
+ The shared memory is created by the first reader to open the
+ FIFO.  It is opened by subsequent readers and by all writers.  It
+ is destroyed by Windows when the last handle to it is closed.
+
+ If a handle to it somehow remains open after all processes
+ holding file descriptors to the FIFO have closed, the shared
+ memory can persist and be reused with stale data by the next
+ process that opens the FIFO.  So far I've seen this happen only
+ as a result of a bug in the code, but there are some debug_printf
+ statements in fhandler_fifo::open to help detect this if it
+ happens again.
 
  At this writing, I know of only one application (Midnight
  Commander when running under tcsh) that *explicitly* opens two
-- 
2.27.0

[PATCH 05/12] Cygwin: FIFO: improve taking ownership in fifo_reader_thread

2020-07-16 Thread Ken Brown via Cygwin-patches

When a reader takes ownership in fifo_reader_thread, it now goes
directly to the part of the main loop that listens for a connection.
Previously it went back to the beginning of the loop.

Also, if the reader has to delay taking ownership because the previous
owner has not finished updating the shared fifo_client handlers, it
now checks to see if cancel_evt has been set.  Previously it might
have had to spin its wheels unnecessarily only to eventually find that
its thread had been canceled.
---
 winsup/cygwin/fhandler_fifo.cc | 44 ++
 1 file changed, 23 insertions(+), 21 deletions(-)

diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index afe21a468..1fb319fcf 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -462,12 +462,30 @@ fhandler_fifo::fifo_reader_thread_func ()
take_ownership = true;
   else if (cur_owner != me)
idle = true;
-  if (take_ownership)
+  else
+   /* I'm the owner. */
+   goto owner_listen;
+  if (idle)
+   {
+ owner_unlock ();
+ HANDLE w[2] = { owner_needed_evt, cancel_evt };
+ switch (WaitForMultipleObjects (2, w, false, INFINITE))
+   {
+   case WAIT_OBJECT_0:
+ continue;
+   case WAIT_OBJECT_0 + 1:
+ goto canceled;
+   default:
+ api_fatal ("WFMO failed, %E");
+   }
+   }
+  else if (take_ownership)
{
  if (!shared_fc_handler_updated ())
{
  owner_unlock ();
- yield ();
+ if (IsEventSignalled (cancel_evt))
+   goto canceled;
  continue;
}
  else
@@ -478,26 +496,11 @@ fhandler_fifo::fifo_reader_thread_func ()
api_fatal ("Can't update my handlers, %E");
  owner_found ();
  owner_unlock ();
- continue;
+ /* Fall through to owner_listen. */
}
}
-  else if (idle)
-   {
- owner_unlock ();
- HANDLE w[2] = { owner_needed_evt, cancel_evt };
- switch (WaitForMultipleObjects (2, w, false, INFINITE))
-   {
-   case WAIT_OBJECT_0:
- continue;
-   case WAIT_OBJECT_0 + 1:
- goto canceled;
-   default:
- api_fatal ("WFMO failed, %E");
-   }
-   }
-  else
-   {
- /* I'm the owner */
+
+owner_listen:
  fifo_client_lock ();
  cleanup_handlers ();
  if (add_client_handler () < 0)
@@ -590,7 +593,6 @@ fhandler_fifo::fifo_reader_thread_func ()
  fifo_client_unlock ();
  if (cancel)
goto canceled;
-   }
 }
 canceled:
   if (conn_evt)
-- 
2.27.0

[PATCH 04/12] Cygwin: FIFO: reduce I/O interleaving

2020-07-16 Thread Ken Brown via Cygwin-patches

Add a bool member 'last_read' to the fifo_client_handler structure,
which is set to true on a successful read.  This is used by raw_read
as follows.

When raw_read is called, it first locates the writer (if any) for
which last_read is true.  raw_read tries to read from that writer and
returns if there is input available.  Otherwise, it proceeds to poll
all the writers, as before.

The effect of this is that if a writer writes some data that is only
partially read, the next attempt to read will continue to read from
the same writer.  This should reduce the interleaving of output from
different writers.
---
 winsup/cygwin/fhandler.h   |  3 +-
 winsup/cygwin/fhandler_fifo.cc | 55 +-
 2 files changed, 50 insertions(+), 8 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index b5bfdd0b3..221c856e6 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1298,7 +1298,8 @@ struct fifo_client_handler
 {
   HANDLE h;
   fifo_client_connect_state state;
-  fifo_client_handler () : h (NULL), state (fc_unknown) {}
+  bool last_read;  /* true if our last successful read was from this client. */
+  fifo_client_handler () : h (NULL), state (fc_unknown), last_read (false) {}
   void close () { NtClose (h); }
   fifo_client_connect_state set_state ();
 };
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index 3685cc0c2..afe21a468 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -404,6 +404,7 @@ fhandler_fifo::update_my_handlers ()
  goto out;
}
   fc.state = shared_fc_handler[i].state;
+  fc.last_read = shared_fc_handler[i].last_read;
 }
 out:
   set_prev_owner (null_fr_id);
@@ -1200,15 +1201,56 @@ fhandler_fifo::raw_read (void *in_ptr, size_t& len)
   /* No one else can take ownership while we hold the reading_lock. */
   reading_lock ();
   take_ownership ();
-  /* Poll the connected clients for input. */
-  int nconnected = 0;
+  /* Poll the connected clients for input.  Make two passes.  On
+the first pass, just try to read from the client from which
+we last read successfully.  This should minimize
+interleaving of writes from different clients. */
   fifo_client_lock ();
+  /* First pass. */
+  int j;
+  for (j = 0; j < nhandlers; j++)
+   if (fc_handler[j].last_read)
+ break;
+  if (j < nhandlers && fc_handler[j].state >= fc_closing)
+   {
+ NTSTATUS status;
+ IO_STATUS_BLOCK io;
+
+ status = NtReadFile (fc_handler[j].h, NULL, NULL, NULL,
+  , in_ptr, len, NULL, NULL);
+ switch (status)
+   {
+   case STATUS_SUCCESS:
+   case STATUS_BUFFER_OVERFLOW:
+ /* io.Information is supposedly valid in latter case. */
+ if (io.Information > 0)
+   {
+ len = io.Information;
+ fifo_client_unlock ();
+ reading_unlock ();
+ return;
+   }
+ break;
+   case STATUS_PIPE_EMPTY:
+ break;
+   case STATUS_PIPE_BROKEN:
+ fc_handler[j].state = fc_disconnected;
+ break;
+   default:
+ debug_printf ("NtReadFile status %y", status);
+ fc_handler[j].state = fc_error;
+ break;
+   }
+ fc_handler[j].last_read = false;
+   }
+
+  /* Second pass. */
+  int nconnected = 0;
   for (int i = 0; i < nhandlers; i++)
if (fc_handler[i].state >= fc_closing)
  {
NTSTATUS status;
IO_STATUS_BLOCK io;
-   size_t nbytes = 0;
 
nconnected++;
status = NtReadFile (fc_handler[i].h, NULL, NULL, NULL,
@@ -1217,11 +1259,10 @@ fhandler_fifo::raw_read (void *in_ptr, size_t& len)
  {
  case STATUS_SUCCESS:
  case STATUS_BUFFER_OVERFLOW:
-   /* io.Information is supposedly valid. */
-   nbytes = io.Information;
-   if (nbytes > 0)
+   if (io.Information > 0)
  {
-   len = nbytes;
+   len = io.Information;
+   fc_handler[i].last_read = true;
fifo_client_unlock ();
reading_unlock ();
return;
-- 
2.27.0

[PATCH 07/12] Cygwin: FIFO: make certain errors non-fatal

2020-07-16 Thread Ken Brown via Cygwin-patches

If update_my_handlers fails to duplicate one or more handles, just
mark the corresponding handlers as being in an error state.

But if update_my_handlers is unable to open the process of the
previous owner, it's likely that something serious has gone wrong, so
we continue to make that a fatal error.
---
 winsup/cygwin/fhandler_fifo.cc | 21 ++---
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index 69dda0811..91a276ee9 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -383,11 +383,7 @@ fhandler_fifo::update_my_handlers ()
   else
 prev_proc = OpenProcess (PROCESS_DUP_HANDLE, false, prev.winpid);
   if (!prev_proc)
-{
-  debug_printf ("Can't open process of previous owner, %E");
-  __seterrno ();
-  goto out;
-}
+api_fatal ("Can't open process of previous owner, %E");
 
   for (int i = 0; i < get_shared_nhandlers (); i++)
 {
@@ -399,14 +395,17 @@ fhandler_fifo::update_my_handlers ()
!close_on_exec (), DUPLICATE_SAME_ACCESS))
{
  debug_printf ("Can't duplicate handle of previous owner, %E");
- --nhandlers;
  __seterrno ();
- goto out;
+ fc.state = fc_error;
+ fc.last_read = false;
+ ret = -1;
+   }
+  else
+   {
+ fc.state = shared_fc_handler[i].state;
+ fc.last_read = shared_fc_handler[i].last_read;
}
-  fc.state = shared_fc_handler[i].state;
-  fc.last_read = shared_fc_handler[i].last_read;
 }
-out:
   set_prev_owner (null_fr_id);
   return ret;
 }
@@ -493,7 +492,7 @@ fhandler_fifo::fifo_reader_thread_func ()
  set_owner (me);
  set_pending_owner (null_fr_id);
  if (update_my_handlers () < 0)
-   api_fatal ("Can't update my handlers, %E");
+   debug_printf ("error updating my handlers, %E");
  owner_found ();
  owner_unlock ();
  /* Fall through to owner_listen. */
-- 
2.27.0

[PATCH 01/12] Cygwin: FIFO: fix problems finding new owner

2020-07-16 Thread Ken Brown via Cygwin-patches

When the owning reader closes and there are still readers open, the
owner needs to wait for a new owner to be found before closing its
fifo_client handlers.  This involves a loop in which dec_nreaders is
called at the beginning and inc_nreaders is called at the end.  Any
other reader that tries to access shmem->_nreaders during this loop
will therefore get an inaccurate answer.

Fix this by adding an nreaders method and using it instead of
dec_nreaders and inc_nreaders.  Also add nreaders_lock to control
access to the shmem->_nreaders.

Make various other changes to improve the reliability of finding a new
owner.
---
 winsup/cygwin/fhandler.h   |  8 +++-
 winsup/cygwin/fhandler_fifo.cc | 86 +-
 2 files changed, 61 insertions(+), 33 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 7a28adc16..cf6daea06 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1328,7 +1328,7 @@ class fifo_shmem_t
 {
   LONG _nreaders;
   fifo_reader_id_t _owner, _prev_owner, _pending_owner;
-  af_unix_spinlock_t _owner_lock, _reading_lock, _reader_opening_lock;
+  af_unix_spinlock_t _owner_lock, _reading_lock, _reader_opening_lock, 
_nreaders_lock;
 
   /* Info about shared memory block used for temporary storage of the
  owner's fc_handler list. */
@@ -1336,6 +1336,7 @@ class fifo_shmem_t
 _sh_fc_handler_updated;
 
 public:
+  int nreaders () const { return (int) _nreaders; }
   int inc_nreaders () { return (int) InterlockedIncrement (&_nreaders); }
   int dec_nreaders () { return (int) InterlockedDecrement (&_nreaders); }
 
@@ -1352,6 +1353,8 @@ public:
   void reading_unlock () { _reading_lock.unlock (); }
   void reader_opening_lock () { _reader_opening_lock.lock (); }
   void reader_opening_unlock () { _reader_opening_lock.unlock (); }
+  void nreaders_lock () { _nreaders_lock.lock (); }
+  void nreaders_unlock () { _nreaders_lock.unlock (); }
 
   int get_shared_nhandlers () const { return (int) _sh_nhandlers; }
   void set_shared_nhandlers (int n) { InterlockedExchange (&_sh_nhandlers, n); 
}
@@ -1420,8 +1423,11 @@ class fhandler_fifo: public fhandler_base
   int reopen_shared_fc_handler ();
   int remap_shared_fc_handler (size_t);
 
+  int nreaders () const { return shmem->nreaders (); }
   int inc_nreaders () { return shmem->inc_nreaders (); }
   int dec_nreaders () { return shmem->dec_nreaders (); }
+  void nreaders_lock () { shmem->nreaders_lock (); }
+  void nreaders_unlock () { shmem->nreaders_unlock (); }
 
   fifo_reader_id_t get_prev_owner () const { return shmem->get_prev_owner (); }
   void set_prev_owner (fifo_reader_id_t fr_id)
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index 3d34cdfab..2d4f7a97e 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -371,6 +371,8 @@ fhandler_fifo::record_connection (fifo_client_handler& fc,
 int
 fhandler_fifo::update_my_handlers ()
 {
+  int ret = 0;
+
   close_all_handlers ();
   fifo_reader_id_t prev = get_prev_owner ();
   if (!prev)
@@ -387,7 +389,7 @@ fhandler_fifo::update_my_handlers ()
 {
   debug_printf ("Can't open process of previous owner, %E");
   __seterrno ();
-  return -1;
+  goto out;
 }
 
   for (int i = 0; i < get_shared_nhandlers (); i++)
@@ -402,11 +404,13 @@ fhandler_fifo::update_my_handlers ()
  debug_printf ("Can't duplicate handle of previous owner, %E");
  --nhandlers;
  __seterrno ();
- return -1;
+ goto out;
}
   fc.state = shared_fc_handler[i].state;
 }
-  return 0;
+out:
+  set_prev_owner (null_fr_id);
+  return ret;
 }
 
 int
@@ -1414,41 +1418,59 @@ fhandler_fifo::close ()
 {
   if (reader)
 {
-  /* If we're the owner, try to find a new owner. */
-  bool find_new_owner = false;
+  /* If we're the owner, we can't close our fc_handlers if a new
+owner might need to duplicate them. */
+  bool close_fc_ok = false;
 
   cancel_reader_thread ();
-  owner_lock ();
-  if (get_owner () == me)
+  nreaders_lock ();
+  if (dec_nreaders () == 0)
{
- find_new_owner = true;
+ close_fc_ok = true;
+ ResetEvent (read_ready);
+ ResetEvent (owner_needed_evt);
+ ResetEvent (owner_found_evt);
  set_owner (null_fr_id);
- set_prev_owner (me);
- owner_needed ();
+ set_prev_owner (null_fr_id);
+ set_pending_owner (null_fr_id);
+ set_shared_nhandlers (0);
}
-  owner_unlock ();
-  if (dec_nreaders () == 0)
-   ResetEvent (read_ready);
-  else if (find_new_owner && !IsEventSignalled (owner_found_evt))
+  else
{
- bool found = false;
- do
-   if (dec_nreaders () >= 0)
- {
-   /* There's still another reader open. */
-   if (WaitForSingleObject (owner_found_evt, 1) == WAIT_OBJECT_0)
-

[PATCH 03/12] Cygwin: fhandler_fifo::hit_eof: improve reliability

2020-07-16 Thread Ken Brown via Cygwin-patches

Use the writer count introduced in the previous commit to help detect
EOF.  Drop the maybe_eof method, which is no longer needed.
---
 winsup/cygwin/fhandler.h   |  7 +++
 winsup/cygwin/fhandler_fifo.cc | 26 ++
 winsup/cygwin/select.cc|  3 +--
 3 files changed, 6 insertions(+), 30 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index f034a110e..b5bfdd0b3 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1392,7 +1392,6 @@ class fhandler_fifo: public fhandler_base
 
   UNICODE_STRING pipe_name;
   WCHAR pipe_name_buf[CYGWIN_FIFO_PIPE_NAME_LEN + 1];
-  bool _maybe_eof;
   fifo_client_handler *fc_handler; /* Dynamically growing array. */
   int shandlers;   /* Size (capacity) of the array. */
   int nhandlers;   /* Number of elements in the array. */
@@ -1473,9 +1472,9 @@ class fhandler_fifo: public fhandler_base
 
 public:
   fhandler_fifo ();
-  bool hit_eof ();
-  bool maybe_eof () const { return _maybe_eof; }
-  void maybe_eof (bool val) { _maybe_eof = val; }
+  /* Called if we appear to be at EOF after polling fc_handlers. */
+  bool hit_eof () const
+  { return !nwriters () && !IsEventSignalled (writer_opening); }
   int get_nhandlers () const { return nhandlers; }
   fifo_client_handler _fc_handler (int i) { return fc_handler[i]; }
   PUNICODE_STRING get_pipe_name ();
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index 26b24d019..3685cc0c2 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -87,7 +87,7 @@ fhandler_fifo::fhandler_fifo ():
   fhandler_base (),
   read_ready (NULL), write_ready (NULL), writer_opening (NULL),
   owner_needed_evt (NULL), owner_found_evt (NULL), update_needed_evt (NULL),
-  cancel_evt (NULL), thr_sync_evt (NULL), _maybe_eof (false),
+  cancel_evt (NULL), thr_sync_evt (NULL),
   fc_handler (NULL), shandlers (0), nhandlers (0),
   reader (false), writer (false), duplexer (false),
   max_atomic_write (DEFAULT_PIPEBUFSIZE),
@@ -361,8 +361,6 @@ fhandler_fifo::record_connection (fifo_client_handler& fc,
  fifo_client_connect_state s)
 {
   fc.state = s;
-  maybe_eof (false);
-  ResetEvent (writer_opening);
   set_pipe_non_blocking (fc.h, true);
 }
 
@@ -1173,25 +1171,6 @@ fhandler_fifo::raw_write (const void *ptr, size_t len)
   return ret;
 }
 
-/* A reader is at EOF if the pipe is empty and no writers are open.
-   hit_eof is called by raw_read and select.cc:peek_fifo if it appears
-   that we are at EOF after polling the fc_handlers.  We recheck this
-   in case a writer opened while we were polling.  */
-bool
-fhandler_fifo::hit_eof ()
-{
-  bool ret = maybe_eof () && !IsEventSignalled (writer_opening);
-  if (ret)
-{
-  yield ();
-  /* Wait for the reader thread to finish recording any connection. */
-  fifo_client_lock ();
-  fifo_client_unlock ();
-  ret = maybe_eof ();
-}
-  return ret;
-}
-
 /* Called from raw_read and select.cc:peek_fifo. */
 void
 fhandler_fifo::take_ownership ()
@@ -1261,9 +1240,8 @@ fhandler_fifo::raw_read (void *in_ptr, size_t& len)
break;
  }
  }
-  maybe_eof (!nconnected && !IsEventSignalled (writer_opening));
   fifo_client_unlock ();
-  if (maybe_eof () && hit_eof ())
+  if (!nconnected && hit_eof ())
{
  reading_unlock ();
  len = 0;
diff --git a/winsup/cygwin/select.cc b/winsup/cygwin/select.cc
index 9ae567c38..80d16f2a7 100644
--- a/winsup/cygwin/select.cc
+++ b/winsup/cygwin/select.cc
@@ -883,9 +883,8 @@ peek_fifo (select_record *s, bool from_select)
goto out;
  }
  }
-  fh->maybe_eof (!nconnected);
   fh->fifo_client_unlock ();
-  if (fh->maybe_eof () && fh->hit_eof ())
+  if (!nconnected && fh->hit_eof ())
{
  select_printf ("read: %s, saw EOF", fh->get_name ());
  gotone += s->read_ready = true;
-- 
2.27.0

[PATCH 06/12] Cygwin: FIFO: fix indentation

2020-07-16 Thread Ken Brown via Cygwin-patches

---
 winsup/cygwin/fhandler_fifo.cc | 168 -
 1 file changed, 84 insertions(+), 84 deletions(-)

diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index 1fb319fcf..69dda0811 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -501,98 +501,98 @@ fhandler_fifo::fifo_reader_thread_func ()
}
 
 owner_listen:
- fifo_client_lock ();
- cleanup_handlers ();
- if (add_client_handler () < 0)
-   api_fatal ("Can't add a client handler, %E");
-
- /* Listen for a writer to connect to the new client handler. */
- fifo_client_handler& fc = fc_handler[nhandlers - 1];
- fifo_client_unlock ();
- shared_fc_handler_updated (false);
- owner_unlock ();
- NTSTATUS status;
- IO_STATUS_BLOCK io;
- bool cancel = false;
- bool update = false;
+  fifo_client_lock ();
+  cleanup_handlers ();
+  if (add_client_handler () < 0)
+   api_fatal ("Can't add a client handler, %E");
 
- status = NtFsControlFile (fc.h, conn_evt, NULL, NULL, ,
-   FSCTL_PIPE_LISTEN, NULL, 0, NULL, 0);
- if (status == STATUS_PENDING)
-   {
- HANDLE w[3] = { conn_evt, update_needed_evt, cancel_evt };
- switch (WaitForMultipleObjects (3, w, false, INFINITE))
-   {
-   case WAIT_OBJECT_0:
- status = io.Status;
- debug_printf ("NtFsControlFile STATUS_PENDING, then %y",
-   status);
- break;
-   case WAIT_OBJECT_0 + 1:
- status = STATUS_WAIT_1;
- update = true;
- break;
-   case WAIT_OBJECT_0 + 2:
- status = STATUS_THREAD_IS_TERMINATING;
- cancel = true;
- update = true;
- break;
-   default:
- api_fatal ("WFMO failed, %E");
-   }
-   }
- else
-   debug_printf ("NtFsControlFile status %y, no STATUS_PENDING",
- status);
- HANDLE ph = NULL;
- NTSTATUS status1;
+  /* Listen for a writer to connect to the new client handler. */
+  fifo_client_handler& fc = fc_handler[nhandlers - 1];
+  fifo_client_unlock ();
+  shared_fc_handler_updated (false);
+  owner_unlock ();
+  NTSTATUS status;
+  IO_STATUS_BLOCK io;
+  bool cancel = false;
+  bool update = false;
 
- fifo_client_lock ();
- switch (status)
+  status = NtFsControlFile (fc.h, conn_evt, NULL, NULL, ,
+   FSCTL_PIPE_LISTEN, NULL, 0, NULL, 0);
+  if (status == STATUS_PENDING)
+   {
+ HANDLE w[3] = { conn_evt, update_needed_evt, cancel_evt };
+ switch (WaitForMultipleObjects (3, w, false, INFINITE))
{
-   case STATUS_SUCCESS:
-   case STATUS_PIPE_CONNECTED:
- record_connection (fc);
+   case WAIT_OBJECT_0:
+ status = io.Status;
+ debug_printf ("NtFsControlFile STATUS_PENDING, then %y",
+   status);
  break;
-   case STATUS_PIPE_CLOSING:
- record_connection (fc, fc_closing);
+   case WAIT_OBJECT_0 + 1:
+ status = STATUS_WAIT_1;
+ update = true;
  break;
-   case STATUS_THREAD_IS_TERMINATING:
-   case STATUS_WAIT_1:
- /* Try to connect a bogus client.  Otherwise fc is still
-listening, and the next connection might not get recorded. */
- status1 = open_pipe (ph);
- WaitForSingleObject (conn_evt, INFINITE);
- if (NT_SUCCESS (status1))
-   /* Bogus cilent connected. */
-   delete_client_handler (nhandlers - 1);
- else
-   /* Did a real client connect? */
-   switch (io.Status)
- {
- case STATUS_SUCCESS:
- case STATUS_PIPE_CONNECTED:
-   record_connection (fc);
-   break;
- case STATUS_PIPE_CLOSING:
-   record_connection (fc, fc_closing);
-   break;
- default:
-   debug_printf ("NtFsControlFile status %y after failing to 
connect bogus client or real client", io.Status);
-   fc.state = fc_unknown;
-   break;
- }
+   case WAIT_OBJECT_0 + 2:
+ status = STATUS_THREAD_IS_TERMINATING;
+ cancel = true;
+ update = true;
  break;
default:
- break;
+ api_fatal ("WFMO failed, %E");
}
- if (ph)
-   NtClose (ph);
- if (update && update_shared_handlers () < 0)
-

[PATCH 00/12] FIFO: fix multiple reader support

2020-07-16 Thread Ken Brown via Cygwin-patches

There were several flaws in my previous attempt to add support for
explicitly opening a FIFO multiple times for reading.  (By
"explicitly" I mean by calling open rather than by calling
fork/exec/dup.)  See

  https://sourceware.org/pipermail/cygwin/2020-July/245456.html

for one indication of problems

The most important flaw was that I tried to use an indirect,
unreliable method for determining whether there are writers open.
This is fixed in the second patch of this series by adding a member
'_nwriters' to struct fifo_shmem_t, which counts the number of open
writers.

We now have to give writers access to the shared memory as well as
readers, so that they can increment _nwriters in open/fork/exec/dup
and decrement it in close.

The other patches contain miscellaneous fixes/improvements.

Ken Brown (12):
  Cygwin: FIFO: fix problems finding new owner
  Cygwin: FIFO: keep a writer count in shared memory
  Cygwin: fhandler_fifo::hit_eof: improve reliability
  Cygwin: FIFO: reduce I/O interleaving
  Cygwin: FIFO: improve taking ownership in fifo_reader_thread
  Cygwin: FIFO: fix indentation
  Cygwin: FIFO: make certain errors non-fatal
  Cygwin: FIFO: add missing lock
  Cygwin: fhandler_fifo::take_ownership: don't set event unnecessarily
  Cygwin: FIFO: allow take_ownership to be interrupted
  Cygwin: FIFO: clean up
  Cygwin: FIFO: update commentary

 winsup/cygwin/fhandler.h   |  55 +--
 winsup/cygwin/fhandler_fifo.cc | 725 ++---
 winsup/cygwin/select.cc|  14 +-
 3 files changed, 433 insertions(+), 361 deletions(-)

-- 
2.27.0

[PATCH 08/12] Cygwin: FIFO: add missing lock

2020-07-16 Thread Ken Brown via Cygwin-patches

---
 winsup/cygwin/fhandler_fifo.cc | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index 91a276ee9..b6e172ddc 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -422,7 +422,9 @@ fhandler_fifo::update_shared_handlers ()
   set_shared_nhandlers (nhandlers);
   memcpy (shared_fc_handler, fc_handler, nhandlers * sizeof (fc_handler[0]));
   shared_fc_handler_updated (true);
+  owner_lock ();
   set_prev_owner (me);
+  owner_unlock ();
   return 0;
 }
 
-- 
2.27.0

[PATCH 02/12] Cygwin: FIFO: keep a writer count in shared memory

2020-07-16 Thread Ken Brown via Cygwin-patches

When a reader opens, it needs to block if there are no writers open
(unless is is opened with O_NONBLOCK).  This is easy for the first
reader to test, since it can just wait for a writer to signal that it
is open (via the write_ready event).  But when a second reader wants
to open, all writers might have closed.

To check this, use a new '_nwriters' member of struct fifo_shmem_t,
which keeps track of the number of open writers.  This should be more
reliable than the previous method.

Add nwriters_lock to control access to shmem->_nwriters, and remove
reader_opening_lock, which is no longer needed.

Previously only readers had access to the shared memory, but now
writers access it too so that they can increment _nwriters during
open/dup/fork/exec and decrement it during close.

Add an optional 'only_open' argument to create_shmem for use by
writers, which only open the shared memory rather than first trying to
create it.  Since writers don't need to access the shared memory until
they have successfully connected to a pipe instance, they can safely
assume that a reader has already created the shared memory.

For debugging purposes, change create_shmem to return 1 instead of 0
when a reader successfully opens the shared memory after finding that
it had already been created.

Remove check_write_ready_evt, write_ready_ok_evt, and
check_write_ready(), which are no longer needed.

When opening a writer and looping to try to get a connection, recheck
read_ready at the top of the loop since the number of readers might
have changed.

To slightly speed up the process of opening the first reader, take
ownership immediately rather than waiting for the fifo_reader_thread
to handle it.
---
 winsup/cygwin/fhandler.h   |  27 ++--
 winsup/cygwin/fhandler_fifo.cc | 263 ++---
 2 files changed, 124 insertions(+), 166 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index cf6daea06..f034a110e 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1323,12 +1323,14 @@ struct fifo_reader_id_t
   }
 };
 
-/* Info needed by all readers of a FIFO, stored in named shared memory. */
+/* Info needed by all fhandlers for a given FIFO, stored in named
+   shared memory.  This is mostly for readers, but writers need access
+   in order to update the count of open writers. */
 class fifo_shmem_t
 {
-  LONG _nreaders;
+  LONG _nreaders, _nwriters;
   fifo_reader_id_t _owner, _prev_owner, _pending_owner;
-  af_unix_spinlock_t _owner_lock, _reading_lock, _reader_opening_lock, 
_nreaders_lock;
+  af_unix_spinlock_t _owner_lock, _reading_lock, _nreaders_lock, 
_nwriters_lock;
 
   /* Info about shared memory block used for temporary storage of the
  owner's fc_handler list. */
@@ -1339,6 +1341,9 @@ public:
   int nreaders () const { return (int) _nreaders; }
   int inc_nreaders () { return (int) InterlockedIncrement (&_nreaders); }
   int dec_nreaders () { return (int) InterlockedDecrement (&_nreaders); }
+  int nwriters () const { return (int) _nwriters; }
+  int inc_nwriters () { return (int) InterlockedIncrement (&_nwriters); }
+  int dec_nwriters () { return (int) InterlockedDecrement (&_nwriters); }
 
   fifo_reader_id_t get_owner () const { return _owner; }
   void set_owner (fifo_reader_id_t fr_id) { _owner = fr_id; }
@@ -1351,10 +1356,10 @@ public:
   void owner_unlock () { _owner_lock.unlock (); }
   void reading_lock () { _reading_lock.lock (); }
   void reading_unlock () { _reading_lock.unlock (); }
-  void reader_opening_lock () { _reader_opening_lock.lock (); }
-  void reader_opening_unlock () { _reader_opening_lock.unlock (); }
   void nreaders_lock () { _nreaders_lock.lock (); }
   void nreaders_unlock () { _nreaders_lock.unlock (); }
+  void nwriters_lock () { _nwriters_lock.lock (); }
+  void nwriters_unlock () { _nwriters_lock.unlock (); }
 
   int get_shared_nhandlers () const { return (int) _sh_nhandlers; }
   void set_shared_nhandlers (int n) { InterlockedExchange (&_sh_nhandlers, n); 
}
@@ -1380,8 +1385,6 @@ class fhandler_fifo: public fhandler_base
   HANDLE owner_needed_evt;  /* The owner is closing. */
   HANDLE owner_found_evt;   /* A new owner has taken over. */
   HANDLE update_needed_evt; /* shared_fc_handler needs updating. */
-  HANDLE check_write_ready_evt; /* write_ready needs to be checked. */
-  HANDLE write_ready_ok_evt;/* check_write_ready is done. */
 
   /* Handles to non-shared events needed for fifo_reader_threads. */
   HANDLE cancel_evt;/* Signal thread to terminate. */
@@ -1417,7 +1420,7 @@ class fhandler_fifo: public fhandler_base
   void record_connection (fifo_client_handler&,
  fifo_client_connect_state = fc_connected);
 
-  int create_shmem ();
+  int create_shmem (bool only_open = false);
   int reopen_shmem ();
   int create_shared_fc_handler ();
   int reopen_shared_fc_handler ();
@@ -1426,8 +1429,13 @@ class fhandler_fifo: public fhandler_base
   int nreaders

[PATCH 09/12] Cygwin: fhandler_fifo::take_ownership: don't set event unnecessarily

2020-07-16 Thread Ken Brown via Cygwin-patches

Don't set update_needed_evt if there's currently no owner.  This will
cause unnecessary churn once I'm the owner and am listening for
connections.
---
 winsup/cygwin/fhandler_fifo.cc | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index b6e172ddc..fd1695f40 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -1186,8 +1186,11 @@ fhandler_fifo::take_ownership ()
   return;
 }
   set_pending_owner (me);
+  /* Wake up my fifo_reader_thread. */
   owner_needed ();
-  SetEvent (update_needed_evt);
+  if (get_owner ())
+/* Wake up owner's fifo_reader_thread. */
+SetEvent (update_needed_evt);
   owner_unlock ();
   /* The reader threads should now do the transfer.  */
   WaitForSingleObject (owner_found_evt, INFINITE);
-- 
2.27.0

[PATCH 11/12] Cygwin: FIFO: clean up

2020-07-16 Thread Ken Brown via Cygwin-patches

Remove the fhandler_fifo::get_me method, which is no longer used.
Make the methods get_owner, set_owner, owner_lock, and owner_unlock
private.
---
 winsup/cygwin/fhandler.h | 10 --
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 0e0cfbd71..60bd27e00 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1437,6 +1437,8 @@ class fhandler_fifo: public fhandler_base
   void nwriters_lock () { shmem->nwriters_lock (); }
   void nwriters_unlock () { shmem->nwriters_unlock (); }
 
+  fifo_reader_id_t get_owner () const { return shmem->get_owner (); }
+  void set_owner (fifo_reader_id_t fr_id) { shmem->set_owner (fr_id); }
   fifo_reader_id_t get_prev_owner () const { return shmem->get_prev_owner (); }
   void set_prev_owner (fifo_reader_id_t fr_id)
   { shmem->set_prev_owner (fr_id); }
@@ -1444,6 +1446,8 @@ class fhandler_fifo: public fhandler_base
   { return shmem->get_pending_owner (); }
   void set_pending_owner (fifo_reader_id_t fr_id)
   { shmem->set_pending_owner (fr_id); }
+  void owner_lock () { shmem->owner_lock (); }
+  void owner_unlock () { shmem->owner_unlock (); }
 
   void owner_needed ()
   {
@@ -1483,12 +1487,6 @@ public:
   void fifo_client_lock () { _fifo_client_lock.lock (); }
   void fifo_client_unlock () { _fifo_client_lock.unlock (); }
 
-  fifo_reader_id_t get_me () const { return me; }
-  fifo_reader_id_t get_owner () const { return shmem->get_owner (); }
-  void set_owner (fifo_reader_id_t fr_id) { shmem->set_owner (fr_id); }
-  void owner_lock () { shmem->owner_lock (); }
-  void owner_unlock () { shmem->owner_unlock (); }
-
   DWORD take_ownership ();
   void reading_lock () { shmem->reading_lock (); }
   void reading_unlock () { shmem->reading_unlock (); }
-- 
2.27.0

[PATCH] Cygwin: mmap: fix mapping beyond EOF on 64 bit

2020-07-20 Thread Ken Brown via Cygwin-patches

Commit 605bdcd410384dda6db66b9b8cd19e863702e1bb enabled mapping beyond
EOF in 64 bit environments.  But the variable 'orig_len' did not get
rounded up to a multiple of 64K.  This rounding was done on 32 bit
only.  Fix this by rounding up orig_len on 64 bit, in the same place
where 'len' is rounded up.

One consequence of this bug is that orig_len could be slightly smaller
than len.  Since these are both unsigned values, the statement
'orig_len -= len' would then cause orig_len to be huge, and mmap would
fail with errno EFBIG.

I observed this failure while debugging the problem reported in

  https://sourceware.org/pipermail/cygwin/2020-July/245557.html.

The failure can be seen by running the test case in that report under
gdb or strace.
---
 winsup/cygwin/mmap.cc | 1 +
 1 file changed, 1 insertion(+)

diff --git a/winsup/cygwin/mmap.cc b/winsup/cygwin/mmap.cc
index feb9e5d0e..a08d00f83 100644
--- a/winsup/cygwin/mmap.cc
+++ b/winsup/cygwin/mmap.cc
@@ -1144,6 +1144,7 @@ go_ahead:
 ends in, but there's nothing at all we can do about that. */
 #ifdef __x86_64__
   len = roundup2 (len, wincap.allocation_granularity ());
+  orig_len = roundup2 (orig_len, wincap.allocation_granularity ());
 #else
   len = roundup2 (len, wincap.is_wow64 () ? wincap.allocation_granularity 
()
  : wincap.page_size ());
-- 
2.27.0

Re: [PATCH] Cygwin: mmap: fix mapping beyond EOF on 64 bit

2020-07-20 Thread Ken Brown via Cygwin-patches


On 7/20/2020 10:23 AM, Corinna Vinschen wrote:

On Jul 20 09:34, Ken Brown via Cygwin-patches wrote:

Commit 605bdcd410384dda6db66b9b8cd19e863702e1bb enabled mapping beyond
EOF in 64 bit environments.  But the variable 'orig_len' did not get
rounded up to a multiple of 64K.  This rounding was done on 32 bit
only.  Fix this by rounding up orig_len on 64 bit, in the same place
where 'len' is rounded up.

One consequence of this bug is that orig_len could be slightly smaller
than len.  Since these are both unsigned values, the statement
'orig_len -= len' would then cause orig_len to be huge, and mmap would
fail with errno EFBIG.

I observed this failure while debugging the problem reported in

   https://sourceware.org/pipermail/cygwin/2020-July/245557.html.

The failure can be seen by running the test case in that report under
gdb or strace.
---
  winsup/cygwin/mmap.cc | 1 +
  1 file changed, 1 insertion(+)

diff --git a/winsup/cygwin/mmap.cc b/winsup/cygwin/mmap.cc
index feb9e5d0e..a08d00f83 100644
--- a/winsup/cygwin/mmap.cc
+++ b/winsup/cygwin/mmap.cc
@@ -1144,6 +1144,7 @@ go_ahead:
 ends in, but there's nothing at all we can do about that. */
  #ifdef __x86_64__
len = roundup2 (len, wincap.allocation_granularity ());
+  orig_len = roundup2 (orig_len, wincap.allocation_granularity ());


Wouldn't it be simpler to just check for

-  if (orig_len - len)
+  if (orig_len > len)

in the code following this #if/#else/#endif snippet?


I don't think so, because we also want to use the rounded-up value of orig_len 
further down when we set sigbus_page_len.


Ken

1 2 >

1 - 100 of 157 matches

Mail list logo