Subject: RE: Bash returns incorrect process status
From: "Pierre A. Humblet" <[email-masked]>
To: <[email-masked]>, <[email-masked]>
Cc: <[email-masked]>
Date: Sun, 24 Oct 2004 22:06:40 -0400

Attached is a "simpler & better" patch to bash, replacing
the one in
<http://cygwin.com/ml/cygwin/2004-09/msg00882.html>

It's simpler because it slightly changes how bash works to
prevent the pid reuse problem, instead of adding a layer to
fix it. It's better because it also fixes the bug reported
in <http://cygwin.com/ml/cygwin/2004-09/msg01503.html>
and some other potential issues.

To build bash, follow the following steps:

Download the bash source package, using setup, and put
the attached pids3.diff file under /usr/src/bash-2.05b
$ cd /usr/src
$ ./bash-2.05b-16.sh prep
$ cd bash-2.05b
$ patch < pids3.diff
$ cd ..
$ ./bash-2.05b-16.sh conf
$ ./bash-2.05b-16.sh build
This produces /usr/src/bash-2.05b/.build/bash.exe,
which you can copy to /bin, preferably after having saved
the original bash.

Pierre

Note: This patch does not modify configure. If you have
applied the previous patch, make sure to run prep again.
--- execute_cmd.c.orig	2002-03-18 13:24:22.000000000 -0500
+++ execute_cmd.c	2004-10-23 21:45:10.000000000 -0400
@@ -619,6 +619,9 @@ execute_command_internal (command, async
 	/* We can't rely on this variable retaining its value across a
 	   call to execute_simple_command if a longjmp occurs as the
 	   result of a `return' builtin.  This is true for sure with gcc. */
+#if defined (RECYCLES_PIDS)
+        last_made_pid = NO_PID;
+#endif
 	last_pid = last_made_pid;
 	was_error_trap = signal_is_trapped (ERROR_TRAP) && signal_is_ignored (ERROR_TRAP) == 0;

@@ -664,13 +667,6 @@ execute_command_internal (command, async
 	       commands, this causes the last simple command in
 	       the function to be waited for twice. */
 	      exec_result = wait_for (last_made_pid);
-#if defined (RECYCLES_PIDS)
-	      /* LynxOS, for one, recycles pids very quickly -- so quickly
-		 that a new process may have the same pid as the last one
-		 created.  This has been reported to fix the problem. */
-	      if (exec_result == 0)
-		last_made_pid = NO_PID;
-#endif
 	  }
       }

@@ -2485,6 +2481,9 @@ execute_simple_command (simple_command,
   first_word_quoted =
     simple_command->words ? (simple_command->words->word->flags & W_QUOTED): 0;

+#if defined (RECYCLES_PIDS)
+  last_command_subst_pid = NO_PID;
+#endif
   old_last_command_subst_pid = last_command_subst_pid;
   old_last_async_pid = last_asynchronous_pid;

@@ -2518,9 +2517,11 @@ execute_simple_command (simple_command,
 	  already_forked = 1;
 	  simple_command->flags |= CMD_NO_FORK;

-	  subshell_environment = (pipe_in != NO_PIPE || pipe_out != NO_PIPE)
-					? (SUBSHELL_PIPE|SUBSHELL_FORK)
-	  				: (SUBSHELL_ASYNC|SUBSHELL_FORK);
+	  subshell_environment = SUBSHELL_FORK;
+	  if (async)
+            subshell_environment |= SUBSHELL_ASYNC;
+	  if (pipe_in != NO_PIPE || pipe_out != NO_PIPE)
+            subshell_environment |= SUBSHELL_PIPE;

 	  /* We need to do this before piping to handle some really
 	     pathological cases where one of the pipe file descriptors
--- jobs.c.orig	2004-09-22 22:52:44.000000000 -0400
+++ jobs.c	2004-10-23 22:05:02.000000000 -0400
@@ -232,7 +232,7 @@ static int map_over_jobs __P((sh_job_map
 static int job_last_stopped __P((int));
 static int job_last_running __P((int));
 static int most_recent_job_in_state __P((int, JOB_STATE));
-static int find_job __P((pid_t, int));
+static int find_job __P((pid_t, int, PROCESS  **));
 static int print_job __P((JOB *, int, int, int));
 static int process_exit_status __P((WAIT));
 static int job_exit_status __P((int));
@@ -504,7 +504,7 @@ stop_pipeline (async, deferred)
   if (the_pipeline)
     {
       register PROCESS *p;
-      int any_alive, any_stopped;
+      int any_running, any_stopped;

       newjob = (JOB *)xmalloc (sizeof (JOB));

@@ -528,16 +528,16 @@ stop_pipeline (async, deferred)

       /* Set the state of this pipeline. */
       p = newjob->pipe;
-      any_alive = any_stopped = 0;
+      any_running = any_stopped = 0;
       do
 	{
-	  any_alive |= p->running;
-	  any_stopped |= WIFSTOPPED (p->status);
+	  any_running |= PRUNNING (p);
+	  any_stopped |= PSTOPPED (p);
 	  p = p->next;
 	}
       while (p != newjob->pipe);

-      newjob->state = any_alive ? JRUNNING : (any_stopped ? JSTOPPED : JDEAD);
+      newjob->state = any_running ? JRUNNING : (any_stopped ? JSTOPPED : JDEAD);
       newjob->wd = job_working_directory ();
       newjob->deferred = deferred;

@@ -739,6 +739,28 @@ add_process (name, pid)
 {
   PROCESS *t, *p;

+#if defined (RECYCLES_PIDS)
+  int job;
+  p = find_pipeline (pid, 0, &job);
+  if (p)
+    {
+	/* As I understand it, job should never be "NO_JOB" because processes
+	   in the current pipeline have not been reaped.
+	   If that's true (the printf below never kicks in), replace the above by
+	   job = find_job (pid, 0, &p)
+	   if (job != NO_JOB)
+	   and remove the test and printf below.
+	*/
+      if (job == NO_JOB)
+	fprintf(stderr, "add_process: process pid %5ld (%s) in the_pipeline",
+		(long) p->pid, p->command);
+      if (PALIVE (p))
+	internal_error ("add_process: pid %5ld (%s) is still alive",
+			(long) p->pid, p->command);
+      p->running = PS_RECYCLED;
+    }
+#endif
+
   t = (PROCESS *)xmalloc (sizeof (PROCESS));
   t->next = the_pipeline;
   t->pid = pid;
@@ -885,13 +907,13 @@ kill_current_pipeline ()
 /* Return the pipeline that PID belongs to.  Note that the pipeline
    doesn't have to belong to a job.  Must be called with SIGCHLD blocked. */
 static PROCESS *
-find_pipeline (pid, running_only, jobp)
+find_pipeline (pid, alive_only, jobp)
      pid_t pid;
-     int running_only;
+     int alive_only;
      int *jobp;		/* index into jobs list or NO_JOB */
 {
   int job;
-  register PROCESS *p;
+  PROCESS *p;

   /* See if this process is in the pipeline that we are building. */
   if (jobp)
@@ -902,32 +924,31 @@ find_pipeline (pid, running_only, jobp)
       do
 	{
 	  /* Return it if we found it. */
-	  if (p->pid == pid)
-	    {
-	      if ((running_only && PRUNNING(p)) || (running_only == 0))
-		return (p);
-	    }
+	  if (p->pid == pid
+              && (PALIVE (p) || ((alive_only == 0) && !PRECYCLED (p))))
+	    return (p);

 	  p = p->next;
 	}
       while (p != the_pipeline);
     }

-  job = find_job (pid, running_only);
+  job = find_job (pid, alive_only, &p);
   if (jobp)
     *jobp = job;
-  return (job == NO_JOB) ? (PROCESS *)NULL : jobs[job]->pipe;
+  return (job == NO_JOB) ? (PROCESS *)NULL : p;
 }

 /* Return the job index that PID belongs to, or NO_JOB if it doesn't
    belong to any job.  Must be called with SIGCHLD blocked. */
 static int
-find_job (pid, running_only)
+find_job (pid, alive_only, pp)
      pid_t pid;
-     int running_only;
+     int alive_only;
+     PROCESS ** pp;
 {
   register int i;
-  register PROCESS *p;
+  PROCESS *p;

   for (i = 0; i < job_slots; i++)
     {
@@ -937,10 +958,12 @@ find_job (pid, running_only)

 	  do
 	    {
-	      if (p->pid == pid)
+	      if (p->pid == pid
+		  && (PALIVE (p) || ((alive_only == 0) && !PRECYCLED (p))))
 		{
-		  if ((running_only && PRUNNING(p)) || (running_only == 0))
-		    return (i);
+		  if (pp)
+		    *pp = p;
+		  return (i);
 		}

 	      p = p->next;
@@ -965,7 +988,7 @@ get_job_by_pid (pid, block)
   if (block)
     BLOCK_CHILD (set, oset);

-  job = find_job (pid, 0);
+  job = find_job (pid, 0, NULL);

   if (block)
     UNBLOCK_CHILD (oset);
@@ -983,7 +1006,7 @@ describe_pid (pid)

   BLOCK_CHILD (set, oset);

-  job = find_job (pid, 0);
+  job = find_job (pid, 0, NULL);

   if (job != NO_JOB)
     printf ("[%d] %ld\n", job + 1, (long)pid);
@@ -1099,7 +1122,7 @@ print_pipeline (p, job_index, format, st
 	    {
 	      if (format)
 		{
-		  if (show->running == first->running &&
+		  if (PRUNNING (show) == PRUNNING (first) &&
 		      WSTATUS (show->status) == WSTATUS (first->status))
 		    temp = "";
 		}
@@ -1374,6 +1397,13 @@ make_child (command, async_p)

       if (async_p)
 	last_asynchronous_pid = getpid ();
+#if defined (RECYCLES_PIDS)
+      /* Avoid aliasing by setting the pid to an unusual value.
+         Setting to NO_PID might cause an error when evaluating $!
+         Using 1 seems safe as that process cannot be killed accidentally */
+      else if (last_asynchronous_pid == getpid ())
+	last_asynchronous_pid = 1;
+#endif
     }
   else
     {
@@ -1407,7 +1437,11 @@ make_child (command, async_p)

       if (async_p)
 	last_asynchronous_pid = pid;
-
+#if defined (RECYCLES_PIDS)
+      /* See comment in child code */
+      else if (last_asynchronous_pid == pid)
+	last_asynchronous_pid = 1;
+#endif
       last_made_pid = pid;

       /* Unblock SIGINT and SIGCHLD. */
@@ -1616,7 +1650,7 @@ wait_for_single_pid (pid)
   /* POSIX.2: if we just waited for a job, we can remove it from the jobs
      table. */
   BLOCK_CHILD (set, oset);
-  job = find_job (pid, 0);
+  job = find_job (pid, 0, NULL);
   if (job != NO_JOB && jobs[job] && DEADJOB (job))
     jobs[job]->flags |= J_NOTIFIED;
   UNBLOCK_CHILD (oset);
@@ -1821,13 +1855,13 @@ wait_for (pid)
 	 We check for JDEAD in case the job state has been set by waitchld
 	 after receipt of a SIGCHLD. */
       if (job == NO_JOB)
-	job = find_job (pid, 0);
+	job = find_job (pid, 0, NULL);

       /* waitchld() takes care of setting the state of the job.  If the job
 	 has already exited before this is called, sigchld_handler will have
 	 called waitchld and the state will be set to JDEAD. */

-      if (child->running || (job != NO_JOB && RUNNING (job)))
+      if (PRUNNING (child) || (job != NO_JOB && RUNNING (job)))
 	{
 #if defined (WAITPID_BROKEN)    /* SCOv4 */
 	  sigset_t suspend_set;
@@ -1881,7 +1915,7 @@ wait_for (pid)
       if (interactive && job_control == 0)
 	QUIT;
     }
-  while (child->running || (job != NO_JOB && RUNNING (job)));
+  while (PRUNNING (child) || (job != NO_JOB && RUNNING (job)));

   /* The exit state of the command is either the termination state of the
      child, or the termination state of the job.  If a job, the status
@@ -2356,6 +2390,10 @@ kill_pid (pid, sig, group)

 	      do
 		{
+#if defined (RECYCLES_PIDS)
+		  if (!PALIVE (p)) /* The pid may have been recycled */
+		    continue;
+#endif
 		  kill (p->pid, sig);
 		  if (p->running == PS_DONE && (sig == SIGTERM || sig == SIGHUP))
 		    kill (p->pid, SIGCONT);
@@ -2473,9 +2511,6 @@ waitchld (wpid, block)
       if (child == 0)
 	continue;

-      while (child->pid != pid)
-	child = child->next;
-
       /* Remember status, and whether or not the process is running. */
       child->status = status;
       child->running = WIFCONTINUED(status) ? PS_RUNNING : PS_DONE;
@@ -2544,8 +2579,8 @@ set_job_status_and_cleanup (job)
   job_state = any_stopped = any_tstped = 0;
   do
     {
-      job_state |= child->running;
-      if (child->running == PS_DONE && (WIFSTOPPED (child->status)))
+      job_state |= PRUNNING (child);
+      if (PSTOPPED (child))
 	{
 	  any_stopped = 1;
 	  any_tstped |= interactive && job_control &&
--- jobs.h.orig	2002-01-17 12:35:12.000000000 -0500
+++ jobs.h	2004-10-23 21:40:00.000000000 -0400
@@ -45,7 +45,7 @@
 /* Values for the `running' field of a struct process. */
 #define PS_DONE		0
 #define PS_RUNNING	1
-#define PS_STOPPED	2
+#define PS_RECYCLED	2

 /* Each child of the shell is remembered in a STRUCT PROCESS.  A chain of
    such structures is a pipeline.  The chain is circular. */
@@ -57,10 +57,14 @@ typedef struct process {
   char *command;	/* The particular program that is running. */
 } PROCESS;

-/* PRUNNING really means `not exited' */
-#define PRUNNING(p)	((p)->running || WIFSTOPPED((p)->status))
+#define PRUNNING(p)	((p)->running == PS_RUNNING)
+#define PALIVE(p)	((p)->running == PS_RUNNING || WIFSTOPPED((p)->status))
 #define PSTOPPED(p)	(WIFSTOPPED((p)->status))
-#define PDEADPROC(p)	((p)->running == PS_DONE)
+#if defined (RECYCLES_PIDS)
+#  define PRECYCLED(p)	((p)->running == PS_RECYCLED)
+#else
+#  define PRECYCLED(p)	0
+#endif

 /* A description of a pipeline's state. */
 typedef enum { JRUNNING, JSTOPPED, JDEAD, JMIXED } JOB_STATE;
--- subst.c.orig	2004-09-17 21:03:52.000000000 -0400
+++ subst.c	2004-10-23 21:48:06.000000000 -0400
@@ -3773,7 +3773,7 @@ command_substitute (string, quoted)
   cleanup_the_pipeline ();
 #endif

-  pid = make_child ((char *)NULL, 0);
+  pid = make_child ((char *)NULL, subshell_environment & SUBSHELL_ASYNC);
   if (pid == 0)
     /* Reset the signal handlers in the child, but don't free the
        trap strings. */

--
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple
Problem reports:       http://cygwin.com/problems.html
Documentation:         http://cygwin.com/docs.html
FAQ:                   http://cygwin.com/faq/