[PATCH] Possibly correct fix to strace phantom process entry

[Daniel Santos]

On 04/24/2017 07:19 AM, Corinna Vinschen wrote:
> Hi Daniel,
>
> On Apr 24 04:37, Daniel Santos wrote:
>> The root cause of problem with strace causing long delays when any
>> process enumerates the process database appears to be calling
>> myself.thisproc () from child_info_spawn::handle_spawn() when we've
>> dynamically loaded cygwin1.dll.  It definately fixes the problem, but I
>> don't konw what other processes dynamically load cygwin1.dll and, thus,
>> what other side-effects that this may have.  Please verify correctness.
>>
>> Please see discussion here: https://cygwin.com/ml/cygwin/2017-04/msg00240.html
>>
>> Daniel
>>
>> Signed-off-by: Daniel Santos <daniel.santos@pobox.com>
>> ---
> I was just looking into this myself, but I was looking into the weird
> Sleep loop itself and was wondering if that makes any sense at all.
>
> Assuming pinfo::init is called from process enumeration (winpids::add)
> then there's no good reason to handle this procinfo block at all.  It
> doesn't resolve into an existing "real" Cygwin process.  And that's
> exactly the case that hangs.

Yeah, and it doesn't represent a unique windows process either.

> So my curent fix would have been this:
>
> diff --git a/winsup/cygwin/pinfo.cc b/winsup/cygwin/pinfo.cc
> index e43082d..090fcb9 100644
> --- a/winsup/cygwin/pinfo.cc
> +++ b/winsup/cygwin/pinfo.cc
> @@ -314,12 +314,18 @@ pinfo::init (pid_t n, DWORD flag, HANDLE h0)
>         /* Detect situation where a transitional memory block is being retrieved.
>   	 If the block has been allocated with PINFO_REDIR_SIZE but not yet
>   	 updated with a PID_EXECED state then we'll retry.  */
> -      if (!created && !(flag & PID_NEW))
> -	/* If not populated, wait 2 seconds for procinfo to become populated.
> -	   Would like to wait with finer granularity but that is not easily
> -	   doable.  */
> -	for (int i = 0; i < 200 && !procinfo->ppid; i++)
> -	  Sleep (10);
> +      if (!created && !(flag & PID_NEW) && !procinfo->ppid)
> +	{
> +	  /* We're fetching process info for /proc or ps so we can just
> +	     ignore this procinfo. */
> +	  if (flag & PID_NOREDIR)
> +	    break;
> +	  /* If not populated, wait 2 seconds for procinfo to become populated.
> +	     Would like to wait with finer granularity but that is not easily
> +	     doable.  */
> +	  for (int i = 0; i < 200 && !procinfo->ppid; i++)
> +	    Sleep (10);
> +	}

Yeah, I hacked this loop up many times, mostly to diagnose the problem.  
I presume that it was originally added for a reason, but as I said 
before, I know that on x86 procinfo->ppid is almost certain to compile 
into a mov that will be atomic, but when I expect another thread to 
change something larger than one byte, I prefer to use a macro or 
function that is always atomic and conveys the intention.

>>   winsup/cygwin/dcrt0.cc | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/winsup/cygwin/dcrt0.cc b/winsup/cygwin/dcrt0.cc
>> index ea6adcbbd..bbab08725 100644
>> --- a/winsup/cygwin/dcrt0.cc
>> +++ b/winsup/cygwin/dcrt0.cc
>> @@ -664,7 +664,8 @@ child_info_spawn::handle_spawn ()
>>     my_wr_proc_pipe = wr_proc_pipe;
>>     rd_proc_pipe = wr_proc_pipe = NULL;
>>   
>> -  myself.thisproc (h);
>> +  if (!dynamically_loaded)
>> +    myself.thisproc (h);
>>     __argc = moreinfo->argc;
>>     __argv = moreinfo->argv;
>>     envp = moreinfo->envp;
>> -- 
>> 2.11.0
> Your patch looks simple and elegant.  I'm just not sure about the side
> effects it may have if a process is missing its procinfo.  No problem
> for strace and ldd, apparently, but other processes...?

This makes me curious if ld produces this problem as well, only that 
it's very brief in execution.

> We could try it for a while.  I'm off all of May anyway and I could
> create a test build for that time...
>
> Btw., would you mind to send a BSD waiver per
> https://cygwin.com/contrib.html and
> https://cygwin.com/git/?p=newlib-cygwin.git;f=winsup/CONTRIBUTORS;hb=HEAD
> Your patches are covered by the "trivial patch" rule yet, but if you
> look into providing more patches you don't have to care anymore.

Thanks, I had overlooked that.  Is this sufficient?

I am providing my patches to the Cygwin sources under the 2-clause BSD 
license.

Daniel