Re: 3.3.0: Possible regression in cygwin DLL (Win10); fixed in snapshot

[Takashi Yano <takashi.yano@nifty.ne.jp>]

[-- Attachment #1: Type: text/plain, Size: 1843 bytes --]

On Thu, 11 Nov 2021 21:02:34 +0900
Takashi Yano wrote:
> On Thu, 11 Nov 2021 12:33:21 +0100
> Corinna Vinschen wrote:
> > On Nov 11 20:12, Takashi Yano wrote:
> > > > [FileProcessIdsUsingFileInformation]
> > > 
> > > Thanks for advice. get_query_hdl_per_* is called in the write side,
> > > so only knows pipe handle of the write side. We would like to know
> > > ProcessId which have pipe handle of the read side. Can we use
> > > FileProcessIdsUsingFileInformation for this perpose?
> > 
> > I don't know.  I just stumbled over it yesterday and I thought it might
> > be something we could utilize.  Supposedly it returns PIDs for processes
> > having that file open, but how this works for pipe read/write sides
> > needs testing.  Of course, knowing how Windows functions usually only go
> > half the way, the function will either only return the current side of
> > the pipe, or even an error code :-/  Never mind, it was just an idea.
> 
> I have tested the behaviour of FileProcessIdsUsingFileInformation
> just now. We can use it! I will try to use it in get_query_hdl().

I have tried to utilize FileProcessIdsUsingFileInformation in
get_query_hdl_per_process() as the patch attached. It works as
expected.

I also measured the response of select() using these functions.
The first time and second time responses are measured. The second
time should be much faster than the first time because search
result has been cached.

First time, Second time
 4.620400 [msec], 0.102300 [msec] << get_query_hdl_per_process()
19.080400 [msec], 0.199000 [msec] << get_query_hdl_per_system()
14.364300 [msec], 0.156800 [msec] << FileProcessIdsUsingFileInformation

Unfortunately, FileProcessIdsUsingFileInformation is slower than
current get_query_hdl_per_process(). It takes about 3 times longer
time.

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

[-- Attachment #2: fpiufi.patch --]
[-- Type: application/octet-stream, Size: 2977 bytes --]

diff --git a/winsup/cygwin/fhandler_pipe.cc b/winsup/cygwin/fhandler_pipe.cc
index 9392a28c1..83c09d7c6 100644
--- a/winsup/cygwin/fhandler_pipe.cc
+++ b/winsup/cygwin/fhandler_pipe.cc
@@ -1201,50 +1201,32 @@ fhandler_pipe::get_query_hdl_per_process (WCHAR *name,
   NTSTATUS status;
   ULONG len;
   DWORD n_process = 256;
-  PSYSTEM_PROCESS_INFORMATION spi;
+  FILE_PROCESS_IDS_USING_FILE_INFORMATION *fpiufi;
   do
-    { /* Enumerate processes */
-      DWORD nbytes = n_process * sizeof (SYSTEM_PROCESS_INFORMATION);
-      spi = (PSYSTEM_PROCESS_INFORMATION) HeapAlloc (GetProcessHeap (),
-						     0, nbytes);
-      if (!spi)
+    { /* Enumerate processes having the pipe handle. */
+      IO_STATUS_BLOCK io;
+      DWORD nbytes = n_process * sizeof (ULONG)
+	+ sizeof (FILE_PROCESS_IDS_USING_FILE_INFORMATION);
+      fpiufi = (FILE_PROCESS_IDS_USING_FILE_INFORMATION *)
+	HeapAlloc (GetProcessHeap (), 0, nbytes);
+      if (!fpiufi)
 	return NULL;
-      status = NtQuerySystemInformation (SystemProcessInformation,
-					 spi, nbytes, &len);
+      status = NtQueryInformationFile (get_handle (), &io, fpiufi, nbytes,
+				       FileProcessIdsUsingFileInformation);
       if (NT_SUCCESS (status))
 	break;
-      HeapFree (GetProcessHeap (), 0, spi);
+      HeapFree (GetProcessHeap (), 0, fpiufi);
       n_process *= 2;
     }
   while (n_process < (1L<<20) && status == STATUS_INFO_LENGTH_MISMATCH);
   if (!NT_SUCCESS (status))
     return NULL;
 
-  /* In most cases, it is faster to check the processes in reverse order.
-     To do this, store PIDs into an array. */
-  DWORD *proc_pids = (DWORD *) HeapAlloc (GetProcessHeap (), 0,
-					  n_process * sizeof (DWORD));
-  if (!proc_pids)
-    {
-      HeapFree (GetProcessHeap (), 0, spi);
-      return NULL;
-    }
-  PSYSTEM_PROCESS_INFORMATION p = spi;
-  n_process = 0;
-  while (true)
-    {
-      proc_pids[n_process++] = (DWORD)(intptr_t) p->UniqueProcessId;
-      if (!p->NextEntryOffset)
-	break;
-      p = (PSYSTEM_PROCESS_INFORMATION) ((char *) p + p->NextEntryOffset);
-    }
-  HeapFree (GetProcessHeap (), 0, spi);
-
-  for (LONG i = (LONG) n_process - 1; i >= 0; i--)
+  for (ULONG i = 0; i < fpiufi->NumberOfProcessIdsInList; i++)
     {
       HANDLE proc = OpenProcess (PROCESS_DUP_HANDLE
 				 | PROCESS_QUERY_INFORMATION,
-				 0, proc_pids[i]);
+				 0, fpiufi->ProcessIdList[i]);
       if (!proc)
 	continue;
 
@@ -1298,7 +1280,7 @@ fhandler_pipe::get_query_hdl_per_process (WCHAR *name,
 	      query_hdl_proc = proc;
 	      query_hdl_value = (HANDLE)(intptr_t) phi->Handles[j].HandleValue;
 	      HeapFree (GetProcessHeap (), 0, phi);
-	      HeapFree (GetProcessHeap (), 0, proc_pids);
+	      HeapFree (GetProcessHeap (), 0, fpiufi);
 	      return h;
 	    }
 close_handle:
@@ -1308,7 +1290,7 @@ close_handle:
 close_proc:
       CloseHandle (proc);
     }
-  HeapFree (GetProcessHeap (), 0, proc_pids);
+  HeapFree (GetProcessHeap (), 0, fpiufi);
   return NULL;
 }