bin/152132: Useless code in script.c (part 2)

[Ronald F.Guilmette]

>Number:         152132
>Category:       bin
>Synopsis:       Useless code in script.c (part 2)
>Confidential:   no
>Severity:       non-critical
>Priority:       low
>Responsible:    freebsd-bugs
>State:          open
>Quarter:        
>Keywords:       
>Date-Required:
>Class:          change-request
>Submitter-Id:   current-users
>Arrival-Date:   Thu Nov 11 06:20:09 UTC 2010
>Closed-Date:
>Last-Modified:
>Originator:     Ronald F. Guilmette
>Release:        FreeBSD 7.0-RELEASE i386
>Organization:
Infinite Monkeys & Co.
>Environment:
System: FreeBSD segfault.tristatelogic.com 7.0-RELEASE FreeBSD 7.0-RELEASE #0: Tue Aug 5 02:38:40 PDT 2008 root at segfault.monkeys.com:/usr/src/sys/i386/compile/rfg20080805 i386

>Description:
	There appears to be a bunch of utterly useless (and perhaps even
	time wasting) code in script.c, and specifically within the
	finish() function.

	In that function there is a loop that reaps all child processes that
	are immediately available for reaping.  But this must be a leftover
	from an earlier time when script.c was implemented with -two- child
	processes.  Nowadays, it uses only one child process, i.e. the
	child shell process.  So it makes no sense to loop while trying to
	reap multiple child processes, because there is at most only one,
	the child shell process (and so it isn't even necessary or useful
	to call wait3() because a simple call to wait() should do.)

	Furthermore, if the idea is to reap the one and only child process,
	then before the reaping attempt even begins something positive
	should be done to cause the child to understand that it should exit
	now, i.e. calling close() on the "master" pty fd, i.e. just prior to
	all places in the code where the finish() function (or the done()
	function) is about to be called.  That should cause the child (shell)
	process to receive EOF on stdin/stdout/stderr, and that will probably
	cause it to figure out that it is time to go bye bye (if it hasn't
	done so already).

>How-To-Repeat:
	Look at the code.
>Fix:

*** script.c.orig	2004-02-15 09:30:13.000000000 -0800
--- script.c	2010-11-10 22:02:59.000000000 -0800
***************
*** 155,158 ****
--- 155,159 ----
  	if (child < 0) {
  		warn("fork");
+ 		(void)close(master);
  		done(1);
  	}
***************
*** 204,207 ****
--- 203,207 ----
  		}
  	}
+ 	(void)close(master);
  	finish();
  	done(0);
***************
*** 220,238 ****
  {
  	pid_t pid;
! 	int die, e, status;
  
! 	die = e = 0;
! 	while ((pid = wait3(&status, WNOHANG, 0)) > 0)
! 	        if (pid == child) {
! 			die = 1;
! 			if (WIFEXITED(status))
! 				e = WEXITSTATUS(status);
! 			else if (WIFSIGNALED(status))
! 				e = WTERMSIG(status);
! 			else /* can't happen */
! 				e = 1;
! 		}
  
! 	if (die)
  		done(e);
  }
--- 220,235 ----
  {
  	pid_t pid;
! 	int e, status;
  
! 	e = 0;
! 	wait(&status);
! 	if (WIFEXITED(status))
! 		e = WEXITSTATUS(status);
! 	else if (WIFSIGNALED(status))
! 		e = WTERMSIG(status);
! 	else /* can't happen */
! 		e = 1;
  
! 	if (e)
  		done(e);
  }
***************
*** 264,267 ****
--- 261,265 ----
  {
  	(void)kill(0, SIGTERM);
+ 	(void)close(master);
  	done(1);
  }
***************
*** 280,284 ****
  	}
  	(void)fclose(fscript);
- 	(void)close(master);
  	exit(eno);
  }
--- 278,281 ----
>Release-Note:
>Audit-Trail:
>Unformatted: