Re: [PATCH] Possibly correct fix to strace phantom process entry

[Corinna Vinschen <corinna-cygwin at cygwin dot com>]

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.

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);
+	}
 
       if (!created && createit && (procinfo->process_state & PID_REAPED))
 	{

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

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,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

Attachment: signature.asc
Description: PGP signature