On Mon, 13 Sep 2021 22:15:25 +0200 Corinna Vinschen wrote: > On Sep 14 04:37, Takashi Yano wrote: > > On Mon, 13 Sep 2021 20:32:33 +0200 > > Corinna Vinschen wrote: > > > I don't understand why calling fork_fixup on query_hdl should depend > > > on the handle count. If you duplicate a writer, you always have to > > > duplicate query_hdl as well to keep the count, no? Inheritence is > > > handled by the O_CLOEXEC flag and fork_fixup will do the right thing. > > > > I thought so, however, counting query_hdl cannot work as expected > > without this check. The number of query_hdl opend seems to exceed > > the number of writer. > > If the write handle as well as the query handle are both opened, > duplicated and closed in the same manner, they should never have a > different count, unless the write side is inherited by a non-Cygwin > client. > > > There seems to be the case that handle is already inherited here > > without fork_fixup. Any idea? > > That should depend on the O_CLOEXEC setting, but identically for > all handles in the fhandler. I found the cause. set_close_on_exec() in fhandler_pipe is missing. set_no_inheritance() calls for all adjunct handles are necessary. > I pushed two more patches to topic/pipe in terms of inheritence, > maybe that gives a clue? I attached two additional patch for this issue. > > > > + if (!DuplicateHandle (GetCurrentProcess (), r, > > > > + GetCurrentProcess (), &fhs[1]->query_hdl, > > > > + GENERIC_READ, !(mode & O_CLOEXEC), 0)) > > > > > > This is a bug I introduced accidentally during testing. This > > > GENERIC_READ is actually supposed to be a FILE_READ_ATTRIBUTES. > > > Sorry about that. > > > > The PoC code uses PeekNamedPipe for query_hdl, so GENERIC_READ is > > necessary I think. > > Oh, right, that's how it's documented. Funny enough, the descriptions > of FSCTL_PIPE_PEEK does not mention any permissions at all. I tried the > permissions and it's FILE_READ_DATA which is required. Thanks. I revised the patch and attach it the subsequent mail. -- Takashi Yano