Re: [PATCH] fhandler_pipe: add sanity limit to handle loops
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...
Re: [PATCH] fhandler_pipe: add sanity limit to handle loops
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.
Re: [PATCH] fhandler_pipe: add sanity limit to handle loops
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
Re: [PATCH] fhandler_pipe: add sanity limit to handle loops
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).
Re: [PATCH] fhandler_pipe: add sanity limit to handle loops
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.
Re: [PATCH] fhandler_pipe: add sanity limit to handle loops
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
Re: [PATCH] fhandler_pipe: add sanity limit to handle loops
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
Re: [PATCH] fhandler_pipe: add sanity limit to handle loops
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).
Re: [PATCH] fhandler_pipe: add sanity limit to handle loops
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, ); 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
Re: [PATCH] fhandler_pipe: add sanity limit to handle loops
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, ); > 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.
Re: [PATCH] fhandler_pipe: add sanity limit to handle loops
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, );
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.)
KenFrom 4858e73321a0618a8b1e1060416ef7d546cda895 Mon Sep 17 00:00:00 2001
From: Ken Brown
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, );
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
Re: [PATCH] fhandler_pipe: add sanity limit to handle loops
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, ); > > 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.
Re: [PATCH] fhandler_pipe: add sanity limit to handle loops
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, );
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
Re: [PATCH] fhandler_pipe: add sanity limit to handle loops
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, );
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
Re: [PATCH] fhandler_pipe: add sanity limit to handle loops
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 = 0x, 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
Re: [PATCH] fhandler_pipe: add sanity limit to handle loops
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 = 0x, 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.
Re: [PATCH] fhandler_pipe: add sanity limit to handle loops
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
Re: [PATCH] fhandler_pipe: add sanity limit to handle loops
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.
Re: [PATCH] fhandler_pipe: add sanity limit to handle loops
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
Re: [PATCH] fhandler_pipe: add sanity limit to handle loops
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.
Re: [PATCH] fhandler_pipe: add sanity limit to handle loops
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
Re: [PATCH] fhandler_pipe: add sanity limit to handle loops
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
Re: [PATCH] fhandler_pipe: add sanity limit to handle loops
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 = 0x, 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
#include
#include
#include
#include
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, );
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,
Re: [PATCH] fhandler_pipe: add sanity limit to handle loops
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 = 0x, 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.
Re: [PATCH] fhandler_pipe: add sanity limit to handle loops
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
Re: [PATCH] fhandler_pipe: add sanity limit to handle loops
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
#include
+#include
#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
Re: [PATCH] fhandler_pipe: add sanity limit to handle loops
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
Re: [PATCH] fhandler_pipe: add sanity limit to handle loops
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
Re: [PATCH] fhandler_pipe: add sanity limit to handle loops
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.
Re: [PATCH] fhandler_pipe: add sanity limit to handle loops
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
Re: [PATCH] fhandler_pipe: add sanity limit to handle loops
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
Re: [PATCH] fhandler_pipe: add sanity limit to handle loops
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 0x000180062f13 in exception::handle (e=0x14cc4f0, frame=, in=, dispatch=) at /c/S/msys2-runtime/src/msys2-runtime/winsup/cygwin/exceptions.cc:835 #4 0x7ff8abd320cf in ntdll!.chkstk () from /c/Windows/SYSTEM32/ntdll.dll #5 0x7ff8abce1454 in ntdll!RtlRaiseException () from /c/Windows/SYSTEM32/ntdll.dll #6 0x7ff8abd30bfe in ntdll!KiUserExceptionDispatcher () from /c/Windows/SYSTEM32/ntdll.dll #7 0x000180092687 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 0x000180092bdb 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
Re: [PATCH] fhandler_pipe: add sanity limit to handle loops
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 0x000180062f13 in exception::handle (e=0x14cc4f0, frame=, in=, dispatch=) at /c/S/msys2-runtime/src/msys2-runtime/winsup/cygwin/exceptions.cc:835 #4 0x7ff8abd320cf in ntdll!.chkstk () from /c/Windows/SYSTEM32/ntdll.dll #5 0x7ff8abce1454 in ntdll!RtlRaiseException () from /c/Windows/SYSTEM32/ntdll.dll #6 0x7ff8abd30bfe in ntdll!KiUserExceptionDispatcher () from /c/Windows/SYSTEM32/ntdll.dll #7 0x000180092687 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 0x000180092bdb 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?).
Re: [PATCH] fhandler_pipe: add sanity limit to handle loops
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
