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

[Ken Brown <kbrown@cornell.edu>]

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