On Sun, 12 Sep 2021 17:48:49 +0900
Takashi Yano wrote:
> On Sat, 11 Sep 2021 09:12:02 -0400
> Ken Brown wrote:
> > On 9/10/2021 10:35 PM, Takashi Yano wrote:
> > > On Fri, 10 Sep 2021 22:17:21 -0400
> > > Ken Brown wrote:
> > >> On 9/10/2021 6:57 PM, Takashi Yano wrote:
> > >>> On Fri, 10 Sep 2021 11:17:58 -0400
> > >>> Ken Brown wrote:
> > >>>> I've rerun your test with the latest version, and the test results are similar.
> > >>>>     I've also run a suite of fifo tests that I've accumulated, and they all pass
> > >>>> also, so I pushed your patch.
> > >>>>
> > >>>> I think we're in pretty good shape now.  The only detail remaining, AFAIK, is
> > >>>> how to best avoid a deadlock if the pipe has been created by a non-Cygwin
> > >>>> process.  I've proposed a timeout, but maybe there's a better idea.
> > >>>
> > >>> I am not pretty sure what is the problem, but is not the following
> > >>> patch enough?
> > >>>
> > >>> diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
> > >>> index d309be2f7..13fba9a14 100644
> > >>> --- a/winsup/cygwin/fhandler.h
> > >>> +++ b/winsup/cygwin/fhandler.h
> > >>> @@ -1205,6 +1205,7 @@ public:
> > >>>      select_record *select_except (select_stuff *);
> > >>>      char *get_proc_fd_name (char *buf);
> > >>>      int open (int flags, mode_t mode = 0);
> > >>> +  void open_setup (int flags);
> > >>>      void fixup_after_fork (HANDLE);
> > >>>      int dup (fhandler_base *child, int);
> > >>>      int close ();
> > >>> diff --git a/winsup/cygwin/fhandler_pipe.cc b/winsup/cygwin/fhandler_pipe.cc
> > >>> index 6994a5dce..d84e6ad84 100644
> > >>> --- a/winsup/cygwin/fhandler_pipe.cc
> > >>> +++ b/winsup/cygwin/fhandler_pipe.cc
> > >>> @@ -191,6 +191,17 @@ out:
> > >>>      return 0;
> > >>>    }
> > >>>
> > >>> +void
> > >>> +fhandler_pipe::open_setup (int flags)
> > >>> +{
> > >>> +  fhandler_base::open_setup (flags);
> > >>> +  if (get_dev () == FH_PIPER && !read_mtx)
> > >>> +    {
> > >>> +      SECURITY_ATTRIBUTES *sa = sec_none_cloexec (flags);
> > >>> +      read_mtx = CreateMutexW (sa, FALSE, NULL);
> > >>> +    }
> > >>> +}
> > >>> +
> > >>>    off_t
> > >>>    fhandler_pipe::lseek (off_t offset, int whence)
> > >>>    {
> > >>>
> > >>>
> > >>> AFAIK, another problem remaining is:
> > >>>
> > >>> On Mon, 6 Sep 2021 14:49:55 +0200
> > >>> Corinna Vinschen wrote:
> > >>>> - What about calling select for writing on pipes read by non-Cygwin
> > >>>>     processes?  In that case, we still can't rely on WriteQuotaAvailable,
> > >>>>     just as before.
> > >>
> > >> This is the problem I was talking about.  In this case the non-Cygwin process
> > >> might have a large pending read, so that the Cygwin process calling select on
> > >> the write side will see WriteQuotaAvailable == 0.  This could lead to a deadlock
> > >> with the Cygwin process waiting for write ready while the non-Cygwin process is
> > >> blocked trying to read.
> > > 
> > > Then, the above patch is for another issue.
> > > The problem happes when:
> > > 1) Start command prompt.
> > > 2) Run 'echo AAAAAAAAAAAA | \cygwin64\bin\cat
> > > This causes hang up in cat. In this case, pipe is created by cmd.exe.
> > > Therefore, read_mtx is not created.
> > 
> > Confirmed, and your patch fixes it.  Maybe you should check for error in the 
> > call to CreateMutexW and print a debug message in that case.
> > 
> > >> My suggestion is that we impose a timeout in this situation, after which select
> > >> reports write ready.
> > > 
> > > Keeping read handle in write pipe (Corinna's query_hdl) causes problem
> > > that write side cannot detect close on read side.
> > > Is it possible to open read handle temporally when pipe_data_available()
> > > is called?
> > 
> > That would be nice, but I have no idea how you could do that.
> 
> Hmm. Then, what about PoC code attached? This returns to Corinna's
> query_hdl, and counts read/write handles to detect closing reader side.
> 
> If the number of read handles is equal to number of write handles,
> only the pairs of write handle and query_hdl are alive. So, read pipe
> supposed to be closed.
> 
> This patch depends another patch I posted a few hours ago.

Revised a bit.

-- 
Takashi Yano <takashi.yano@nifty.ne.jp>