[PATCH] fhandler_pipe: add sanity limit to handle loops
Ken Brown
kbrown@cornell.edu
Sun Dec 26 23:12:29 GMT 2021
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
More information about the Cygwin-patches
mailing list