Subject: RE: Bash returns incorrect process status
From: "Pierre A. Humblet" <[email-masked]>
To: <[email-masked]>, <[email-masked]>
Cc: <[email-masked]>
Date: Fri, 17 Sep 2004 22:58:11 -0400

At 09:51 AM 9/17/2004 +0200, Peter Ekberg wrote:
>Pierre A. Humblet wrote:
>> FWIW, attached is a patch to bash that may improve its 
>> behavior on Cygwin.
>> The idea is that when a new process is stored in the memory array, any
>> existing process with the same pid is marked "reused". 
>> "reused" processes
>> are never considered when searching for a process by pid. 
>> They are still
>> still available, e.g. to get the status of processes in a job. 
>> 
>> It's a proof of principle code, not meant to be efficient. It 
>> can still print
>> a debug message on stderr.
>
>Tried it and it doesn't solve the problem for me. It shifts the trigger
>pattern though.

There was another problem. bash keeps track of the
"last_made_pid" and compares it to its previous value
to decide if it should wait on a process.

So if a new command has the same pid as the previous one,
bash won't wait on it.

You might think that this is OK as long as consecutive
forks return different pids. Wrong, because forks used
for some purposes, such as back tick evaluations,
don't count.

So bash is currently unreliable on any system where
processes don't have unique pids. If pid values are reused,
and all intervening forks serve only for backtick evaluations,
bash will return 0 status for a command.
Naturally this gets to be unlikely as the reuse period grows.

Here is another patch against the Cygwin release of bash.
It includes the previous one in this thread. Please try it.

Thanks to Bogdan Vacaliuc and Peter Ekberg for their help.

Pierre

--- configure.orig	2004-09-17 21:04:34.000000000 -0400
+++ configure	2004-09-17 21:25:24.000000000 -0400
@@ -19323,7 +19323,7 @@ lynxos*)	LOCAL_CFLAGS=-DRECYCLES_PIDS ;;
 linux*)		LOCAL_LDFLAGS=-rdynamic ;;	 # allow dynamic loading
 *qnx*)		LOCAL_CFLAGS="-Dqnx -F -3s" LOCAL_LDFLAGS="-3s" LOCAL_LIBS="-lunix -lncurses" ;;
 powerux*)	LOCAL_LIBS="-lgen" ;;
-cygwin*)	LOCAL_CFLAGS="-DRECYCLES_PIDS" ;;
+cygwin*)	LOCAL_CFLAGS="" ;;
 opennt*|interix*) LOCAL_CFLAGS="-DNO_MAIN_ENV_ARG -DBROKEN_DIRENT_D_INO" ;;
 esac

--- execute_cmd.c.orig	2002-03-18 13:24:22.000000000 -0500
+++ execute_cmd.c	2004-09-17 22:03:44.000000000 -0400
@@ -468,7 +468,7 @@ execute_command_internal (command, async
 {
   int exec_result, invert, ignore_return, was_error_trap;
   REDIRECT *my_undo_list, *exec_undo_list;
-  volatile pid_t last_pid;
+  volatile upid_t last_pid;

   if (command == 0 || breaking || continuing || read_but_dont_execute)
     return (EXECUTION_SUCCESS);
@@ -646,13 +646,13 @@ execute_command_internal (command, async
 	/* XXX - this is something to watch out for if there are problems
 	   when the shell is compiled without job control. */
 	if (already_making_children && pipe_out == NO_PIPE &&
-	    last_pid != last_made_pid)
+	    (last_pid.pid != last_made_pid.pid || last_pid.seq != last_made_pid.seq))
 	  {
 	    stop_pipeline (asynchronous, (COMMAND *)NULL);

 	    if (asynchronous)
 	      {
-		DESCRIBE_PID (last_made_pid);
+		DESCRIBE_PID (last_made_pid.pid);
 	      }
 	    else
 #if !defined (JOB_CONTROL)
@@ -663,7 +663,7 @@ execute_command_internal (command, async
 	    /* When executing a shell function that executes other
 	       commands, this causes the last simple command in
 	       the function to be waited for twice. */
-	      exec_result = wait_for (last_made_pid);
+	      exec_result = wait_for (last_made_pid.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
--- jobs.c.orig	2002-05-09 11:56:20.000000000 -0400
+++ jobs.c	2004-09-17 21:45:16.000000000 -0400
@@ -187,11 +187,14 @@ int current_job = NO_JOB;
 int previous_job = NO_JOB;

 /* Last child made by the shell.  */
-pid_t last_made_pid = NO_PID;
+upid_t last_made_pid = NO_UPID;

 /* Pid of the last asynchronous child. */
 pid_t last_asynchronous_pid = NO_PID;

+/* Sequence number for unique pid identification */
+unsigned int pid_sequence = 0;
+
 /* The pipeline currently being built. */
 PROCESS *the_pipeline = (PROCESS *)NULL;

@@ -737,7 +740,35 @@ add_process (name, pid)
      char *name;
      pid_t pid;
 {
-  PROCESS *t, *p;
+  PROCESS *t, *p, * p_start;
+  register int i;
+
+  /* Mark any pid that is being reused */
+  for (i = -1; i < job_slots; i++)
+    {
+      if (i < 0)
+        {
+	  if (!(p = the_pipeline))
+	      continue;
+	}
+      else if (jobs[i])
+	p = jobs[i]->pipe;
+      else
+	continue;
+
+      p_start = p;
+      do
+        {
+	  if (p->pid == pid && !p->reused)
+	    {
+	      p->reused = 1;
+	      goto done;
+	    }
+	  p = p->next;
+	}
+      while (p != p_start);
+    }
+ done:

   t = (PROCESS *)xmalloc (sizeof (PROCESS));
   t->next = the_pipeline;
@@ -745,6 +776,7 @@ add_process (name, pid)
   WSTATUS (t->status) = 0;
   t->running = PS_RUNNING;
   t->command = name;
+  t->reused = 0;
   the_pipeline = t;

   if (t->next == 0)
@@ -902,7 +934,7 @@ find_pipeline (pid, running_only, jobp)
       do
 	{
 	  /* Return it if we found it. */
-	  if (p->pid == pid)
+	  if (p->pid == pid && !p->reused)
 	    {
 	      if ((running_only && PRUNNING(p)) || (running_only == 0))
 		return (p);
@@ -937,7 +969,7 @@ find_job (pid, running_only)

 	  do
 	    {
-	      if (p->pid == pid)
+	      if (p->pid == pid && !p->reused)
 		{
 		  if ((running_only && PRUNNING(p)) || (running_only == 0))
 		    return (i);
@@ -1408,7 +1440,8 @@ make_child (command, async_p)
       if (async_p)
 	last_asynchronous_pid = pid;

-      last_made_pid = pid;
+      last_made_pid.pid = pid;
+      last_made_pid.seq = pid_sequence++;

       /* Unblock SIGINT and SIGCHLD. */
       sigprocmask (SIG_SETMASK, &oset, (sigset_t *)NULL);
--- jobs.h.orig	2002-01-17 12:35:12.000000000 -0500
+++ jobs.h	2004-09-17 21:27:08.000000000 -0400
@@ -55,6 +55,7 @@ typedef struct process {
   WAIT status;		/* The status of this command as returned by wait. */
   int running;		/* Non-zero if this process is running. */
   char *command;	/* The particular program that is running. */
+  int reused;
 } PROCESS;

 /* PRUNNING really means `not exited' */
@@ -104,9 +105,17 @@ typedef struct job {
 extern pid_t fork (), getpid (), getpgrp ();
 #endif /* !HAVE_UNISTD_H */

+/* Unique pid */
+typedef struct {
+  pid_t pid;
+  unsigned int seq;
+} upid_t;
+#define NO_UPID {(pid_t)-1, 0}
+
 /* Stuff from the jobs.c file. */
-extern pid_t original_pgrp, shell_pgrp, pipeline_pgrp;
-extern pid_t last_made_pid, last_asynchronous_pid;
+extern pid_t original_pgrp, shell_pgrp, pipeline_pgrp, last_asynchronous_pid;
+extern upid_t last_made_pid;
+extern unsigned int pid_sequence;
 extern int current_job, previous_job;
 extern int asynchronous_notification;
 extern JOB **jobs;
--- subst.c.orig	2004-09-17 21:03:52.000000000 -0400
+++ subst.c	2004-09-17 21:30:00.000000000 -0400
@@ -3449,7 +3449,8 @@ process_substitute (string, open_for_rea
 {
   char *pathname;
   int fd, result;
-  pid_t old_pid, pid;
+  upid_t old_pid;
+  pid_t pid;
 #if defined (HAVE_DEV_FD)
   int parent_pipe_fd, child_pipe_fd;
   int fildes[2];
@@ -3713,7 +3714,8 @@ command_substitute (string, quoted)
      char *string;
      int quoted;
 {
-  pid_t pid, old_pid, old_pipeline_pgrp;
+  pid_t pid, old_pipeline_pgrp;
+  upid_t old_pid;
   char *istring;
   int result, fildes[2], function_value;
   int i, closeit[3];

--
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/