[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