[PATCH] fhandler_pipe: add sanity limit to handle loops

[PATCH] fhandler_pipe: add sanity limit to handle loops

[Jeremy Drake]

[-- 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

Re: [PATCH] fhandler_pipe: add sanity limit to handle loops

[Ken Brown]

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

Re: [PATCH] fhandler_pipe: add sanity limit to handle loops

[Jeremy Drake]

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?).

Re: [PATCH] fhandler_pipe: add sanity limit to handle loops

[Ken Brown]

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

Re: [PATCH] fhandler_pipe: add sanity limit to handle loops

[Jeremy Drake]

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

[Ken Brown]

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

[Jeremy Drake]

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

[Jeremy Drake]

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

[Takashi Yano]

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>

Re: [PATCH] fhandler_pipe: add sanity limit to handle loops

[Jeremy Drake]

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

Re: [PATCH] fhandler_pipe: add sanity limit to handle loops

[Takashi Yano]

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>

Re: [PATCH] fhandler_pipe: add sanity limit to handle loops

[Jeremy Drake]

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.

Re: [PATCH] fhandler_pipe: add sanity limit to handle loops

[Takashi Yano]

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>

Re: [PATCH] fhandler_pipe: add sanity limit to handle loops

[Takashi Yano]

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>

Re: [PATCH] fhandler_pipe: add sanity limit to handle loops

[Marco Atzeri]

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

[Jeremy Drake]

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

[Ken Brown]

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

[Jeremy Drake]

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

[Ken Brown]

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

[Jeremy Drake]

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.

Re: [PATCH] fhandler_pipe: add sanity limit to handle loops

[Ken Brown]

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

Re: [PATCH] fhandler_pipe: add sanity limit to handle loops

[Ken Brown]

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

Re: [PATCH] fhandler_pipe: add sanity limit to handle loops

[Ken Brown]

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

Re: [PATCH] fhandler_pipe: add sanity limit to handle loops

[Jeremy Drake]

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.

Re: [PATCH] fhandler_pipe: add sanity limit to handle loops

[Ken Brown]

[-- 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

Re: [PATCH] fhandler_pipe: add sanity limit to handle loops

[Jeremy Drake]

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.

Re: [PATCH] fhandler_pipe: add sanity limit to handle loops

[Ken Brown]

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

Re: [PATCH] fhandler_pipe: add sanity limit to handle loops

[Jeremy Drake]

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

[Ken Brown]

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

[Jon Turney]

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

[Jeremy Drake]

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

[Jeremy Drake]

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

[Ken Brown]

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

[Jeremy Drake]

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

[Jon Turney]

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...