Bug#926896: sysvinit-utils: pidof is unreliable

[Hoyer, David]

I am not seeing how it would have skipped the zombie processes in the past but I also did not closely review that code.    I did see in the comments that skipping those processes was put in place because the stats would sometimes fail.   I would argue that this should have been handled at the point where a stat was attempted.   I do see a special code path lower in the code for that.   Having said all this, I made the change locally to remove the check for zombie and disk access and now the program works every time for me.   I think this needs to be reverted and a different fix be put in place for handle the failed stat.   

Explanations for the other changes in this diff
1) When "pathname" was added to readproc, the spots where the memory is freed was not done consistent with the other frees.  There was also one missing spot.   
2) Cleaned up a code addition where spaces were used instead of tabs which was not consistent with the rest of the code.  
3) There was a block of old code which was commented out.   I went ahead and removed it.



diff --git a/src/killall5.c b/src/killall5.c
index 25b333e..6f45858 100644
--- a/src/killall5.c
+++ b/src/killall5.c
@@ -503,7 +503,7 @@ int readproc(int do_stat)
 		if (p->argv0) free(p->argv0);
 		if (p->argv1) free(p->argv1);
 		if (p->statname) free(p->statname);
-		free(p->pathname);
+		if (p->pathname) free(p->pathname);
 		free(p);
 	}
 	plist = NULL;
@@ -552,7 +552,7 @@ int readproc(int do_stat)
 					if (p->argv0) free(p->argv0);
 					if (p->argv1) free(p->argv1);
 					if (p->statname) free(p->statname);
-					free(p->pathname);
+					if (p->pathname) free(p->pathname);
 					free(p);
 					continue;
 				}
@@ -568,19 +568,12 @@ int readproc(int do_stat)
 
 			/* Get session, startcode, endcode. */
 			startcode = endcode = 0;
-                        /*
-			if (sscanf(q, 	"%*c %*d %*d %d %*d %*d %*u %*u "
+			if (sscanf(q,   "%10s %*d %*d %d %*d %*d %*u %*u "
 					"%*u %*u %*u %*u %*u %*d %*d "
 					"%*d %*d %*d %*d %*u %*u %*d "
 					"%*u %lu %lu",
-					&p->sid, &startcode, &endcode) != 3) {
-                        */
-                        if (sscanf(q,   "%10s %*d %*d %d %*d %*d %*u %*u "
-                                        "%*u %*u %*u %*u %*u %*d %*d "
-                                        "%*d %*d %*d %*d %*u %*u %*d "
-                                        "%*u %lu %lu",
-                                        process_status, 
-                                        &p->sid, &startcode, &endcode) != 4) {
+					process_status, 
+					&p->sid, &startcode, &endcode) != 4) {
 
 				p->sid = 0;
 				nsyslog(LOG_ERR, "can't read sid from %s\n",
@@ -589,31 +582,19 @@ int readproc(int do_stat)
 				if (p->argv0) free(p->argv0);
 				if (p->argv1) free(p->argv1);
 				if (p->statname) free(p->statname);
-				free(p->pathname);
+				if (p->pathname) free(p->pathname);
 				free(p);
 				continue;
 			}
 			if (startcode == 0 && endcode == 0)
 				p->kernel = 1;
 			fclose(fp);
-                        if ( (strchr(process_status, 'D') != NULL) ||
-                             (strchr(process_status, 'Z') != NULL) ){
-                           /* Ignore zombie processes or processes in
-                              disk sleep, as attempts
-                              to access the stats of these will
-                              sometimes fail. */
-                              if (p->argv0) free(p->argv0);
-                              if (p->argv1) free(p->argv1);
-                              if (p->statname) free(p->statname);
-                             free(p);
-                             continue;
-                        }
 		} else {
 			/* Process disappeared.. */
 			if (p->argv0) free(p->argv0);
 			if (p->argv1) free(p->argv1);
 			if (p->statname) free(p->statname);
-			free(p->pathname);
+			if (p->pathname) free(p->pathname);
 			free(p);
 			continue;
 		}
@@ -661,7 +642,7 @@ int readproc(int do_stat)
 			if (p->argv0) free(p->argv0);
 			if (p->argv1) free(p->argv1);
 			if (p->statname) free(p->statname);
-			free(p->pathname);
+			if (p->pathname) free(p->pathname);
 			free(p);
 			continue;
 		}