* [PATCH] fhandler_pipe: add sanity limit to handle loops @ 2021-12-23 23:10 Jeremy Drake 2021-12-24 0:06 ` Ken Brown 0 siblings, 1 reply; 35+ messages in thread From: Jeremy Drake @ 2021-12-23 23:10 UTC (permalink / raw) To: cygwin-patches [-- Attachment #1: Type: text/plain, Size: 1343 bytes --] Attempt to avoid an exception I caught once in gdb, trying to debug msys2/MSYS2-packages#2752. Unfortunately I haven't been able to catch it in the act again since, but with this change running on Windows on ARM64 has been more reliable than it had been. --- winsup/cygwin/fhandler_pipe.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/winsup/cygwin/fhandler_pipe.cc b/winsup/cygwin/fhandler_pipe.cc index ba6b70f55..48713a38d 100644 --- a/winsup/cygwin/fhandler_pipe.cc +++ b/winsup/cygwin/fhandler_pipe.cc @@ -1239,7 +1239,7 @@ fhandler_pipe::get_query_hdl_per_process (WCHAR *name, if (!NT_SUCCESS (status)) goto close_proc; - for (ULONG j = 0; j < phi->NumberOfHandles; j++) + for (ULONG j = 0; j < min(phi->NumberOfHandles, n_handle); j++) { /* Check for the peculiarity of cygwin read pipe */ const ULONG access = FILE_READ_DATA | FILE_READ_EA @@ -1309,7 +1309,7 @@ fhandler_pipe::get_query_hdl_per_system (WCHAR *name, if (!NT_SUCCESS (status)) return NULL; - for (LONG i = (LONG) shi->NumberOfHandles - 1; i >= 0; i--) + for (LONG i = (LONG) min(shi->NumberOfHandles, n_handle) - 1; i >= 0; i--) { /* Check for the peculiarity of cygwin read pipe */ const ULONG access = FILE_READ_DATA | FILE_READ_EA -- 2.34.1.windows.1 [-- Attachment #2: Type: text/plain, Size: 1408 bytes --] From aba830150ebb1707c66acdad8254728c5f962705 Mon Sep 17 00:00:00 2001 From: Jeremy Drake <cygwin@jdrake.com> Date: Thu, 23 Dec 2021 12:22:46 -0800 Subject: [PATCH] fhandler_pipe: add sanity limit to handle loops Attempt to avoid an exception I caught once in gdb, trying to debug msys2/MSYS2-packages#2752 --- winsup/cygwin/fhandler_pipe.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/winsup/cygwin/fhandler_pipe.cc b/winsup/cygwin/fhandler_pipe.cc index ba6b70f55..48713a38d 100644 --- a/winsup/cygwin/fhandler_pipe.cc +++ b/winsup/cygwin/fhandler_pipe.cc @@ -1239,7 +1239,7 @@ fhandler_pipe::get_query_hdl_per_process (WCHAR *name, if (!NT_SUCCESS (status)) goto close_proc; - for (ULONG j = 0; j < phi->NumberOfHandles; j++) + for (ULONG j = 0; j < min(phi->NumberOfHandles, n_handle); j++) { /* Check for the peculiarity of cygwin read pipe */ const ULONG access = FILE_READ_DATA | FILE_READ_EA @@ -1309,7 +1309,7 @@ fhandler_pipe::get_query_hdl_per_system (WCHAR *name, if (!NT_SUCCESS (status)) return NULL; - for (LONG i = (LONG) shi->NumberOfHandles - 1; i >= 0; i--) + for (LONG i = (LONG) min(shi->NumberOfHandles, n_handle) - 1; i >= 0; i--) { /* Check for the peculiarity of cygwin read pipe */ const ULONG access = FILE_READ_DATA | FILE_READ_EA -- 2.34.1.windows.1 ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] fhandler_pipe: add sanity limit to handle loops 2021-12-23 23:10 [PATCH] fhandler_pipe: add sanity limit to handle loops Jeremy Drake @ 2021-12-24 0:06 ` Ken Brown 2021-12-24 0:29 ` Jeremy Drake 0 siblings, 1 reply; 35+ messages in thread From: Ken Brown @ 2021-12-24 0:06 UTC (permalink / raw) To: cygwin-patches On 12/23/2021 6:10 PM, Jeremy Drake via Cygwin-patches wrote: > diff --git a/winsup/cygwin/fhandler_pipe.cc > b/winsup/cygwin/fhandler_pipe.cc > index ba6b70f55..48713a38d 100644 > --- a/winsup/cygwin/fhandler_pipe.cc > +++ b/winsup/cygwin/fhandler_pipe.cc > @@ -1239,7 +1239,7 @@ fhandler_pipe::get_query_hdl_per_process (WCHAR *name, > if (!NT_SUCCESS (status)) > goto close_proc; > > - for (ULONG j = 0; j < phi->NumberOfHandles; j++) > + for (ULONG j = 0; j < min(phi->NumberOfHandles, n_handle); j++) Reading the preceding code, I don't see how n_handle could be less than phi->NumberOfHandles. Can you explain? > { > /* Check for the peculiarity of cygwin read pipe */ > const ULONG access = FILE_READ_DATA | FILE_READ_EA > @@ -1309,7 +1309,7 @@ fhandler_pipe::get_query_hdl_per_system (WCHAR *name, > if (!NT_SUCCESS (status)) > return NULL; > > - for (LONG i = (LONG) shi->NumberOfHandles - 1; i >= 0; i--) > + for (LONG i = (LONG) min(shi->NumberOfHandles, n_handle) - 1; i >= 0; i--) Same comment. Ken ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] fhandler_pipe: add sanity limit to handle loops 2021-12-24 0:06 ` Ken Brown @ 2021-12-24 0:29 ` Jeremy Drake 2021-12-24 17:17 ` Ken Brown 2021-12-27 20:01 ` Jon Turney 0 siblings, 2 replies; 35+ messages in thread From: Jeremy Drake @ 2021-12-24 0:29 UTC (permalink / raw) To: cygwin-patches On Thu, 23 Dec 2021, Ken Brown wrote: > > - for (ULONG j = 0; j < phi->NumberOfHandles; j++) > > + for (ULONG j = 0; j < min(phi->NumberOfHandles, n_handle); j++) > > Reading the preceding code, I don't see how n_handle could be less than > phi->NumberOfHandles. Can you explain? > Not really. I saw this stack trace: ... #3 0x0000000180062f13 in exception::handle (e=0x14cc4f0, frame=<optimized out>, in=<optimized out>, dispatch=<optimized out>) at /c/S/msys2-runtime/src/msys2-runtime/winsup/cygwin/exceptions.cc:835 #4 0x00007ff8abd320cf in ntdll!.chkstk () from /c/Windows/SYSTEM32/ntdll.dll #5 0x00007ff8abce1454 in ntdll!RtlRaiseException () from /c/Windows/SYSTEM32/ntdll.dll #6 0x00007ff8abd30bfe in ntdll!KiUserExceptionDispatcher () from /c/Windows/SYSTEM32/ntdll.dll #7 0x0000000180092687 in fhandler_pipe::get_query_hdl_per_process (this=this@entry=0x1803700f8, name=name@entry=0x14cc820 L"\\Device\\NamedPipe\\dd50a72ab4668b33-10348-pipe-nt-0x6E6", ntfn=ntfn@entry=0x8000c2ce0) at /c/S/msys2-runtime/src/msys2-runtime/winsup/cygwin/fhandler_pipe.cc:1281 #8 0x0000000180092bdb in fhandler_pipe::temporary_query_hdl (this=this@entry=0x1803700f8) at /c/S/msys2-runtime/src/msys2-runtime/winsup/cygwin/fhandler_pipe.cc:1190 ... Line 1281 of fhandler_pipe.cc was if (phi->Handles[j].GrantedAccess != access) The only way I could see that causing an exception was if it was reading past the end of the allocated memory, if j was greater than (or equal to) n_handle. Unfortunately, I haven't been able to catch it in a debugger again, so I can't confirm this. I took a core with 'dumper' but gdb doesn't want to load it (it says Core file format not supported, maybe something with msys2's gdb?). ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] fhandler_pipe: add sanity limit to handle loops 2021-12-24 0:29 ` Jeremy Drake @ 2021-12-24 17:17 ` Ken Brown 2021-12-24 19:42 ` Jeremy Drake 2021-12-27 20:01 ` Jon Turney 1 sibling, 1 reply; 35+ messages in thread From: Ken Brown @ 2021-12-24 17:17 UTC (permalink / raw) To: cygwin-patches On 12/23/2021 7:29 PM, Jeremy Drake via Cygwin-patches wrote: > On Thu, 23 Dec 2021, Ken Brown wrote: > >>> - for (ULONG j = 0; j < phi->NumberOfHandles; j++) >>> + for (ULONG j = 0; j < min(phi->NumberOfHandles, n_handle); j++) >> >> Reading the preceding code, I don't see how n_handle could be less than >> phi->NumberOfHandles. Can you explain? >> > > Not really. I saw this stack trace: > ... > #3 0x0000000180062f13 in exception::handle (e=0x14cc4f0, frame=<optimized out>, in=<optimized out>, dispatch=<optimized out>) at /c/S/msys2-runtime/src/msys2-runtime/winsup/cygwin/exceptions.cc:835 > #4 0x00007ff8abd320cf in ntdll!.chkstk () from /c/Windows/SYSTEM32/ntdll.dll > #5 0x00007ff8abce1454 in ntdll!RtlRaiseException () from /c/Windows/SYSTEM32/ntdll.dll > #6 0x00007ff8abd30bfe in ntdll!KiUserExceptionDispatcher () from /c/Windows/SYSTEM32/ntdll.dll > #7 0x0000000180092687 in fhandler_pipe::get_query_hdl_per_process (this=this@entry=0x1803700f8, name=name@entry=0x14cc820 L"\\Device\\NamedPipe\\dd50a72ab4668b33-10348-pipe-nt-0x6E6", ntfn=ntfn@entry=0x8000c2ce0) at /c/S/msys2-runtime/src/msys2-runtime/winsup/cygwin/fhandler_pipe.cc:1281 > #8 0x0000000180092bdb in fhandler_pipe::temporary_query_hdl (this=this@entry=0x1803700f8) at /c/S/msys2-runtime/src/msys2-runtime/winsup/cygwin/fhandler_pipe.cc:1190 > ... > > Line 1281 of fhandler_pipe.cc was > if (phi->Handles[j].GrantedAccess != access) > > The only way I could see that causing an exception was if it was reading > past the end of the allocated memory, if j was greater than (or equal to) > n_handle. Unfortunately, I haven't been able to catch it in a debugger > again, so I can't confirm this. I took a core with 'dumper' but gdb > doesn't want to load it (it says Core file format not supported, maybe > something with msys2's gdb?). I agree that it's hard to see how the line you quoted could cause an exception. But you were using an optimized build, so I'm not sure how reliable the line-number information is. Is it feasible to run your test under strace? If so, you could add some debug_printf statements to examine the values of n_handle and phi->NumberOfHandles. Or what about simply adding an assertion that phi->NumberOfHandles <= n_handle? Ken ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] fhandler_pipe: add sanity limit to handle loops 2021-12-24 17:17 ` Ken Brown @ 2021-12-24 19:42 ` Jeremy Drake 2021-12-24 22:46 ` Ken Brown 0 siblings, 1 reply; 35+ messages in thread From: Jeremy Drake @ 2021-12-24 19:42 UTC (permalink / raw) To: Ken Brown; +Cc: cygwin-patches On Fri, 24 Dec 2021, Ken Brown wrote: > I agree that it's hard to see how the line you quoted could cause an > exception. But you were using an optimized build, so I'm not sure how > reliable the line-number information is. > > Is it feasible to run your test under strace? If so, you could add some > debug_printf statements to examine the values of n_handle and > phi->NumberOfHandles. Or what about simply adding an assertion that > phi->NumberOfHandles <= n_handle? > > Ken This issue is not consistent, I was able to reproduce once on x64 by running commands in a loop overnight, but the next time I tried to reproduce I ran for over 24 hours without hitting it. It does seem to happen much more often on Windows on ARM64 (so much so that at first I thought it was an issue with their emulation). With this patch I have not seen the issue again. Also, it seems to have started cropping up in msys2's CI when the GHA runner was switched from "windows-2019" to "windows-2022". I forgot to give a full link to the MSYS2 issue where I have been investigating this: https://github.com/msys2/MSYS2-packages/issues/2752 ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] fhandler_pipe: add sanity limit to handle loops 2021-12-24 19:42 ` Jeremy Drake @ 2021-12-24 22:46 ` Ken Brown 2021-12-24 23:42 ` Jeremy Drake 2021-12-25 0:39 ` Jeremy Drake 0 siblings, 2 replies; 35+ messages in thread From: Ken Brown @ 2021-12-24 22:46 UTC (permalink / raw) To: Jeremy Drake; +Cc: cygwin-patches On 12/24/2021 2:42 PM, Jeremy Drake wrote: > On Fri, 24 Dec 2021, Ken Brown wrote: > >> I agree that it's hard to see how the line you quoted could cause an >> exception. But you were using an optimized build, so I'm not sure how >> reliable the line-number information is. >> >> Is it feasible to run your test under strace? If so, you could add some >> debug_printf statements to examine the values of n_handle and >> phi->NumberOfHandles. Or what about simply adding an assertion that >> phi->NumberOfHandles <= n_handle? >> >> Ken > > This issue is not consistent, I was able to reproduce once on x64 by > running commands in a loop overnight, but the next time I tried to > reproduce I ran for over 24 hours without hitting it. > > It does seem to happen much more often on Windows on ARM64 (so much so > that at first I thought it was an issue with their emulation). With this > patch I have not seen the issue again. So can you test your diagnosis by removing your patch and adding an assertion? > Also, it seems to have started cropping up in msys2's CI when the GHA > runner was switched from "windows-2019" to "windows-2022". And does your patch help here too? > I forgot to give a full link to the MSYS2 issue where I have been > investigating this: > https://github.com/msys2/MSYS2-packages/issues/2752 Actually I think I might see a small bug in the code. But even if I'm right, it would result in n_handle being unnecessarily big rather than too small, so it wouldn't explain what you're seeing. Takashi, in fhandler_pipe.cc:1225, shouldn't you use offsetof(struct _PROCESS_HANDLE_SNAPSHOT_INFORMATION, Handles) instead of 2*sizeof(ULONG_PTR), to take account of possible padding? (And there's a similar issue in fhandler_pipe.cc:1296.) Ken ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] fhandler_pipe: add sanity limit to handle loops 2021-12-24 22:46 ` Ken Brown @ 2021-12-24 23:42 ` Jeremy Drake 2021-12-25 0:39 ` Jeremy Drake 1 sibling, 0 replies; 35+ messages in thread From: Jeremy Drake @ 2021-12-24 23:42 UTC (permalink / raw) To: cygwin-patches On Fri, 24 Dec 2021, Ken Brown wrote: > So can you test your diagnosis by removing your patch and adding an assertion? I will attempt to do this. Do I need any special configure args or anything for assertions to be enabled? > > Also, it seems to have started cropping up in msys2's CI when the GHA > > runner was switched from "windows-2019" to "windows-2022". > > And does your patch help here too? I have not tried this. This seemed to be pretty clearly a cygwin rather than msys2-specific issue so I submitted the patch here first rather than getting it integrated into msys2 so that it would then be used on their runners. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] fhandler_pipe: add sanity limit to handle loops 2021-12-24 22:46 ` Ken Brown 2021-12-24 23:42 ` Jeremy Drake @ 2021-12-25 0:39 ` Jeremy Drake 2021-12-25 3:19 ` Takashi Yano 1 sibling, 1 reply; 35+ messages in thread From: Jeremy Drake @ 2021-12-25 0:39 UTC (permalink / raw) To: Ken Brown; +Cc: cygwin-patches On Fri, 24 Dec 2021, Ken Brown wrote: > On 12/24/2021 2:42 PM, Jeremy Drake wrote: > > It does seem to happen much more often on Windows on ARM64 (so much so > > that at first I thought it was an issue with their emulation). With this > > patch I have not seen the issue again. > > So can you test your diagnosis by removing your patch and adding an assertion? Done: :: Starting core system upgrade... there is nothing to do :: Starting full system upgrade... there is nothing to do assertion "phi->NumberOfHandles < n_handle" failed: file "../../.././winsup/cygwin/fhandler_pipe.cc", line 1275, function: void* fhandler_pipe::get_query_hdl_per_process(WCHAR*, OBJECT_NAME_INFORMATION*) Aborted ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] fhandler_pipe: add sanity limit to handle loops 2021-12-25 0:39 ` Jeremy Drake @ 2021-12-25 3:19 ` Takashi Yano 2021-12-25 3:47 ` Jeremy Drake 0 siblings, 1 reply; 35+ messages in thread From: Takashi Yano @ 2021-12-25 3:19 UTC (permalink / raw) To: cygwin-patches On Fri, 24 Dec 2021 16:39:13 -0800 (PST) Jeremy Drake wrote: > On Fri, 24 Dec 2021, Ken Brown wrote: > > > On 12/24/2021 2:42 PM, Jeremy Drake wrote: > > > It does seem to happen much more often on Windows on ARM64 (so much so > > > that at first I thought it was an issue with their emulation). With this > > > patch I have not seen the issue again. > > > > So can you test your diagnosis by removing your patch and adding an assertion? > > Done: > :: Starting core system upgrade... > there is nothing to do > :: Starting full system upgrade... > there is nothing to do > assertion "phi->NumberOfHandles < n_handle" failed: file > "../../.././winsup/cygwin/fhandler_pipe.cc", line 1275, function: void* > fhandler_pipe::get_query_hdl_per_process(WCHAR*, OBJECT_NAME_INFORMATION*) > Aborted Could you please try assert(phi->NumberOfHandles <= n_handle) rather than assert(phi->NumberOfHandles < n_handle) ? -- Takashi Yano <takashi.yano@nifty.ne.jp> ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] fhandler_pipe: add sanity limit to handle loops 2021-12-25 3:19 ` Takashi Yano @ 2021-12-25 3:47 ` Jeremy Drake 2021-12-25 4:12 ` Takashi Yano 0 siblings, 1 reply; 35+ messages in thread From: Jeremy Drake @ 2021-12-25 3:47 UTC (permalink / raw) To: cygwin-patches On Sat, 25 Dec 2021, Takashi Yano wrote: > Could you please try > assert(phi->NumberOfHandles <= n_handle) > rather than > assert(phi->NumberOfHandles < n_handle) > ? I thought of that when I was re-reading my email. I also added a printf: index 9ce140089..4d10451c1 100644 --- a/winsup/cygwin/fhandler_pipe.cc +++ b/winsup/cygwin/fhandler_pipe.cc @@ -11,6 +11,7 @@ details. */ #include "winsup.h" #include <stdlib.h> #include <sys/socket.h> +#include <assert.h> #include "cygerrno.h" #include "security.h" #include "path.h" @@ -1271,6 +1272,13 @@ fhandler_pipe::get_query_hdl_per_process (WCHAR *name, if (!NT_SUCCESS (status)) goto close_proc; + if (phi->NumberOfHandles > n_handle) + { + small_printf ("phi->NumberOfHandles = %lu, n_handle = %lu\n", + (unsigned long) phi->NumberOfHandles, + (unsigned long) n_handle); + assert(phi->NumberOfHandles <= n_handle); + } for (ULONG j = 0; j < phi->NumberOfHandles; j++) { /* Check for the peculiarity of cygwin read pipe */ phi->NumberOfHandles = 7999168, n_handle = 256 assertion "phi->NumberOfHandles <= n_handle" failed: file "../../.././winsup/cygwin/fhandler_pipe.cc", line 1280, function: void* fhandler_pipe::get_query_hdl_per_process(WCHAR*, OBJECT_NAME_INFORMATION*) Aborted ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] fhandler_pipe: add sanity limit to handle loops 2021-12-25 3:47 ` Jeremy Drake @ 2021-12-25 4:12 ` Takashi Yano 2021-12-25 5:40 ` Jeremy Drake 0 siblings, 1 reply; 35+ messages in thread From: Takashi Yano @ 2021-12-25 4:12 UTC (permalink / raw) To: cygwin-patches On Fri, 24 Dec 2021 19:47:46 -0800 (PST) Jeremy Drake wrote: > phi->NumberOfHandles = 7999168, n_handle = 256 > assertion "phi->NumberOfHandles <= n_handle" failed: file > "../../.././winsup/cygwin/fhandler_pipe.cc", line 1280, function: void* > fhandler_pipe::get_query_hdl_per_process(WCHAR*, OBJECT_NAME_INFORMATION*) > Aborted What!? Could you please check value of the "status" ? What version of windows do you use? -- Takashi Yano <takashi.yano@nifty.ne.jp> ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] fhandler_pipe: add sanity limit to handle loops 2021-12-25 4:12 ` Takashi Yano @ 2021-12-25 5:40 ` Jeremy Drake 2021-12-25 17:10 ` Takashi Yano 0 siblings, 1 reply; 35+ messages in thread From: Jeremy Drake @ 2021-12-25 5:40 UTC (permalink / raw) To: cygwin-patches On Sat, 25 Dec 2021, Takashi Yano wrote: > On Fri, 24 Dec 2021 19:47:46 -0800 (PST) > Jeremy Drake wrote: > > phi->NumberOfHandles = 7999168, n_handle = 256 > > assertion "phi->NumberOfHandles <= n_handle" failed: file > > "../../.././winsup/cygwin/fhandler_pipe.cc", line 1280, function: void* > > fhandler_pipe::get_query_hdl_per_process(WCHAR*, OBJECT_NAME_INFORMATION*) > > Aborted > > What!? Could you please check value of the "status" ? status = 0x00000000, phi->NumberOfHandles = 7286688, n_handle = 256 assertion "phi->NumberOfHandles <= n_handle" failed: file "../../.././winsup/cygwin/fhandler_pipe.cc", line 1281, function: void* fhandler_pipe::get_query_hdl_per_process(WCHAR*, OBJECT_NAME_INFORMATION*) Aborted > What version of windows do you use? This was on Windows 11 (22000.376) on ARM64, but msys2 has started seeing similar hangs on Github's "windows-2022" runner. I don't have one of those locally to test against however. But if push came to shove, I think I downloaded a Server 2022 evaluation ISO, I could set up a VM and see what happens. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] fhandler_pipe: add sanity limit to handle loops 2021-12-25 5:40 ` Jeremy Drake @ 2021-12-25 17:10 ` Takashi Yano 2021-12-25 17:16 ` Takashi Yano 2021-12-25 19:20 ` Jeremy Drake 0 siblings, 2 replies; 35+ messages in thread From: Takashi Yano @ 2021-12-25 17:10 UTC (permalink / raw) To: cygwin-patches On Fri, 24 Dec 2021 21:40:24 -0800 (PST) Jeremy Drake wrote: > On Sat, 25 Dec 2021, Takashi Yano wrote: > > > On Fri, 24 Dec 2021 19:47:46 -0800 (PST) > > Jeremy Drake wrote: > > > phi->NumberOfHandles = 7999168, n_handle = 256 > > > assertion "phi->NumberOfHandles <= n_handle" failed: file > > > "../../.././winsup/cygwin/fhandler_pipe.cc", line 1280, function: void* > > > fhandler_pipe::get_query_hdl_per_process(WCHAR*, OBJECT_NAME_INFORMATION*) > > > Aborted > > > > What!? Could you please check value of the "status" ? > > status = 0x00000000, phi->NumberOfHandles = 7286688, n_handle = 256 > assertion "phi->NumberOfHandles <= n_handle" failed: file > "../../.././winsup/cygwin/fhandler_pipe.cc", line 1281, function: void* > fhandler_pipe::get_query_hdl_per_process(WCHAR*, OBJECT_NAME_INFORMATION*) > Aborted > > > What version of windows do you use? > > This was on Windows 11 (22000.376) on ARM64, but msys2 has started seeing > similar hangs on Github's "windows-2022" runner. I don't have one of > those locally to test against however. But if push came to shove, I think > I downloaded a Server 2022 evaluation ISO, I could set up a VM and see > what happens. Could you please check the result of the following test case in that ARM64 platform? The following code can be compiled using mingw compiper with -lntdll flag. #include <windows.h> #include <ntdef.h> #include <ntstatus.h> #include <stdlib.h> #include <stdio.h> typedef enum { ProcessHandleInformation = 51 /* Since Win8 */ } PROCESSINFOCLASS; typedef struct { HANDLE HandleValue; ULONG_PTR HandleCount; ULONG_PTR PointerCount; ULONG GrantedAccess; ULONG ObjectTypeIndex; ULONG HandleAttributes; ULONG Reserved; } PROCESS_HANDLE_TABLE_ENTRY_INFO, *PPROCESS_HANDLE_TABLE_ENTRY_INFO; typedef struct { ULONG_PTR NumberOfHandles; ULONG_PTR Reserved; PROCESS_HANDLE_TABLE_ENTRY_INFO Handles[1]; } PROCESS_HANDLE_SNAPSHOT_INFORMATION; NTSTATUS NTAPI NtQueryInformationProcess (HANDLE, PROCESSINFOCLASS, PVOID, ULONG, PULONG); typedef enum { SystemHandleInformation = 16 } SYSTEM_INFORMATION_CLASS; typedef struct { USHORT UniqueProcessId; USHORT CreatorBackTraceIndex; UCHAR ObjectTypeIndex; UCHAR HandleAttributes; USHORT HandleValue; PVOID Object; ULONG GrantedAccess; } SYSTEM_HANDLE_TABLE_ENTRY_INFO; typedef struct { ULONG NumberOfHandles; SYSTEM_HANDLE_TABLE_ENTRY_INFO Handles[1]; } SYSTEM_HANDLE_INFORMATION; NTSTATUS NTAPI NtQuerySystemInformation (SYSTEM_INFORMATION_CLASS, PVOID, ULONG, PULONG); int main() { NTSTATUS status; DWORD n_handle = 1; PROCESS_HANDLE_SNAPSHOT_INFORMATION *phi; do { DWORD nbytes = 2 * sizeof(ULONG_PTR) + n_handle * sizeof(PROCESS_HANDLE_TABLE_ENTRY_INFO); phi = (PROCESS_HANDLE_SNAPSHOT_INFORMATION *) HeapAlloc(GetProcessHeap(), 0, nbytes); if (!phi) { fprintf(stderr, "HeapAlloc() Error: %08x\n", GetLastError()); exit(1); } ULONG len; status = NtQueryInformationProcess(GetCurrentProcess(), ProcessHandleInformation, phi, nbytes, &len); if (NT_SUCCESS (status)) break; HeapFree(GetProcessHeap(), 0, phi); n_handle ++; } while (status == STATUS_INFO_LENGTH_MISMATCH); if (!NT_SUCCESS (status)) { fprintf(stderr, "NtQueryInformationProcess() error: %08x\n", status); HeapFree(GetProcessHeap(), 0, phi); exit(1); } printf("per_process: n_handle=%d, NumberOfHandles=%d\n", n_handle, phi->NumberOfHandles); if (phi->NumberOfHandles > n_handle) { HeapFree(GetProcessHeap(), 0, phi); exit(1); } HeapFree(GetProcessHeap(), 0, phi); n_handle = 1; SYSTEM_HANDLE_INFORMATION *shi; do { SIZE_T nbytes = sizeof(ULONG) + n_handle * sizeof(SYSTEM_HANDLE_TABLE_ENTRY_INFO); shi = (SYSTEM_HANDLE_INFORMATION *) HeapAlloc (GetProcessHeap(), 0, nbytes); if (!shi) { fprintf(stderr, "HeapAlloc() Error: %08x\n", GetLastError()); exit(1); } status = NtQuerySystemInformation(SystemHandleInformation, shi, nbytes, NULL); if (NT_SUCCESS(status)) break; HeapFree (GetProcessHeap(), 0, shi); n_handle *= 2; } while (status == STATUS_INFO_LENGTH_MISMATCH); if (!NT_SUCCESS (status)) { fprintf(stderr, "NtQuerySystemInformation() error: %08x\n", status); HeapFree(GetProcessHeap(), 0, shi); exit(1); } printf("per_system: n_handle=%d, NumberOfHandles=%d\n", n_handle, shi->NumberOfHandles); if (shi->NumberOfHandles > n_handle) { HeapFree(GetProcessHeap(), 0, shi); exit(1); } HeapFree(GetProcessHeap(), 0, shi); return 0; } -- Takashi Yano <takashi.yano@nifty.ne.jp> ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] fhandler_pipe: add sanity limit to handle loops 2021-12-25 17:10 ` Takashi Yano @ 2021-12-25 17:16 ` Takashi Yano 2021-12-25 19:00 ` Marco Atzeri 2021-12-25 19:20 ` Jeremy Drake 1 sibling, 1 reply; 35+ messages in thread From: Takashi Yano @ 2021-12-25 17:16 UTC (permalink / raw) To: cygwin-patches On Sun, 26 Dec 2021 02:10:10 +0900 Takashi Yano wrote: > if (phi->NumberOfHandles > n_handle) { > HeapFree(GetProcessHeap(), 0, phi); > exit(1); > } [...] > if (shi->NumberOfHandles > n_handle) { > HeapFree(GetProcessHeap(), 0, shi); > exit(1); > } Sorry, please remove above lines. -- Takashi Yano <takashi.yano@nifty.ne.jp> ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] fhandler_pipe: add sanity limit to handle loops 2021-12-25 17:16 ` Takashi Yano @ 2021-12-25 19:00 ` Marco Atzeri 0 siblings, 0 replies; 35+ messages in thread From: Marco Atzeri @ 2021-12-25 19:00 UTC (permalink / raw) To: cygwin-patches On 25.12.2021 18:16, Takashi Yano wrote: > On Sun, 26 Dec 2021 02:10:10 +0900 > Takashi Yano wrote: >> if (phi->NumberOfHandles > n_handle) { >> HeapFree(GetProcessHeap(), 0, phi); >> exit(1); >> } > [...] >> if (shi->NumberOfHandles > n_handle) { >> HeapFree(GetProcessHeap(), 0, shi); >> exit(1); >> } > > Sorry, please remove above lines. > I think it is better to put as attachment the full requested test code. Regards Marco ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] fhandler_pipe: add sanity limit to handle loops 2021-12-25 17:10 ` Takashi Yano 2021-12-25 17:16 ` Takashi Yano @ 2021-12-25 19:20 ` Jeremy Drake 2021-12-25 22:18 ` Ken Brown 2021-12-25 23:00 ` Jeremy Drake 1 sibling, 2 replies; 35+ messages in thread From: Jeremy Drake @ 2021-12-25 19:20 UTC (permalink / raw) To: Takashi Yano; +Cc: cygwin-patches On Sun, 26 Dec 2021, Takashi Yano wrote: > Could you please check the result of the following test case > in that ARM64 platform? > I will probably not be able to get to this until tomorrow at the earliest. But keep in mind the issue I'm seeing is not deterministic - I have to run pacman in a loop validating files via GPGME and eventually it will hang (or hit the assert I added in that version). Most of the time, it's perfectly fine. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] fhandler_pipe: add sanity limit to handle loops 2021-12-25 19:20 ` Jeremy Drake @ 2021-12-25 22:18 ` Ken Brown 2021-12-25 23:00 ` Jeremy Drake 1 sibling, 0 replies; 35+ messages in thread From: Ken Brown @ 2021-12-25 22:18 UTC (permalink / raw) To: Jeremy Drake, Takashi Yano; +Cc: cygwin-patches On 12/25/2021 2:20 PM, Jeremy Drake via Cygwin-patches wrote: > On Sun, 26 Dec 2021, Takashi Yano wrote: > >> Could you please check the result of the following test case >> in that ARM64 platform? >> > > I will probably not be able to get to this until tomorrow at the earliest. > But keep in mind the issue I'm seeing is not deterministic - I have to run > pacman in a loop validating files via GPGME and eventually it will hang > (or hit the assert I added in that version). Most of the time, it's > perfectly fine. The results you've already posted seem to indicate that, on your platform, NtQueryInformationProcess(ProcessHandleInformation) returns STATUS_SUCCESS even if the buffer it's passed is too small. [That won't necessarily cause a problem in every one of your pacman runs, so it might appear non-deterministic.] Takashi's test case is designed to verify that that's what's going on. And I think he also wants to see if phi->NumberOfHandles is reliable on your platform even when the buffer is too small. If so, then (on your platform), the do-while loop could be replaced by two calls to NtQueryInformationProcess. The first call would determine how big a buffer is needed, and the second call would use a buffer of that size. But we don't know any of that for sure yet. We also don't know (or at least I don't know) what aspects of your platform are relevant. For example, does this always happen on Windows 11? or on ARM64? Ken ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] fhandler_pipe: add sanity limit to handle loops 2021-12-25 19:20 ` Jeremy Drake 2021-12-25 22:18 ` Ken Brown @ 2021-12-25 23:00 ` Jeremy Drake 2021-12-26 3:04 ` Ken Brown 1 sibling, 1 reply; 35+ messages in thread From: Jeremy Drake @ 2021-12-25 23:00 UTC (permalink / raw) To: Takashi Yano; +Cc: cygwin-patches On Sat, 25 Dec 2021, Jeremy Drake via Cygwin-patches wrote: > On Sun, 26 Dec 2021, Takashi Yano wrote: > > > Could you please check the result of the following test case > > in that ARM64 platform? > > > OK, on Windows 11 ARM64 (same machine as I was testing the assert on): per_process: n_handle=52, NumberOfHandles=52 per_system: n_handle=65536, NumberOfHandles=35331 On GitHub "windows-2022" runner: per_process: n_handle=63, NumberOfHandles=63 per_system: n_handle=65536, NumberOfHandles=34077 On Windows 10 x86_64 (can't remember if it was 21H1 or 21H2): per_process: n_handle=37, NumberOfHandles=37 per_system: n_handle=131072, NumberOfHandles=76069 I was able to reproduce on that Windows 10 box *once*, when I got the stack traces, but not since. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] fhandler_pipe: add sanity limit to handle loops 2021-12-25 23:00 ` Jeremy Drake @ 2021-12-26 3:04 ` Ken Brown 2021-12-26 4:56 ` Jeremy Drake 0 siblings, 1 reply; 35+ messages in thread From: Ken Brown @ 2021-12-26 3:04 UTC (permalink / raw) To: Jeremy Drake, Takashi Yano; +Cc: cygwin-patches On 12/25/2021 6:00 PM, Jeremy Drake via Cygwin-patches wrote: > On Sat, 25 Dec 2021, Jeremy Drake via Cygwin-patches wrote: > >> On Sun, 26 Dec 2021, Takashi Yano wrote: >> >>> Could you please check the result of the following test case >>> in that ARM64 platform? >>> >> > > OK, on Windows 11 ARM64 (same machine as I was testing the assert on): > per_process: n_handle=52, NumberOfHandles=52 > per_system: n_handle=65536, NumberOfHandles=35331 > > On GitHub "windows-2022" runner: > per_process: n_handle=63, NumberOfHandles=63 > per_system: n_handle=65536, NumberOfHandles=34077 > > On Windows 10 x86_64 (can't remember if it was 21H1 or 21H2): > per_process: n_handle=37, NumberOfHandles=37 > per_system: n_handle=131072, NumberOfHandles=76069 This completely shoots down the speculation in my last email. Nevertheless, the results you posted earlier do indicate that *sometimes* NtQueryInformationProcess(ProcessHandleInformation) returns STATUS_SUCCESS even if the buffer it's passed is too small. I hope Takashi has an idea what's going on. Ken ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] fhandler_pipe: add sanity limit to handle loops 2021-12-26 3:04 ` Ken Brown @ 2021-12-26 4:56 ` Jeremy Drake 2021-12-26 15:09 ` Ken Brown 0 siblings, 1 reply; 35+ messages in thread From: Jeremy Drake @ 2021-12-26 4:56 UTC (permalink / raw) To: Ken Brown; +Cc: Takashi Yano, cygwin-patches I set up a windows server 2022 VM last night and went nuts stressing pacman/GPGME. I was able to reproduce the issue there: status = 0x00000000, phi->NumberOfHandles = 8261392, n_handle = 256 [#####----------------------------------] 14% assertion "phi->NumberOfHandles <= n_handle" failed: file "../../.././winsup/cygwin/fhandler_pipe.cc", line 1281, function: void* fhandler_pipe::get_query_hdl_per_process(WCHAR*, OBJECT_NAME_INFORMATION*) So it is not something inherent in the x86_64-on-ARM64 emulation but can happen on native x86_64 also. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] fhandler_pipe: add sanity limit to handle loops 2021-12-26 4:56 ` Jeremy Drake @ 2021-12-26 15:09 ` Ken Brown 2021-12-26 16:04 ` Ken Brown 0 siblings, 1 reply; 35+ messages in thread From: Ken Brown @ 2021-12-26 15:09 UTC (permalink / raw) To: Jeremy Drake; +Cc: Takashi Yano, cygwin-patches On 12/25/2021 11:56 PM, Jeremy Drake wrote: > I set up a windows server 2022 VM last night and went nuts stressing > pacman/GPGME. I was able to reproduce the issue there: > > status = 0x00000000, phi->NumberOfHandles = 8261392, n_handle = 256 > [#####----------------------------------] 14% > assertion "phi->NumberOfHandles <= n_handle" failed: file > "../../.././winsup/cygwin/fhandler_pipe.cc", line 1281, function: void* > fhandler_pipe::get_query_hdl_per_process(WCHAR*, OBJECT_NAME_INFORMATION*) > > So it is not something inherent in the x86_64-on-ARM64 emulation but can > happen on native x86_64 also. A Google search led me to something that might explain what's going on. Look at the function PhEnumHandlesEx2 starting at line 5713 in https://github.com/processhacker/processhacker/blob/master/phlib/native.c#L5152 Two interesting things: 1. For some processes, NtQueryInformationProcess(ProcessHandleInformation) can return STATUS_SUCCESS with invalid handle information. See the comment starting at line 5754, where it is shown how to detect this. 2. You can use the ReturnLength parameter of NtQueryInformationProcess to see how big a buffer is needed. This might be more efficient than repeatedly doubling the buffer size. Ken ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] fhandler_pipe: add sanity limit to handle loops 2021-12-26 15:09 ` Ken Brown @ 2021-12-26 16:04 ` Ken Brown 2021-12-26 16:24 ` Ken Brown 0 siblings, 1 reply; 35+ messages in thread From: Ken Brown @ 2021-12-26 16:04 UTC (permalink / raw) To: Jeremy Drake; +Cc: cygwin-patches On 12/26/2021 10:09 AM, Ken Brown wrote: > 1. For some processes, NtQueryInformationProcess(ProcessHandleInformation) can > return STATUS_SUCCESS with invalid handle information. See the comment starting > at line 5754, where it is shown how to detect this. If I'm right, the following patch should fix the problem: diff --git a/winsup/cygwin/fhandler_pipe.cc b/winsup/cygwin/fhandler_pipe.cc index ba6b70f55..4cef3e4ca 100644 --- a/winsup/cygwin/fhandler_pipe.cc +++ b/winsup/cygwin/fhandler_pipe.cc @@ -1228,6 +1228,7 @@ fhandler_pipe::get_query_hdl_per_process (WCHAR *name, HeapAlloc (GetProcessHeap (), 0, nbytes); if (!phi) goto close_proc; + phi->NumberOfHandles = 0; status = NtQueryInformationProcess (proc, ProcessHandleInformation, phi, nbytes, &len); if (NT_SUCCESS (status)) @@ -1238,6 +1239,11 @@ fhandler_pipe::get_query_hdl_per_process (WCHAR *name, while (n_handle < (1L<<20) && status == STATUS_INFO_LENGTH_MISMATCH); if (!NT_SUCCESS (status)) goto close_proc; + if (phi->NumberOfHandles == 0) + { + HeapFree (GetProcessHeap (), 0, phi); + goto close_proc; + } for (ULONG j = 0; j < phi->NumberOfHandles; j++) { Jeremy, could you try this? Ken ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] fhandler_pipe: add sanity limit to handle loops 2021-12-26 16:04 ` Ken Brown @ 2021-12-26 16:24 ` Ken Brown 2021-12-26 21:35 ` Jeremy Drake 0 siblings, 1 reply; 35+ messages in thread From: Ken Brown @ 2021-12-26 16:24 UTC (permalink / raw) To: Jeremy Drake; +Cc: cygwin-patches On 12/26/2021 11:04 AM, Ken Brown wrote: > On 12/26/2021 10:09 AM, Ken Brown wrote: >> 1. For some processes, NtQueryInformationProcess(ProcessHandleInformation) can >> return STATUS_SUCCESS with invalid handle information. See the comment >> starting at line 5754, where it is shown how to detect this. > > If I'm right, the following patch should fix the problem: > > diff --git a/winsup/cygwin/fhandler_pipe.cc b/winsup/cygwin/fhandler_pipe.cc > index ba6b70f55..4cef3e4ca 100644 > --- a/winsup/cygwin/fhandler_pipe.cc > +++ b/winsup/cygwin/fhandler_pipe.cc > @@ -1228,6 +1228,7 @@ fhandler_pipe::get_query_hdl_per_process (WCHAR *name, > HeapAlloc (GetProcessHeap (), 0, nbytes); > if (!phi) > goto close_proc; > + phi->NumberOfHandles = 0; > status = NtQueryInformationProcess (proc, ProcessHandleInformation, > phi, nbytes, &len); > if (NT_SUCCESS (status)) Actually, this first hunk should suffice. > @@ -1238,6 +1239,11 @@ fhandler_pipe::get_query_hdl_per_process (WCHAR *name, > while (n_handle < (1L<<20) && status == STATUS_INFO_LENGTH_MISMATCH); > if (!NT_SUCCESS (status)) > goto close_proc; > + if (phi->NumberOfHandles == 0) > + { > + HeapFree (GetProcessHeap (), 0, phi); > + goto close_proc; > + } > > for (ULONG j = 0; j < phi->NumberOfHandles; j++) > { > > Jeremy, could you try this? > > Ken ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] fhandler_pipe: add sanity limit to handle loops 2021-12-26 16:24 ` Ken Brown @ 2021-12-26 21:35 ` Jeremy Drake 2021-12-26 22:18 ` Ken Brown 0 siblings, 1 reply; 35+ messages in thread From: Jeremy Drake @ 2021-12-26 21:35 UTC (permalink / raw) To: Ken Brown; +Cc: cygwin-patches On Sun, 26 Dec 2021, Ken Brown wrote: > On 12/26/2021 11:04 AM, Ken Brown wrote: > > On 12/26/2021 10:09 AM, Ken Brown wrote: > > > 1. For some processes, NtQueryInformationProcess(ProcessHandleInformation) > > > can return STATUS_SUCCESS with invalid handle information. See the > > > comment starting at line 5754, where it is shown how to detect this. I kind of thought something like this (that NumberOfHandles was uninitialized memory). > > If I'm right, the following patch should fix the problem: > > > > diff --git a/winsup/cygwin/fhandler_pipe.cc b/winsup/cygwin/fhandler_pipe.cc > > index ba6b70f55..4cef3e4ca 100644 > > --- a/winsup/cygwin/fhandler_pipe.cc > > +++ b/winsup/cygwin/fhandler_pipe.cc > > @@ -1228,6 +1228,7 @@ fhandler_pipe::get_query_hdl_per_process (WCHAR *name, > > HeapAlloc (GetProcessHeap (), 0, nbytes); > > if (!phi) > > goto close_proc; > > + phi->NumberOfHandles = 0; > > status = NtQueryInformationProcess (proc, > > ProcessHandleInformation, > > phi, nbytes, &len); > > if (NT_SUCCESS (status)) > > Actually, this first hunk should suffice. > > > Jeremy, could you try this? > > > > Ken I've built (leaving the assert in place too), and I've got 3 loops going on server 2022 and 1 going on ARM64. So far so good. I don't know how long before calling it good though. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] fhandler_pipe: add sanity limit to handle loops 2021-12-26 21:35 ` Jeremy Drake @ 2021-12-26 22:18 ` Ken Brown 2021-12-26 22:43 ` Jeremy Drake 0 siblings, 1 reply; 35+ messages in thread From: Ken Brown @ 2021-12-26 22:18 UTC (permalink / raw) To: Jeremy Drake; +Cc: cygwin-patches [-- Attachment #1: Type: text/plain, Size: 1813 bytes --] On 12/26/2021 4:35 PM, Jeremy Drake wrote: > On Sun, 26 Dec 2021, Ken Brown wrote: > >> On 12/26/2021 11:04 AM, Ken Brown wrote: >>> On 12/26/2021 10:09 AM, Ken Brown wrote: >>>> 1. For some processes, NtQueryInformationProcess(ProcessHandleInformation) >>>> can return STATUS_SUCCESS with invalid handle information. See the >>>> comment starting at line 5754, where it is shown how to detect this. > > I kind of thought something like this (that NumberOfHandles was > uninitialized memory). > >>> If I'm right, the following patch should fix the problem: >>> >>> diff --git a/winsup/cygwin/fhandler_pipe.cc b/winsup/cygwin/fhandler_pipe.cc >>> index ba6b70f55..4cef3e4ca 100644 >>> --- a/winsup/cygwin/fhandler_pipe.cc >>> +++ b/winsup/cygwin/fhandler_pipe.cc >>> @@ -1228,6 +1228,7 @@ fhandler_pipe::get_query_hdl_per_process (WCHAR *name, >>> HeapAlloc (GetProcessHeap (), 0, nbytes); >>> if (!phi) >>> goto close_proc; >>> + phi->NumberOfHandles = 0; >>> status = NtQueryInformationProcess (proc, >>> ProcessHandleInformation, >>> phi, nbytes, &len); >>> if (NT_SUCCESS (status)) >> >> Actually, this first hunk should suffice. >> >>> Jeremy, could you try this? >>> >>> Ken > > > I've built (leaving the assert in place too), and I've got 3 loops going > on server 2022 and 1 going on ARM64. So far so good. I don't know how > long before calling it good though. Great, thanks for testing. I'm attaching the complete patch (with documentation). I'll push it once you're convinced that it fixes the problem, assuming Takashi agrees. (I think Corinna is unavailable.) Ken [-- Attachment #2: 0001-Cygwin-fhandler_pipe-get_query_hdl_per_process-avoid.patch --] [-- Type: text/plain, Size: 2146 bytes --] From 4858e73321a0618a8b1e1060416ef7d546cda895 Mon Sep 17 00:00:00 2001 From: Ken Brown <kbrown@cornell.edu> Date: Sun, 26 Dec 2021 16:42:26 -0500 Subject: [PATCH] Cygwin: fhandler_pipe::get_query_hdl_per_process: avoid a crash NtQueryInformationProcess(ProcessHandleInformation) can return STATUS_SUCCESS with invalid handle data for certain processes ("minimal" processes on Windows 10). This can cause a crash when there's an attempt to access that data. Fix that by setting NumberOfHandles to zero before calling NtQueryInformationProcess. Addresses: https://cygwin.com/pipermail/cygwin-patches/2021q4/011611.html --- winsup/cygwin/fhandler_pipe.cc | 6 ++++++ winsup/cygwin/release/3.3.4 | 3 +++ 2 files changed, 9 insertions(+) diff --git a/winsup/cygwin/fhandler_pipe.cc b/winsup/cygwin/fhandler_pipe.cc index 25a092262..2674d154c 100644 --- a/winsup/cygwin/fhandler_pipe.cc +++ b/winsup/cygwin/fhandler_pipe.cc @@ -1256,6 +1256,12 @@ fhandler_pipe::get_query_hdl_per_process (WCHAR *name, HeapAlloc (GetProcessHeap (), 0, nbytes); if (!phi) goto close_proc; + /* NtQueryInformationProcess can return STATUS_SUCCESS with + invalid handle data for certain processes. See + https://github.com/processhacker/processhacker/blob/master/phlib/native.c#L5754. + We need to ensure that NumberOfHandles is zero in this + case to avoid a crash in the loop below. */ + phi->NumberOfHandles = 0; status = NtQueryInformationProcess (proc, ProcessHandleInformation, phi, nbytes, &len); if (NT_SUCCESS (status)) diff --git a/winsup/cygwin/release/3.3.4 b/winsup/cygwin/release/3.3.4 index a15684fdb..048426942 100644 --- a/winsup/cygwin/release/3.3.4 +++ b/winsup/cygwin/release/3.3.4 @@ -14,3 +14,6 @@ Bug Fixes rather than io_handle while neither read() nor select() is called after the cygwin app is started from non-cygwin app. Addresses: https://cygwin.com/pipermail/cygwin-patches/2021q4/011587.html + +- Avoid a crash when NtQueryInformationProcess returns invalid handle data. + Addresses: https://cygwin.com/pipermail/cygwin-patches/2021q4/011611.html -- 2.34.1 ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] fhandler_pipe: add sanity limit to handle loops 2021-12-26 22:18 ` Ken Brown @ 2021-12-26 22:43 ` Jeremy Drake 2021-12-26 23:12 ` Ken Brown 0 siblings, 1 reply; 35+ messages in thread From: Jeremy Drake @ 2021-12-26 22:43 UTC (permalink / raw) To: Ken Brown; +Cc: cygwin-patches On Sun, 26 Dec 2021, Ken Brown wrote: > + /* NtQueryInformationProcess can return STATUS_SUCCESS with > + invalid handle data for certain processes. See > + https://github.com/processhacker/processhacker/blob/master/phlib/native.c#L5754. I would recommend using the "permalink" to the line, since future commits could change both the line number and even the comment you are referring to. https://github.com/processhacker/processhacker/blob/05f5e9fa477dcaa1709d9518170d18e1b3b8330d/phlib/native.c#L5754 > + We need to ensure that NumberOfHandles is zero in this > + case to avoid a crash in the loop below. */ > + phi->NumberOfHandles = 0; > status = NtQueryInformationProcess (proc, ProcessHandleInformation, > phi, nbytes, &len); > if (NT_SUCCESS (status)) Would it make sense to leave an assert (phi->NumberOfHandles <= n_handle) before the for loop too just in case something odd happens in the future? That made it a lot easier to know what was going on. My loops are still going after an hour. I know that ARM64 would have hit the assert before now. Would this also be pushed to the 3.3 branch? Or are there plans to make a 3.3.4 at some point? I saw a pipe-related hang reported to MSYS2 (that I didn't see this issue in the stack traces), but I am not sure if there are any more pipe fixes pending post 3.3.3. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] fhandler_pipe: add sanity limit to handle loops 2021-12-26 22:43 ` Jeremy Drake @ 2021-12-26 23:12 ` Ken Brown 2021-12-26 23:23 ` Jeremy Drake 2021-12-29 21:59 ` Ken Brown 0 siblings, 2 replies; 35+ messages in thread From: Ken Brown @ 2021-12-26 23:12 UTC (permalink / raw) To: Jeremy Drake; +Cc: cygwin-patches On 12/26/2021 5:43 PM, Jeremy Drake wrote: > On Sun, 26 Dec 2021, Ken Brown wrote: > >> + /* NtQueryInformationProcess can return STATUS_SUCCESS with >> + invalid handle data for certain processes. See >> + https://github.com/processhacker/processhacker/blob/master/phlib/native.c#L5754. > > I would recommend using the "permalink" to the line, since future > commits could change both the line number and even the comment you are > referring to. > > https://github.com/processhacker/processhacker/blob/05f5e9fa477dcaa1709d9518170d18e1b3b8330d/phlib/native.c#L5754 Good idea, thanks. >> + We need to ensure that NumberOfHandles is zero in this >> + case to avoid a crash in the loop below. */ >> + phi->NumberOfHandles = 0; >> status = NtQueryInformationProcess (proc, ProcessHandleInformation, >> phi, nbytes, &len); >> if (NT_SUCCESS (status)) > > Would it make sense to leave an assert (phi->NumberOfHandles <= n_handle) > before the for loop too just in case something odd happens in the > future? That made it a lot easier to know what was going on. Yes, I think so. > My loops are still going after an hour. I know that ARM64 would have hit > the assert before now. > > Would this also be pushed to the 3.3 branch? Or are there plans to make a > 3.3.4 at some point? I saw a pipe-related hang reported to MSYS2 (that I > didn't see this issue in the stack traces), but I am not sure if there are > any more pipe fixes pending post 3.3.3. There are some fixes (though not pipe-related) pending for 3.3.4, so I'll push it to the 3.3 branch after I've heard from Takashi and/or Corinna. Ken ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] fhandler_pipe: add sanity limit to handle loops 2021-12-26 23:12 ` Ken Brown @ 2021-12-26 23:23 ` Jeremy Drake 2021-12-27 2:42 ` Ken Brown 2021-12-29 21:59 ` Ken Brown 1 sibling, 1 reply; 35+ messages in thread From: Jeremy Drake @ 2021-12-26 23:23 UTC (permalink / raw) To: Ken Brown; +Cc: cygwin-patches On Sun, 26 Dec 2021, Ken Brown wrote: > On 12/26/2021 5:43 PM, Jeremy Drake wrote: > > My loops are still going after an hour. I know that ARM64 would have hit > > the assert before now. Well, ARM64 hung up, but didn't hit the assert, so maybe there's some *other* issue running around. Unfortunately gdb doesn't work to attach to a process there (it gets confused with the ARM64 DLLs loaded in the process). And WinDbg can't load the symbols for a cygwin dll (cv2pdb generally works pretty well for this, but it seems to be confused by the split .dbg file for msys-2.0.dll). ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] fhandler_pipe: add sanity limit to handle loops 2021-12-26 23:23 ` Jeremy Drake @ 2021-12-27 2:42 ` Ken Brown 2021-12-27 21:12 ` Jeremy Drake 0 siblings, 1 reply; 35+ messages in thread From: Ken Brown @ 2021-12-27 2:42 UTC (permalink / raw) To: Jeremy Drake; +Cc: cygwin-patches On 12/26/2021 6:23 PM, Jeremy Drake wrote: > On Sun, 26 Dec 2021, Ken Brown wrote: > >> On 12/26/2021 5:43 PM, Jeremy Drake wrote: >>> My loops are still going after an hour. I know that ARM64 would have hit >>> the assert before now. > > Well, ARM64 hung up, but didn't hit the assert, so maybe there's some > *other* issue running around. Unfortunately gdb doesn't work to attach to > a process there (it gets confused with the ARM64 DLLs loaded in the > process). And WinDbg can't load the symbols for a cygwin dll (cv2pdb > generally works pretty well for this, but it seems to be confused by the > split .dbg file for msys-2.0.dll). Sounds like nothing can be done until it shows up on x86_64. Ken ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] fhandler_pipe: add sanity limit to handle loops 2021-12-27 2:42 ` Ken Brown @ 2021-12-27 21:12 ` Jeremy Drake 0 siblings, 0 replies; 35+ messages in thread From: Jeremy Drake @ 2021-12-27 21:12 UTC (permalink / raw) To: Ken Brown; +Cc: cygwin-patches On Sun, 26 Dec 2021, Ken Brown wrote: > On 12/26/2021 6:23 PM, Jeremy Drake wrote: > > On Sun, 26 Dec 2021, Ken Brown wrote: > > > > > On 12/26/2021 5:43 PM, Jeremy Drake wrote: > > > > My loops are still going after an hour. I know that ARM64 would have > > > > hit > > > > the assert before now. x86_64 server 2022 is still going after 24 hours. I'm going to stop it now, and try to get an msys-2.0.dll with working PDB symbols to hopefully see where ARM64 is hanging up. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] fhandler_pipe: add sanity limit to handle loops 2021-12-26 23:12 ` Ken Brown 2021-12-26 23:23 ` Jeremy Drake @ 2021-12-29 21:59 ` Ken Brown 2021-12-29 23:29 ` Jeremy Drake 1 sibling, 1 reply; 35+ messages in thread From: Ken Brown @ 2021-12-29 21:59 UTC (permalink / raw) To: Jeremy Drake; +Cc: cygwin-patches On 12/26/2021 6:12 PM, Ken Brown wrote: > There are some fixes (though not pipe-related) pending for 3.3.4, so I'll push > it to the 3.3 branch after I've heard from Takashi and/or Corinna. Takashi must be unavailable also, but it's a simple enough fix that I decided to go ahead and push it. Ken ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] fhandler_pipe: add sanity limit to handle loops 2021-12-29 21:59 ` Ken Brown @ 2021-12-29 23:29 ` Jeremy Drake 0 siblings, 0 replies; 35+ messages in thread From: Jeremy Drake @ 2021-12-29 23:29 UTC (permalink / raw) To: Ken Brown; +Cc: cygwin-patches On Wed, 29 Dec 2021, Ken Brown wrote: > Takashi must be unavailable also, but it's a simple enough fix that I decided > to go ahead and push it. Thanks. Regarding the other hang I'm seeing on ARM64, I tried gdb windbg and lldb, and none of them seem able to read the 'context' of the main thread when in this state so I can't get a stack trace. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] fhandler_pipe: add sanity limit to handle loops 2021-12-24 0:29 ` Jeremy Drake 2021-12-24 17:17 ` Ken Brown @ 2021-12-27 20:01 ` Jon Turney 2021-12-29 5:45 ` Jeremy Drake 1 sibling, 1 reply; 35+ messages in thread From: Jon Turney @ 2021-12-27 20:01 UTC (permalink / raw) To: Jeremy Drake, Cygwin Patches On 24/12/2021 00:29, Jeremy Drake via Cygwin-patches wrote: > again, so I can't confirm this. I took a core with 'dumper' but gdb > doesn't want to load it (it says Core file format not supported, maybe > something with msys2's gdb?). I think you need gdb 11 (for this patch set [1], which is also in cygwin's gdb 10 package) to read x86_64 cygwin core dumps. [1] https://sourceware.org/pipermail/gdb-patches/2020-August/171232.html ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] fhandler_pipe: add sanity limit to handle loops 2021-12-27 20:01 ` Jon Turney @ 2021-12-29 5:45 ` Jeremy Drake 2021-12-30 15:44 ` Jon Turney 0 siblings, 1 reply; 35+ messages in thread From: Jeremy Drake @ 2021-12-29 5:45 UTC (permalink / raw) To: Jon Turney; +Cc: Cygwin Patches On Mon, 27 Dec 2021, Jon Turney wrote: > On 24/12/2021 00:29, Jeremy Drake via Cygwin-patches wrote: > > again, so I can't confirm this. I took a core with 'dumper' but gdb > > doesn't want to load it (it says Core file format not supported, maybe > > something with msys2's gdb?). > > I think you need gdb 11 (for this patch set [1], which is also in cygwin's > gdb 10 package) to read x86_64 cygwin core dumps. > > [1] https://sourceware.org/pipermail/gdb-patches/2020-August/171232.html Thanks, this was the problem. But the core dump wasn't much help anyway, the stuff I was interested in was pre-exception, and the backtrace seemed to stop at the exception handling (unlike when 'live' debugging when the stack trace continued). ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] fhandler_pipe: add sanity limit to handle loops 2021-12-29 5:45 ` Jeremy Drake @ 2021-12-30 15:44 ` Jon Turney 0 siblings, 0 replies; 35+ messages in thread From: Jon Turney @ 2021-12-30 15:44 UTC (permalink / raw) To: Cygwin Patches On 29/12/2021 05:45, Jeremy Drake wrote: > On Mon, 27 Dec 2021, Jon Turney wrote: > >> On 24/12/2021 00:29, Jeremy Drake via Cygwin-patches wrote: >>> again, so I can't confirm this. I took a core with 'dumper' but gdb >>> doesn't want to load it (it says Core file format not supported, maybe >>> something with msys2's gdb?). >> >> I think you need gdb 11 (for this patch set [1], which is also in cygwin's >> gdb 10 package) to read x86_64 cygwin core dumps. >> >> [1] https://sourceware.org/pipermail/gdb-patches/2020-August/171232.html > > Thanks, this was the problem. But the core dump wasn't much help anyway, > the stuff I was interested in was pre-exception, and the backtrace > seemed to stop at the exception handling (unlike when 'live' debugging > when the stack trace continued). Hmm.. That's probably a bug of some sort, since I think the two methods should produce the same results... ^ permalink raw reply [flat|nested] 35+ messages in thread
end of thread, other threads:[~2021-12-30 15:44 UTC | newest] Thread overview: 35+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-12-23 23:10 [PATCH] fhandler_pipe: add sanity limit to handle loops Jeremy Drake 2021-12-24 0:06 ` Ken Brown 2021-12-24 0:29 ` Jeremy Drake 2021-12-24 17:17 ` Ken Brown 2021-12-24 19:42 ` Jeremy Drake 2021-12-24 22:46 ` Ken Brown 2021-12-24 23:42 ` Jeremy Drake 2021-12-25 0:39 ` Jeremy Drake 2021-12-25 3:19 ` Takashi Yano 2021-12-25 3:47 ` Jeremy Drake 2021-12-25 4:12 ` Takashi Yano 2021-12-25 5:40 ` Jeremy Drake 2021-12-25 17:10 ` Takashi Yano 2021-12-25 17:16 ` Takashi Yano 2021-12-25 19:00 ` Marco Atzeri 2021-12-25 19:20 ` Jeremy Drake 2021-12-25 22:18 ` Ken Brown 2021-12-25 23:00 ` Jeremy Drake 2021-12-26 3:04 ` Ken Brown 2021-12-26 4:56 ` Jeremy Drake 2021-12-26 15:09 ` Ken Brown 2021-12-26 16:04 ` Ken Brown 2021-12-26 16:24 ` Ken Brown 2021-12-26 21:35 ` Jeremy Drake 2021-12-26 22:18 ` Ken Brown 2021-12-26 22:43 ` Jeremy Drake 2021-12-26 23:12 ` Ken Brown 2021-12-26 23:23 ` Jeremy Drake 2021-12-27 2:42 ` Ken Brown 2021-12-27 21:12 ` Jeremy Drake 2021-12-29 21:59 ` Ken Brown 2021-12-29 23:29 ` Jeremy Drake 2021-12-27 20:01 ` Jon Turney 2021-12-29 5:45 ` Jeremy Drake 2021-12-30 15:44 ` Jon Turney
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).