[PATCH] Cygwin: pipe: Give up to use query_hdl for non-cygwin apps.

[PATCH] Cygwin: pipe: Give up to use query_hdl for non-cygwin apps.

[Takashi Yano]

Non-cygwin app may call ReadFile() which makes NtQueryObject() for
ObjectNameInformation block in fhandler_pipe::get_query_hdl_per_process.
Therefore, stop to try to get query_hdl for non-cygwin apps.

Addresses: https://github.com/msys2/msys2-runtime/issues/202

Fixes: b531d6b06eeb ("Cygwin: pipe: Introduce temporary query_hdl.")
Reported-by: Alisa Sireneva, Johannes Schindelin <Johannes.Schindelin@gmx.de>
Reviewed-by:
Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
---
 winsup/cygwin/fhandler/pipe.cc | 10 ++++++++++
 winsup/cygwin/release/3.5.2    |  4 ++++
 2 files changed, 14 insertions(+)

diff --git a/winsup/cygwin/fhandler/pipe.cc b/winsup/cygwin/fhandler/pipe.cc
index 1a97108b5..95c2f843a 100644
--- a/winsup/cygwin/fhandler/pipe.cc
+++ b/winsup/cygwin/fhandler/pipe.cc
@@ -1241,6 +1241,16 @@ fhandler_pipe::get_query_hdl_per_process (WCHAR *name,
 
   for (LONG i = (LONG) n_process - 1; i >= 0; i--)
     {
+      /* Non-cygwin app may call ReadFile() which makes NtQueryObject()
+	for ObjectNameInformation block. Therefore, stop to try to get
+	query_hdl for non-cygwin apps. */
+      pid_t cygpid;
+      if (!(cygpid = cygwin_pid (proc_pids[i])))
+	continue;
+      pinfo p (cygpid);
+      if (p && ISSTATE (p, PID_NOTCYGWIN))
+	continue;
+
       HANDLE proc = OpenProcess (PROCESS_DUP_HANDLE
 				 | PROCESS_QUERY_INFORMATION,
 				 0, proc_pids[i]);
diff --git a/winsup/cygwin/release/3.5.2 b/winsup/cygwin/release/3.5.2
index 7d8df9489..efd30b64a 100644
--- a/winsup/cygwin/release/3.5.2
+++ b/winsup/cygwin/release/3.5.2
@@ -5,3 +5,7 @@ Fixes:
   is already unmapped due to race condition. To avoid this issue,
   shared console memory will be kept mapped if it belongs to CTTY.
   Addresses:  https://cygwin.com/pipermail/cygwin/2024-February/255561.html
+
+- Fix a problem that select() call for write-side of a pipe possibly
+  hangs with non-cygwin reader.
+  Addresses: https://github.com/msys2/msys2-runtime/issues/202
-- 
2.43.0

Re: [PATCH] Cygwin: pipe: Give up to use query_hdl for non-cygwin apps.

[Johannes Schindelin]

Hi Takashi,

thank you for the fix! I can confirm that it addresses the problem
demonstrated by the reproducer in that MSYS2-runtime ticket.

After noticing that we enumerate all the processes (which is an expensive
operation) just to skip all of the non-Cygwin ones anyway, I wonder if it
wouldn't be smarter to go through the internal list of cygpids and take it
from there, skipping the `SystemProcessInformation` calls altogether.

Ciao,
Johannes

On Sun, 3 Mar 2024, Takashi Yano wrote:

> Non-cygwin app may call ReadFile() which makes NtQueryObject() for
> ObjectNameInformation block in fhandler_pipe::get_query_hdl_per_process.
> Therefore, stop to try to get query_hdl for non-cygwin apps.
>
> Addresses: https://github.com/msys2/msys2-runtime/issues/202
>
> Fixes: b531d6b06eeb ("Cygwin: pipe: Introduce temporary query_hdl.")
> Reported-by: Alisa Sireneva, Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Reviewed-by:
> Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
> ---
>  winsup/cygwin/fhandler/pipe.cc | 10 ++++++++++
>  winsup/cygwin/release/3.5.2    |  4 ++++
>  2 files changed, 14 insertions(+)
>
> diff --git a/winsup/cygwin/fhandler/pipe.cc b/winsup/cygwin/fhandler/pipe.cc
> index 1a97108b5..95c2f843a 100644
> --- a/winsup/cygwin/fhandler/pipe.cc
> +++ b/winsup/cygwin/fhandler/pipe.cc
> @@ -1241,6 +1241,16 @@ fhandler_pipe::get_query_hdl_per_process (WCHAR *name,
>
>    for (LONG i = (LONG) n_process - 1; i >= 0; i--)
>      {
> +      /* Non-cygwin app may call ReadFile() which makes NtQueryObject()
> +	for ObjectNameInformation block. Therefore, stop to try to get
> +	query_hdl for non-cygwin apps. */
> +      pid_t cygpid;
> +      if (!(cygpid = cygwin_pid (proc_pids[i])))
> +	continue;
> +      pinfo p (cygpid);
> +      if (p && ISSTATE (p, PID_NOTCYGWIN))
> +	continue;
> +
>        HANDLE proc = OpenProcess (PROCESS_DUP_HANDLE
>  				 | PROCESS_QUERY_INFORMATION,
>  				 0, proc_pids[i]);
> diff --git a/winsup/cygwin/release/3.5.2 b/winsup/cygwin/release/3.5.2
> index 7d8df9489..efd30b64a 100644
> --- a/winsup/cygwin/release/3.5.2
> +++ b/winsup/cygwin/release/3.5.2
> @@ -5,3 +5,7 @@ Fixes:
>    is already unmapped due to race condition. To avoid this issue,
>    shared console memory will be kept mapped if it belongs to CTTY.
>    Addresses:  https://cygwin.com/pipermail/cygwin/2024-February/255561.html
> +
> +- Fix a problem that select() call for write-side of a pipe possibly
> +  hangs with non-cygwin reader.
> +  Addresses: https://github.com/msys2/msys2-runtime/issues/202
> --
> 2.43.0
>
>

Re: [PATCH] Cygwin: pipe: Give up to use query_hdl for non-cygwin apps.

[Takashi Yano]

On Sun, 3 Mar 2024 10:34:44 +0100 (CET)
Johannes Schindelin wrote:
> Hi Takashi,
> 
> thank you for the fix! I can confirm that it addresses the problem
> demonstrated by the reproducer in that MSYS2-runtime ticket.

Thanks for testing!

> After noticing that we enumerate all the processes (which is an expensive
> operation) just to skip all of the non-Cygwin ones anyway, I wonder if it
> wouldn't be smarter to go through the internal list of cygpids and take it
> from there, skipping the `SystemProcessInformation` calls altogether.

Yeah, that makes sens. I'll submit v2 patch.

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

Re: [PATCH] Cygwin: pipe: Give up to use query_hdl for non-cygwin apps.

[ASSI]

Takashi Yano writes:
>> After noticing that we enumerate all the processes (which is an expensive
>> operation) just to skip all of the non-Cygwin ones anyway, I wonder if it
>> wouldn't be smarter to go through the internal list of cygpids and take it
>> from there, skipping the `SystemProcessInformation` calls altogether.
>
> Yeah, that makes sens. I'll submit v2 patch.

Keep in mind that there may be different independent Cygwin
installations running on the same nachine.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Waldorf MIDI Implementation & additional documentation:
http://Synth.Stromeko.net/Downloads.html#WaldorfDocs

Re: [PATCH] Cygwin: pipe: Give up to use query_hdl for non-cygwin apps.

[Takashi Yano]

On Sun, 03 Mar 2024 11:39:40 +0100
ASSI wrote:
> Takashi Yano writes:
> >> After noticing that we enumerate all the processes (which is an expensive
> >> operation) just to skip all of the non-Cygwin ones anyway, I wonder if it
> >> wouldn't be smarter to go through the internal list of cygpids and take it
> >> from there, skipping the `SystemProcessInformation` calls altogether.
> >
> > Yeah, that makes sens. I'll submit v2 patch.
> 
> Keep in mind that there may be different independent Cygwin
> installations running on the same nachine.

That's possible. But how can we know that is a process in another
installaion of cygwin?

If it is difficult, I think it is not so nonsense to treat it as
same as non-cygwin process.

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

Re: [PATCH] Cygwin: pipe: Give up to use query_hdl for non-cygwin apps.

[Corinna Vinschen]

On Mar  3 20:36, Takashi Yano wrote:
> On Sun, 03 Mar 2024 11:39:40 +0100
> ASSI wrote:
> > Takashi Yano writes:
> > >> After noticing that we enumerate all the processes (which is an expensive
> > >> operation) just to skip all of the non-Cygwin ones anyway, I wonder if it
> > >> wouldn't be smarter to go through the internal list of cygpids and take it
> > >> from there, skipping the `SystemProcessInformation` calls altogether.
> > >
> > > Yeah, that makes sens. I'll submit v2 patch.
> > 
> > Keep in mind that there may be different independent Cygwin
> > installations running on the same nachine.
> 
> That's possible. But how can we know that is a process in another
> installaion of cygwin?
> 
> If it is difficult, I think it is not so nonsense to treat it as
> same as non-cygwin process.

Right you are.  We always said that independent Cygwin installations
are supposed to *stay* independent.

Keep in mind that they don't share the same shared objects in the native
NT namespace and they don't know of each other.  It's not only the
process table but also in-use FIFO stuff, pty info, etc.


Corinna

Re: [PATCH] Cygwin: pipe: Give up to use query_hdl for non-cygwin apps.

[ASSI]

Corinna Vinschen writes:
> Right you are.  We always said that independent Cygwin installations
> are supposed to *stay* independent.
>
> Keep in mind that they don't share the same shared objects in the native
> NT namespace and they don't know of each other.  It's not only the
> process table but also in-use FIFO stuff, pty info, etc.

What I was getting at is that a process not showing up in the process
list in one Cygwin installation doesn't automatically mean it's a native
Windows process, it could be a process started by an independent Cygwin
installation.  So this way of checking for "native" Windows processes
may or may not do what was originally intended.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Factory and User Sound Singles for Waldorf Q+, Q and microQ:
http://Synth.Stromeko.net/Downloads.html#WaldorfSounds

Re: [PATCH] Cygwin: pipe: Give up to use query_hdl for non-cygwin apps.

[Corinna Vinschen]

On Mar  4 16:45, ASSI wrote:
> Corinna Vinschen writes:
> > Right you are.  We always said that independent Cygwin installations
> > are supposed to *stay* independent.
> >
> > Keep in mind that they don't share the same shared objects in the native
> > NT namespace and they don't know of each other.  It's not only the
> > process table but also in-use FIFO stuff, pty info, etc.
> 
> What I was getting at is that a process not showing up in the process
> list in one Cygwin installation doesn't automatically mean it's a native
> Windows process, it could be a process started by an independent Cygwin
> installation.  So this way of checking for "native" Windows processes
> may or may not do what was originally intended.

But that was my point. A "foreign" Cygwin process from another
installation is not a Cygwin process.  Lots of interoperability
just won't work, so it's basically a native process.


Corinna

Re: [PATCH] Cygwin: pipe: Give up to use query_hdl for non-cygwin apps.

[Takashi Yano]

On Mon, 4 Mar 2024 18:38:07 +0100
Corinna Vinschen wrote:
> On Mar  4 16:45, ASSI wrote:
> > Corinna Vinschen writes:
> > > Right you are.  We always said that independent Cygwin installations
> > > are supposed to *stay* independent.
> > >
> > > Keep in mind that they don't share the same shared objects in the native
> > > NT namespace and they don't know of each other.  It's not only the
> > > process table but also in-use FIFO stuff, pty info, etc.
> > 
> > What I was getting at is that a process not showing up in the process
> > list in one Cygwin installation doesn't automatically mean it's a native
> > Windows process, it could be a process started by an independent Cygwin
> > installation.  So this way of checking for "native" Windows processes
> > may or may not do what was originally intended.
> 
> But that was my point. A "foreign" Cygwin process from another
> installation is not a Cygwin process.  Lots of interoperability
> just won't work, so it's basically a native process.

Actually, I think query_hdl can be retrieved from the process
from another installation of cygwin using NtQueryInformationProcess()
with ProcessHandleInformation. However, I cannot imagne the case
that the pipe is made by one cygwin installation but the reader
process is from another installation of cygwin.

BTW, what about v2 patch itself?

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

Re: [PATCH] Cygwin: pipe: Give up to use query_hdl for non-cygwin apps.

[Corinna Vinschen]

On Mar  5 09:06, Takashi Yano wrote:
> On Mon, 4 Mar 2024 18:38:07 +0100
> Corinna Vinschen wrote:
> > On Mar  4 16:45, ASSI wrote:
> > > Corinna Vinschen writes:
> > > > Right you are.  We always said that independent Cygwin installations
> > > > are supposed to *stay* independent.
> > > >
> > > > Keep in mind that they don't share the same shared objects in the native
> > > > NT namespace and they don't know of each other.  It's not only the
> > > > process table but also in-use FIFO stuff, pty info, etc.
> > > 
> > > What I was getting at is that a process not showing up in the process
> > > list in one Cygwin installation doesn't automatically mean it's a native
> > > Windows process, it could be a process started by an independent Cygwin
> > > installation.  So this way of checking for "native" Windows processes
> > > may or may not do what was originally intended.
> > 
> > But that was my point. A "foreign" Cygwin process from another
> > installation is not a Cygwin process.  Lots of interoperability
> > just won't work, so it's basically a native process.
> 
> Actually, I think query_hdl can be retrieved from the process
> from another installation of cygwin using NtQueryInformationProcess()
> with ProcessHandleInformation. However, I cannot imagne the case
> that the pipe is made by one cygwin installation but the reader
> process is from another installation of cygwin.
> 
> BTW, what about v2 patch itself?

It does the job with less code and less memory, which is good.
I would change the comment

  stop to try to get query_hdl for non-cygwin apps

to something like

  don't try to fetch query_hdl from non-cygwin apps

"stop trying" is a bit of a back-reference to the old code, which
is not necessary, I think.

This doesn't affect your patch, but while looking into this, what
strikes me as weird is that fhandler_pipe::temporary_query_hdl() calls
NtQueryObject() and assembles the pipe name via swscanf() every time it
is called.

Wouldn't it make sense to store the name in the fhandler's
path_conv::wide_path/uni_path at creation time instead?
The wide_path member is not used at all in pipes, ostensibly.


Thanks,
Corinna

Re: [PATCH] Cygwin: pipe: Give up to use query_hdl for non-cygwin apps.

[Takashi Yano]

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

On Tue, 5 Mar 2024 11:14:46 +0100
Corinna Vinschen wrote:
> On Mar  5 09:06, Takashi Yano wrote:
> > On Mon, 4 Mar 2024 18:38:07 +0100
> > Corinna Vinschen wrote:
> > > On Mar  4 16:45, ASSI wrote:
> > > > Corinna Vinschen writes:
> > > > > Right you are.  We always said that independent Cygwin installations
> > > > > are supposed to *stay* independent.
> > > > >
> > > > > Keep in mind that they don't share the same shared objects in the native
> > > > > NT namespace and they don't know of each other.  It's not only the
> > > > > process table but also in-use FIFO stuff, pty info, etc.
> > > > 
> > > > What I was getting at is that a process not showing up in the process
> > > > list in one Cygwin installation doesn't automatically mean it's a native
> > > > Windows process, it could be a process started by an independent Cygwin
> > > > installation.  So this way of checking for "native" Windows processes
> > > > may or may not do what was originally intended.
> > > 
> > > But that was my point. A "foreign" Cygwin process from another
> > > installation is not a Cygwin process.  Lots of interoperability
> > > just won't work, so it's basically a native process.
> > 
> > Actually, I think query_hdl can be retrieved from the process
> > from another installation of cygwin using NtQueryInformationProcess()
> > with ProcessHandleInformation. However, I cannot imagne the case
> > that the pipe is made by one cygwin installation but the reader
> > process is from another installation of cygwin.
> > 
> > BTW, what about v2 patch itself?
> 
> It does the job with less code and less memory, which is good.
> I would change the comment
> 
>   stop to try to get query_hdl for non-cygwin apps
> 
> to something like
> 
>   don't try to fetch query_hdl from non-cygwin apps
> 
> "stop trying" is a bit of a back-reference to the old code, which
> is not necessary, I think.

I'll submit v3 patch. Please review.

> This doesn't affect your patch, but while looking into this, what
> strikes me as weird is that fhandler_pipe::temporary_query_hdl() calls
> NtQueryObject() and assembles the pipe name via swscanf() every time it
> is called.
> 
> Wouldn't it make sense to store the name in the fhandler's
> path_conv::wide_path/uni_path at creation time instead?
> The wide_path member is not used at all in pipes, ostensibly.

Is the patch attached as you intended?

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

[-- Attachment #2: 0001-Cygwin-pipe-Simplify-chhecking-procedure-of-query_hd.patch --]
[-- Type: text/plain, Size: 4161 bytes --]

From 998027fe7a7a56af9507341266c9fcadee66beb9 Mon Sep 17 00:00:00 2001
From: Takashi Yano <takashi.yano@nifty.ne.jp>
Date: Tue, 5 Mar 2024 23:34:21 +0900
Subject: [PATCH] Cygwin: pipe: Simplify chhecking procedure of query_hdl.

This patch eliminates verbose NtQueryObject() calls in the procedure
to get query_hdl by storing pipe name into fhandler_base::pc when
the pipe is created.  fhandler_pipe::temporary_query_hdl() uses the
storedpipe name rather than the name retrieved by NtQueryObject().

Suggested-by: Corinna Vinschen <corinna@vinschen.de>
Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
---
 winsup/cygwin/fhandler/pipe.cc          | 39 ++++++++++++-------------
 winsup/cygwin/local_includes/fhandler.h |  3 --
 2 files changed, 19 insertions(+), 23 deletions(-)

diff --git a/winsup/cygwin/fhandler/pipe.cc b/winsup/cygwin/fhandler/pipe.cc
index c877d89d7..0611dd1c3 100644
--- a/winsup/cygwin/fhandler/pipe.cc
+++ b/winsup/cygwin/fhandler/pipe.cc
@@ -93,6 +93,19 @@ fhandler_pipe::init (HANDLE f, DWORD a, mode_t mode, int64_t uniq_id)
        even with FILE_SYNCHRONOUS_IO_NONALERT. */
     set_pipe_non_blocking (get_device () == FH_PIPER ?
 			   true : is_nonblocking ());
+
+  /* Store pipe name to path_conv pc for query_hdl check */
+  if (get_dev () == FH_PIPEW)
+    {
+      ULONG len;
+      tmp_pathbuf tp;
+      OBJECT_NAME_INFORMATION *ntfn = (OBJECT_NAME_INFORMATION *) tp.w_get ();
+      NTSTATUS status = NtQueryObject (f, ObjectNameInformation, ntfn,
+				       65536, &len);
+      if (NT_SUCCESS (status) && ntfn->Name.Buffer)
+	pc.set_nt_native_path (&ntfn->Name);
+    }
+
   return 1;
 }
 
@@ -1149,6 +1162,9 @@ fhandler_pipe::temporary_query_hdl ()
   tmp_pathbuf tp;
   OBJECT_NAME_INFORMATION *ntfn = (OBJECT_NAME_INFORMATION *) tp.w_get ();
 
+  UNICODE_STRING *name = pc.get_nt_native_path (NULL);
+  name->Buffer[name->Length / sizeof (WCHAR)] = L'\0';
+
   /* Try process handle opened and pipe handle value cached first
      in order to reduce overhead. */
   if (query_hdl_proc && query_hdl_value)
@@ -1161,14 +1177,7 @@ fhandler_pipe::temporary_query_hdl ()
       status = NtQueryObject (h, ObjectNameInformation, ntfn, 65536, &len);
       if (!NT_SUCCESS (status) || !ntfn->Name.Buffer)
 	goto hdl_err;
-      ntfn->Name.Buffer[ntfn->Name.Length / sizeof (WCHAR)] = L'\0';
-      uint64_t key;
-      DWORD pid;
-      LONG id;
-      if (swscanf (ntfn->Name.Buffer,
-		   L"\\Device\\NamedPipe\\%llx-%u-pipe-nt-0x%x",
-		   &key, &pid, &id) == 3 &&
-	  key == pipename_key && pid == pipename_pid && id == pipename_id)
+      if (RtlEqualUnicodeString (name, &ntfn->Name, FALSE))
 	return h;
 hdl_err:
       CloseHandle (h);
@@ -1178,19 +1187,9 @@ cache_err:
       query_hdl_value = NULL;
     }
 
-  status = NtQueryObject (get_handle (), ObjectNameInformation, ntfn,
-			  65536, &len);
-  if (!NT_SUCCESS (status) || !ntfn->Name.Buffer)
+  if (name->Length == 0 || name->Buffer == NULL)
     return NULL; /* Non cygwin pipe? */
-  WCHAR name[MAX_PATH];
-  int namelen = min (ntfn->Name.Length / sizeof (WCHAR), MAX_PATH-1);
-  memcpy (name, ntfn->Name.Buffer, namelen * sizeof (WCHAR));
-  name[namelen] = L'\0';
-  if (swscanf (name, L"\\Device\\NamedPipe\\%llx-%u-pipe-nt-0x%x",
-	       &pipename_key, &pipename_pid, &pipename_id) != 3)
-    return NULL; /* Non cygwin pipe? */
-
-  return get_query_hdl_per_process (name, ntfn); /* Since Win8 */
+  return get_query_hdl_per_process (name->Buffer, ntfn); /* Since Win8 */
 }
 
 HANDLE
diff --git a/winsup/cygwin/local_includes/fhandler.h b/winsup/cygwin/local_includes/fhandler.h
index 6ddf37370..028eb49d0 100644
--- a/winsup/cygwin/local_includes/fhandler.h
+++ b/winsup/cygwin/local_includes/fhandler.h
@@ -1216,9 +1216,6 @@ private:
   HANDLE query_hdl_proc;
   HANDLE query_hdl_value;
   HANDLE query_hdl_close_req_evt;
-  uint64_t pipename_key;
-  DWORD pipename_pid;
-  LONG pipename_id;
   void release_select_sem (const char *);
   HANDLE get_query_hdl_per_process (WCHAR *, OBJECT_NAME_INFORMATION *);
 public:
-- 
2.43.0

Re: [PATCH] Cygwin: pipe: Give up to use query_hdl for non-cygwin apps.

[Corinna Vinschen]

On Mar  5 23:47, Takashi Yano wrote:
> On Tue, 5 Mar 2024 11:14:46 +0100
> Corinna Vinschen wrote:
> > This doesn't affect your patch, but while looking into this, what
> > strikes me as weird is that fhandler_pipe::temporary_query_hdl() calls
> > NtQueryObject() and assembles the pipe name via swscanf() every time it
> > is called.
> > 
> > Wouldn't it make sense to store the name in the fhandler's
> > path_conv::wide_path/uni_path at creation time instead?
> > The wide_path member is not used at all in pipes, ostensibly.
> 
> Is the patch attached as you intended?

Yes, but it looks like it misses a few potential simplifications:

> diff --git a/winsup/cygwin/fhandler/pipe.cc b/winsup/cygwin/fhandler/pipe.cc
> index c877d89d7..0611dd1c3 100644
> --- a/winsup/cygwin/fhandler/pipe.cc
> +++ b/winsup/cygwin/fhandler/pipe.cc
> @@ -93,6 +93,19 @@ fhandler_pipe::init (HANDLE f, DWORD a, mode_t mode, int64_t uniq_id)
>         even with FILE_SYNCHRONOUS_IO_NONALERT. */
>      set_pipe_non_blocking (get_device () == FH_PIPER ?
>  			   true : is_nonblocking ());
> +
> +  /* Store pipe name to path_conv pc for query_hdl check */
> +  if (get_dev () == FH_PIPEW)
> +    {
> +      ULONG len;
> +      tmp_pathbuf tp;
> +      OBJECT_NAME_INFORMATION *ntfn = (OBJECT_NAME_INFORMATION *) tp.w_get ();
> +      NTSTATUS status = NtQueryObject (f, ObjectNameInformation, ntfn,
> +				       65536, &len);
> +      if (NT_SUCCESS (status) && ntfn->Name.Buffer)
> +	pc.set_nt_native_path (&ntfn->Name);

We don't have to call NtQueryObject.  The name is created in nt_create()
and we know the unique id, so the name is

  "%S%S-%u-pipe-nt-%p", &ro_u_ntfs, &cygheap->installation_key,
  			GetCurrentProcessId (), unique_id);

Do you think it's cheaper to call NtQueryObject()?  If so, no worries,
but NtQueryObject() has to call into the kernel, while just creating
the name by ourselves doesn't.

> @@ -1149,6 +1162,9 @@ fhandler_pipe::temporary_query_hdl ()
>    tmp_pathbuf tp;
>    OBJECT_NAME_INFORMATION *ntfn = (OBJECT_NAME_INFORMATION *) tp.w_get ();
>  
> +  UNICODE_STRING *name = pc.get_nt_native_path (NULL);
> +  name->Buffer[name->Length / sizeof (WCHAR)] = L'\0';

The string returned by get_nt_native_path() is always NUL-terminated.

>    /* Try process handle opened and pipe handle value cached first
>       in order to reduce overhead. */
>    if (query_hdl_proc && query_hdl_value)
> @@ -1161,14 +1177,7 @@ fhandler_pipe::temporary_query_hdl ()
>        status = NtQueryObject (h, ObjectNameInformation, ntfn, 65536, &len);
>        if (!NT_SUCCESS (status) || !ntfn->Name.Buffer)
>  	goto hdl_err;
> -      ntfn->Name.Buffer[ntfn->Name.Length / sizeof (WCHAR)] = L'\0';
> -      uint64_t key;
> -      DWORD pid;
> -      LONG id;
> -      if (swscanf (ntfn->Name.Buffer,
> -		   L"\\Device\\NamedPipe\\%llx-%u-pipe-nt-0x%x",
> -		   &key, &pid, &id) == 3 &&
> -	  key == pipename_key && pid == pipename_pid && id == pipename_id)
> +      if (RtlEqualUnicodeString (name, &ntfn->Name, FALSE))
>  	return h;
>  hdl_err:
>        CloseHandle (h);
> @@ -1178,19 +1187,9 @@ cache_err:
>        query_hdl_value = NULL;
>      }
>  
> -  status = NtQueryObject (get_handle (), ObjectNameInformation, ntfn,
> -			  65536, &len);
> -  if (!NT_SUCCESS (status) || !ntfn->Name.Buffer)
> +  if (name->Length == 0 || name->Buffer == NULL)
>      return NULL; /* Non cygwin pipe? */
> -  WCHAR name[MAX_PATH];
> -  int namelen = min (ntfn->Name.Length / sizeof (WCHAR), MAX_PATH-1);
> -  memcpy (name, ntfn->Name.Buffer, namelen * sizeof (WCHAR));
> -  name[namelen] = L'\0';
> -  if (swscanf (name, L"\\Device\\NamedPipe\\%llx-%u-pipe-nt-0x%x",
> -	       &pipename_key, &pipename_pid, &pipename_id) != 3)
> -    return NULL; /* Non cygwin pipe? */
> -
> -  return get_query_hdl_per_process (name, ntfn); /* Since Win8 */
> +  return get_query_hdl_per_process (name->Buffer, ntfn); /* Since Win8 */

Given the name is stored in the fhandler, get_query_hdl_per_process()
doesn't need it as argument, and get_query_hdl_per_process() can just
call RtlCompareUnicodeString() instead of adding a \0 and calling
wcscmp().


Thanks,
Corinna

Re: [PATCH] Cygwin: pipe: Give up to use query_hdl for non-cygwin apps.

[Takashi Yano]

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

On Tue, 5 Mar 2024 17:54:19 +0100
Corinna Vinschen wrote:
> On Mar  5 23:47, Takashi Yano wrote:
> > On Tue, 5 Mar 2024 11:14:46 +0100
> > Corinna Vinschen wrote:
> > > This doesn't affect your patch, but while looking into this, what
> > > strikes me as weird is that fhandler_pipe::temporary_query_hdl() calls
> > > NtQueryObject() and assembles the pipe name via swscanf() every time it
> > > is called.
> > > 
> > > Wouldn't it make sense to store the name in the fhandler's
> > > path_conv::wide_path/uni_path at creation time instead?
> > > The wide_path member is not used at all in pipes, ostensibly.
> > 
> > Is the patch attached as you intended?
> 
> Yes, but it looks like it misses a few potential simplifications:
> 
> > diff --git a/winsup/cygwin/fhandler/pipe.cc b/winsup/cygwin/fhandler/pipe.cc
> > index c877d89d7..0611dd1c3 100644
> > --- a/winsup/cygwin/fhandler/pipe.cc
> > +++ b/winsup/cygwin/fhandler/pipe.cc
> > @@ -93,6 +93,19 @@ fhandler_pipe::init (HANDLE f, DWORD a, mode_t mode, int64_t uniq_id)
> >         even with FILE_SYNCHRONOUS_IO_NONALERT. */
> >      set_pipe_non_blocking (get_device () == FH_PIPER ?
> >  			   true : is_nonblocking ());
> > +
> > +  /* Store pipe name to path_conv pc for query_hdl check */
> > +  if (get_dev () == FH_PIPEW)
> > +    {
> > +      ULONG len;
> > +      tmp_pathbuf tp;
> > +      OBJECT_NAME_INFORMATION *ntfn = (OBJECT_NAME_INFORMATION *) tp.w_get ();
> > +      NTSTATUS status = NtQueryObject (f, ObjectNameInformation, ntfn,
> > +				       65536, &len);
> > +      if (NT_SUCCESS (status) && ntfn->Name.Buffer)
> > +	pc.set_nt_native_path (&ntfn->Name);
> 
> We don't have to call NtQueryObject.  The name is created in nt_create()
> and we know the unique id, so the name is
> 
>   "%S%S-%u-pipe-nt-%p", &ro_u_ntfs, &cygheap->installation_key,
>   			GetCurrentProcessId (), unique_id);
> 
> Do you think it's cheaper to call NtQueryObject()?  If so, no worries,
> but NtQueryObject() has to call into the kernel, while just creating
> the name by ourselves doesn't.
> 
> > @@ -1149,6 +1162,9 @@ fhandler_pipe::temporary_query_hdl ()
> >    tmp_pathbuf tp;
> >    OBJECT_NAME_INFORMATION *ntfn = (OBJECT_NAME_INFORMATION *) tp.w_get ();
> >  
> > +  UNICODE_STRING *name = pc.get_nt_native_path (NULL);
> > +  name->Buffer[name->Length / sizeof (WCHAR)] = L'\0';
> 
> The string returned by get_nt_native_path() is always NUL-terminated.
> 
> >    /* Try process handle opened and pipe handle value cached first
> >       in order to reduce overhead. */
> >    if (query_hdl_proc && query_hdl_value)
> > @@ -1161,14 +1177,7 @@ fhandler_pipe::temporary_query_hdl ()
> >        status = NtQueryObject (h, ObjectNameInformation, ntfn, 65536, &len);
> >        if (!NT_SUCCESS (status) || !ntfn->Name.Buffer)
> >  	goto hdl_err;
> > -      ntfn->Name.Buffer[ntfn->Name.Length / sizeof (WCHAR)] = L'\0';
> > -      uint64_t key;
> > -      DWORD pid;
> > -      LONG id;
> > -      if (swscanf (ntfn->Name.Buffer,
> > -		   L"\\Device\\NamedPipe\\%llx-%u-pipe-nt-0x%x",
> > -		   &key, &pid, &id) == 3 &&
> > -	  key == pipename_key && pid == pipename_pid && id == pipename_id)
> > +      if (RtlEqualUnicodeString (name, &ntfn->Name, FALSE))
> >  	return h;
> >  hdl_err:
> >        CloseHandle (h);
> > @@ -1178,19 +1187,9 @@ cache_err:
> >        query_hdl_value = NULL;
> >      }
> >  
> > -  status = NtQueryObject (get_handle (), ObjectNameInformation, ntfn,
> > -			  65536, &len);
> > -  if (!NT_SUCCESS (status) || !ntfn->Name.Buffer)
> > +  if (name->Length == 0 || name->Buffer == NULL)
> >      return NULL; /* Non cygwin pipe? */
> > -  WCHAR name[MAX_PATH];
> > -  int namelen = min (ntfn->Name.Length / sizeof (WCHAR), MAX_PATH-1);
> > -  memcpy (name, ntfn->Name.Buffer, namelen * sizeof (WCHAR));
> > -  name[namelen] = L'\0';
> > -  if (swscanf (name, L"\\Device\\NamedPipe\\%llx-%u-pipe-nt-0x%x",
> > -	       &pipename_key, &pipename_pid, &pipename_id) != 3)
> > -    return NULL; /* Non cygwin pipe? */
> > -
> > -  return get_query_hdl_per_process (name, ntfn); /* Since Win8 */
> > +  return get_query_hdl_per_process (name->Buffer, ntfn); /* Since Win8 */
> 
> Given the name is stored in the fhandler, get_query_hdl_per_process()
> doesn't need it as argument, and get_query_hdl_per_process() can just
> call RtlCompareUnicodeString() instead of adding a \0 and calling
> wcscmp().

Thanks for advice. I have revised the patch.

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

[-- Attachment #2: v2-0001-Cygwin-pipe-Simplify-chhecking-procedure-of-query.patch --]
[-- Type: text/plain, Size: 5000 bytes --]

From aac59194af8bb80a6b9a0891a27fd7de9d0b69fc Mon Sep 17 00:00:00 2001
From: Takashi Yano <takashi.yano@nifty.ne.jp>
Date: Tue, 5 Mar 2024 23:34:21 +0900
Subject: [PATCH v2] Cygwin: pipe: Simplify chhecking procedure of query_hdl.

This patch eliminates verbose NtQueryObject() calls in the procedure
to get query_hdl by storing pipe name into fhandler_base::pc when
the pipe is created.  fhandler_pipe::temporary_query_hdl() uses the
storedpipe name rather than the name retrieved by NtQueryObject().

Suggested-by: Corinna Vinschen <corinna@vinschen.de>
Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
---
 winsup/cygwin/fhandler/pipe.cc          | 47 ++++++++++++-------------
 winsup/cygwin/local_includes/fhandler.h |  5 +--
 2 files changed, 24 insertions(+), 28 deletions(-)

diff --git a/winsup/cygwin/fhandler/pipe.cc b/winsup/cygwin/fhandler/pipe.cc
index c877d89d7..0d57f5585 100644
--- a/winsup/cygwin/fhandler/pipe.cc
+++ b/winsup/cygwin/fhandler/pipe.cc
@@ -93,6 +93,21 @@ fhandler_pipe::init (HANDLE f, DWORD a, mode_t mode, int64_t uniq_id)
        even with FILE_SYNCHRONOUS_IO_NONALERT. */
     set_pipe_non_blocking (get_device () == FH_PIPER ?
 			   true : is_nonblocking ());
+
+  /* Store pipe name to path_conv pc for query_hdl check */
+  if (get_dev () == FH_PIPEW)
+    {
+      UNICODE_STRING name;
+      WCHAR pipename_buf[MAX_PATH];
+      __small_swprintf (pipename_buf, L"%S%S-%u-pipe-nt-%p",
+			&ro_u_npfs, &cygheap->installation_key,
+			GetCurrentProcessId (), unique_id >> 32);
+      name.Length = wcslen (pipename_buf) * sizeof (WCHAR);
+      name.MaximumLength = MAX_PATH * sizeof (WCHAR);
+      name.Buffer = pipename_buf;
+      pc.set_nt_native_path (&name);
+    }
+
   return 1;
 }
 
@@ -1149,6 +1164,8 @@ fhandler_pipe::temporary_query_hdl ()
   tmp_pathbuf tp;
   OBJECT_NAME_INFORMATION *ntfn = (OBJECT_NAME_INFORMATION *) tp.w_get ();
 
+  UNICODE_STRING *name = pc.get_nt_native_path (NULL);
+
   /* Try process handle opened and pipe handle value cached first
      in order to reduce overhead. */
   if (query_hdl_proc && query_hdl_value)
@@ -1161,14 +1178,7 @@ fhandler_pipe::temporary_query_hdl ()
       status = NtQueryObject (h, ObjectNameInformation, ntfn, 65536, &len);
       if (!NT_SUCCESS (status) || !ntfn->Name.Buffer)
 	goto hdl_err;
-      ntfn->Name.Buffer[ntfn->Name.Length / sizeof (WCHAR)] = L'\0';
-      uint64_t key;
-      DWORD pid;
-      LONG id;
-      if (swscanf (ntfn->Name.Buffer,
-		   L"\\Device\\NamedPipe\\%llx-%u-pipe-nt-0x%x",
-		   &key, &pid, &id) == 3 &&
-	  key == pipename_key && pid == pipename_pid && id == pipename_id)
+      if (RtlEqualUnicodeString (name, &ntfn->Name, FALSE))
 	return h;
 hdl_err:
       CloseHandle (h);
@@ -1178,24 +1188,13 @@ cache_err:
       query_hdl_value = NULL;
     }
 
-  status = NtQueryObject (get_handle (), ObjectNameInformation, ntfn,
-			  65536, &len);
-  if (!NT_SUCCESS (status) || !ntfn->Name.Buffer)
-    return NULL; /* Non cygwin pipe? */
-  WCHAR name[MAX_PATH];
-  int namelen = min (ntfn->Name.Length / sizeof (WCHAR), MAX_PATH-1);
-  memcpy (name, ntfn->Name.Buffer, namelen * sizeof (WCHAR));
-  name[namelen] = L'\0';
-  if (swscanf (name, L"\\Device\\NamedPipe\\%llx-%u-pipe-nt-0x%x",
-	       &pipename_key, &pipename_pid, &pipename_id) != 3)
+  if (name->Length == 0 || name->Buffer == NULL)
     return NULL; /* Non cygwin pipe? */
-
-  return get_query_hdl_per_process (name, ntfn); /* Since Win8 */
+  return get_query_hdl_per_process (ntfn); /* Since Win8 */
 }
 
 HANDLE
-fhandler_pipe::get_query_hdl_per_process (WCHAR *name,
-					  OBJECT_NAME_INFORMATION *ntfn)
+fhandler_pipe::get_query_hdl_per_process (OBJECT_NAME_INFORMATION *ntfn)
 {
   winpids pids ((DWORD) 0);
 
@@ -1272,8 +1271,8 @@ fhandler_pipe::get_query_hdl_per_process (WCHAR *name,
 				  ntfn, 65536, &len);
 	  if (!NT_SUCCESS (status) || !ntfn->Name.Buffer)
 	    goto close_handle;
-	  ntfn->Name.Buffer[ntfn->Name.Length / sizeof (WCHAR)] = L'\0';
-	  if (wcscmp (name, ntfn->Name.Buffer) == 0)
+	  if (RtlEqualUnicodeString (pc.get_nt_native_path (),
+				     &ntfn->Name, FALSE))
 	    {
 	      query_hdl_proc = proc;
 	      query_hdl_value = (HANDLE)(intptr_t) phi->Handles[j].HandleValue;
diff --git a/winsup/cygwin/local_includes/fhandler.h b/winsup/cygwin/local_includes/fhandler.h
index 6ddf37370..8729eb276 100644
--- a/winsup/cygwin/local_includes/fhandler.h
+++ b/winsup/cygwin/local_includes/fhandler.h
@@ -1216,11 +1216,8 @@ private:
   HANDLE query_hdl_proc;
   HANDLE query_hdl_value;
   HANDLE query_hdl_close_req_evt;
-  uint64_t pipename_key;
-  DWORD pipename_pid;
-  LONG pipename_id;
   void release_select_sem (const char *);
-  HANDLE get_query_hdl_per_process (WCHAR *, OBJECT_NAME_INFORMATION *);
+  HANDLE get_query_hdl_per_process (OBJECT_NAME_INFORMATION *);
 public:
   fhandler_pipe ();
 
-- 
2.43.0

Re: [PATCH] Cygwin: pipe: Give up to use query_hdl for non-cygwin apps.

[Takashi Yano]

On Wed, 6 Mar 2024 03:42:23 +0900
Takashi Yano wrote:
> +      name.MaximumLength = MAX_PATH * sizeof (WCHAR);

This should be:
name.MaximumLength = sizeof (pipename_buf);

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

Re: [PATCH] Cygwin: pipe: Give up to use query_hdl for non-cygwin apps.

[Corinna Vinschen]

On Mar  6 03:42, Takashi Yano wrote:
> On Tue, 5 Mar 2024 17:54:19 +0100
> Corinna Vinschen wrote:
> > On Mar  5 23:47, Takashi Yano wrote:
> > > On Tue, 5 Mar 2024 11:14:46 +0100
> > > Corinna Vinschen wrote:
> > > > This doesn't affect your patch, but while looking into this, what
> > > > strikes me as weird is that fhandler_pipe::temporary_query_hdl() calls
> > > > NtQueryObject() and assembles the pipe name via swscanf() every time it
> > > > is called.
> > > > 
> > > > Wouldn't it make sense to store the name in the fhandler's
> > > > path_conv::wide_path/uni_path at creation time instead?
> > > > The wide_path member is not used at all in pipes, ostensibly.
> > > 
> > > Is the patch attached as you intended?
> > 
> > Yes, but it looks like it misses a few potential simplifications:
> > [...]
> Thanks for advice. I have revised the patch.

Looks good, thanks!


Corinna