bug#12829: 24.3.50; emacs_abort () called from w32proc.c:1128

[Eli Zaretskii <eliz@gnu.org>]

> Date: Sat, 10 Nov 2012 10:27:10 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 12829@debbugs.gnu.org
> 
> Thanks, that figures.  Basically, the modified code that handles
> demise of child processes is incompatible with the emulated 'wait'
> function, because it does not support waiting for a process by its
> PID.  I think we will have to rewrite 'sys_wait' to emulate 'waitpid',
> although I'll try first to come up with a simpler band-aid.

I'd still like to have the details as requested here (with the current
trunk code):

> Just so my understanding of the exact scenario is better, could you
> please add "bt 10" to the commands of breakpoint 4, the one set in
> process_status_retrieved, and again post the full transcript of the
> GDB session leading to the crash?

However in the meantime, I came up with the changes below, which seem
to work in my limited testing, and should fix the fundamental problem
that caused your crashes.  Could you please try these changes for a
while, and see if they give good results?  (You will have to re-run
configure.bat before compiling.)  If they do, I will then commit this
to the trunk.  TIA.

=== modified file 'nt/config.nt'
--- nt/config.nt	2012-11-05 14:30:32 +0000
+++ nt/config.nt	2012-11-10 12:02:26 +0000
@@ -959,7 +959,7 @@ along with GNU Emacs.  If not, see <http
 #undef HAVE_SYS_VLIMIT_H
 
 /* Define to 1 if you have <sys/wait.h> that is POSIX.1 compatible. */
-#undef HAVE_SYS_WAIT_H
+#define HAVE_SYS_WAIT_H 1
 
 /* Define to 1 if you have the <term.h> header file. */
 #undef HAVE_TERM_H

=== modified file 'nt/inc/ms-w32.h'
--- nt/inc/ms-w32.h	2012-10-19 19:25:18 +0000
+++ nt/inc/ms-w32.h	2012-11-10 12:06:16 +0000
@@ -185,15 +185,12 @@ extern char *getenv ();
 
 /* Subprocess calls that are emulated.  */
 #define spawnve sys_spawnve
-#define wait    sys_wait
 #define kill    sys_kill
 #define signal  sys_signal
 
 /* Internal signals.  */
 #define emacs_raise(sig) emacs_abort()
 
-extern int sys_wait (int *);
-
 /* termcap.c calls that are emulated.  */
 #define tputs   sys_tputs
 #define tgetstr sys_tgetstr

=== added file 'nt/inc/sys/wait.h'
--- nt/inc/sys/wait.h	1970-01-01 00:00:00 +0000
+++ nt/inc/sys/wait.h	2012-11-10 12:05:45 +0000
@@ -0,0 +1,33 @@
+/* A limited emulation of sys/wait.h on Posix systems.
+
+Copyright (C) 2012  Free Software Foundation, Inc.
+
+This file is part of GNU Emacs.
+
+GNU Emacs is free software: you can redistribute it and/or modify
+it under the terms of the GNU General Public License as published by
+the Free Software Foundation, either version 3 of the License, or
+(at your option) any later version.
+
+GNU Emacs is distributed in the hope that it will be useful,
+but WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+GNU General Public License for more details.
+
+You should have received a copy of the GNU General Public License
+along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#ifndef INC_SYS_WAIT_H_
+#define INC_SYS_WAIT_H_
+
+#define WNOHANG    1
+#define WUNTRACED  2
+#define WSTOPPED   2	/* same as WUNTRACED */
+#define WEXITED    4
+#define WCONTINUED 8
+
+/* The various WIF* macros are defined in src/syswait.h.  */
+
+extern pid_t waitpid (pid_t, int *, int);
+
+#endif	/* INC_SYS_WAIT_H_ */

=== modified file 'src/sysdep.c'
--- src/sysdep.c	2012-11-05 03:18:32 +0000
+++ src/sysdep.c	2012-11-10 13:58:31 +0000
@@ -290,7 +290,7 @@ wait_for_termination_1 (pid_t pid, int i
   while (1)
     {
 #ifdef WINDOWSNT
-      wait (0);
+      waitpid (pid, NULL, 0);
       break;
 #else /* not WINDOWSNT */
       int status;

=== modified file 'src/w32proc.c'
--- src/w32proc.c	2012-11-05 03:18:32 +0000
+++ src/w32proc.c	2012-11-10 14:04:54 +0000
@@ -775,7 +775,6 @@ alarm (int seconds)
 /* Child process management list.  */
 int child_proc_count = 0;
 child_process child_procs[ MAX_CHILDREN ];
-child_process *dead_child = NULL;
 
 static DWORD WINAPI reader_thread (void *arg);
 
@@ -1028,9 +1027,6 @@ create_child (char *exe, char *cmdline, 
   if (cp->pid < 0)
     cp->pid = -cp->pid;
 
-  /* pid must fit in a Lisp_Int */
-  cp->pid = cp->pid & INTMASK;
-
   *pPid = cp->pid;
 
   return TRUE;
@@ -1106,55 +1102,110 @@ reap_subprocess (child_process *cp)
     delete_child (cp);
 }
 
-/* Wait for any of our existing child processes to die
-   When it does, close its handle
-   Return the pid and fill in the status if non-NULL.  */
+/* Wait for a child process specified by PID, or for any of our
+   existing child processes (if PID is nonpositive) to die.  When it
+   does, close its handle.  Return the pid of the process that died
+   and fill in STATUS if non-NULL.  */
 
-int
-sys_wait (int *status)
+pid_t
+waitpid (pid_t pid, int *status, int options)
 {
   DWORD active, retval;
   int nh;
-  int pid;
   child_process *cp, *cps[MAX_CHILDREN];
   HANDLE wait_hnd[MAX_CHILDREN];
+  DWORD timeout_ms;
+  int dont_wait = (options & WNOHANG) != 0;
 
   nh = 0;
-  if (dead_child != NULL)
+  /* According to Posix:
+
+     PID = -1 means status is requested for any child process.
+
+     PID > 0 means status is requested for a single child process
+     whose pid is PID.
+
+     PID = 0 means status is requested for any child process whose
+     process group ID is equal to that of the calling process.  But
+     since Windows has only a limited support for process groups (only
+     for console processes and only for the purposes of passing
+     Ctrl-BREAK signal to them), and since we have no documented way
+     of determining whether a given process belongs to our group, we
+     treat 0 as -1.
+
+     PID < -1 means status is requested for any child process whose
+     process group ID is equal to the absolute value of PID.  Again,
+     since we don't support process groups, we treat that as -1.  */
+  if (pid > 0)
     {
-      /* We want to wait for a specific child */
-      wait_hnd[nh] = dead_child->procinfo.hProcess;
-      cps[nh] = dead_child;
-      if (!wait_hnd[nh]) emacs_abort ();
-      nh++;
-      active = 0;
-      goto get_result;
+      int our_child = 0;
+
+      /* We are requested to wait for a specific child.  */
+      for (cp = child_procs + (child_proc_count-1); cp >= child_procs; cp--)
+	{
+	  /* Some child_procs might be sockets; ignore them.  Also
+	     ignore subprocesses whose output is not yet completely
+	     read.  */
+	  if (CHILD_ACTIVE (cp)
+	      && cp->procinfo.hProcess
+	      && cp->pid == pid)
+	    {
+	      our_child = 1;
+	      break;
+	    }
+	}
+      if (our_child)
+	{
+	  if (cp->fd < 0 || (fd_info[cp->fd].flags & FILE_AT_EOF) != 0)
+	    {
+	      wait_hnd[nh] = cp->procinfo.hProcess;
+	      cps[nh] = cp;
+	      nh++;
+	    }
+	  else if (dont_wait)
+	    {
+	      /* PID specifies our subprocess, but its status is not
+		 yet available.  */
+	      return 0;
+	    }
+	}
+      if (nh == 0)
+	{
+	  /* No such child process, or nothing to wait for, so fail.  */
+	  errno = ECHILD;
+	  return -1;
+	}
     }
   else
     {
       for (cp = child_procs + (child_proc_count-1); cp >= child_procs; cp--)
-	/* some child_procs might be sockets; ignore them */
-	if (CHILD_ACTIVE (cp) && cp->procinfo.hProcess
-	    && (cp->fd < 0 || (fd_info[cp->fd].flags & FILE_AT_EOF) != 0))
-	  {
-	    wait_hnd[nh] = cp->procinfo.hProcess;
-	    cps[nh] = cp;
-	    nh++;
-	  }
+	{
+	  if (CHILD_ACTIVE (cp)
+	      && cp->procinfo.hProcess
+	      && (cp->fd < 0 || (fd_info[cp->fd].flags & FILE_AT_EOF) != 0))
+	    {
+	      wait_hnd[nh] = cp->procinfo.hProcess;
+	      cps[nh] = cp;
+	      nh++;
+	    }
+	}
+      if (nh == 0)
+	{
+	  /* Nothing to wait on, so fail.  */
+	  errno = ECHILD;
+	  return -1;
+	}
     }
 
-  if (nh == 0)
-    {
-      /* Nothing to wait on, so fail */
-      errno = ECHILD;
-      return -1;
-    }
+  if (dont_wait)
+    timeout_ms = 0;
+  else
+    timeout_ms = 1000;	/* check for quit about once a second. */
 
   do
     {
-      /* Check for quit about once a second. */
       QUIT;
-      active = WaitForMultipleObjects (nh, wait_hnd, FALSE, 1000);
+      active = WaitForMultipleObjects (nh, wait_hnd, FALSE, timeout_ms);
     } while (active == WAIT_TIMEOUT);
 
   if (active == WAIT_FAILED)
@@ -1184,8 +1235,10 @@ get_result:
     }
   if (retval == STILL_ACTIVE)
     {
-      /* Should never happen */
+      /* Should never happen.  */
       DebPrint (("Wait.WaitForMultipleObjects returned an active process\n"));
+      if (pid > 0 && dont_wait)
+	return 0;
       errno = EINVAL;
       return -1;
     }
@@ -1199,6 +1252,8 @@ get_result:
   else
     retval <<= 8;
 
+  if (pid > 0 && active != 0)
+    emacs_abort ();
   cp = cps[active];
   pid = cp->pid;
 #ifdef FULL_DEBUG
@@ -1987,9 +2042,7 @@ count_children:
 	      DebPrint (("select calling SIGCHLD handler for pid %d\n",
 			 cp->pid));
 #endif
-	      dead_child = cp;
 	      sig_handlers[SIGCHLD] (SIGCHLD);
-	      dead_child = NULL;
 	    }
 	}
       else if (fdindex[active] == -1)