From owner-freebsd-hackers  Mon Apr 22 15:14:38 1996
Return-Path: owner-hackers
Received: (from root@localhost)
          by freefall.freebsd.org (8.7.3/8.7.3) id PAA18327
          for hackers-outgoing; Mon, 22 Apr 1996 15:14:38 -0700 (PDT)
Received: from vent.pipex.net (root@vent.pipex.net [158.43.128.5])
          by freefall.freebsd.org (8.7.3/8.7.3) with SMTP id PAA18243
          for <freebsd-hackers@freebsd.org>; Mon, 22 Apr 1996 15:13:30 -0700 (PDT)
Received:  from dial.pipex.com by vent.pipex.net (8.6.12/PIPEX simple 1.20)
	id XAA08352; Mon, 22 Apr 1996 23:12:23 +0100
Received: (from jraynard@localhost) by dial.pipex.com (8.6.12/8.6.12) id VAA00923; Mon, 22 Apr 1996 21:42:43 GMT
Date: Mon, 22 Apr 1996 21:42:43 GMT
From: James Raynard <jraynard@dial.pipex.com>
Message-Id: <199604222142.VAA00923@dial.pipex.com>
To: freebsd-hackers@freebsd.org
Subject: Flaws in system() implementation?
Sender: owner-hackers@freebsd.org
X-Loop: FreeBSD.org
Precedence: bulk

Inspired by Rich Stevens' implementation of system() in his book
"Advanced Programming in the Unix Environment", I've been looking at
the FreeBSD implementation and was struck by the following points:-

1. Use of a wait union when an int would do.

2. Use of the soon-to-be-obsolescent vfork() instead of fork().

3. Returns 0 if fork() fails, when -1 seems more appropriate.

4. The SIGINT and SIGQUIT are not ignored until after the fork().
   If the child runs first, one of these signals could in theory be
   generated before the parent gets around to ignoring it. Hence the
   dispositions should be changed before the fork().

Here's an untested patch which addresses these points.
Comments/corrections welcomed!

(In /usr/src/lib/libc/stdlib)
*** system.c~	Mon Apr 22 21:04:59 1996
--- system.c	Mon Apr 22 21:20:18 1996
***************
*** 46,76 ****
  system(command)
  	const char *command;
  {
- 	union wait pstat;
  	pid_t pid;
! 	int omask;
  	sig_t intsave, quitsave;
  
  	if (!command)		/* just checking... */
  		return(1);
  
  	omask = sigblock(sigmask(SIGCHLD));
! 	switch(pid = vfork()) {
  	case -1:			/* error */
! 		(void)sigsetmask(omask);
! 		pstat.w_status = 0;
! 		pstat.w_retcode = 127;
! 		return(pstat.w_status);
  	case 0:				/* child */
  		(void)sigsetmask(omask);
  		execl(_PATH_BSHELL, "sh", "-c", command, (char *)NULL);
  		_exit(127);
  	}
- 	intsave = signal(SIGINT, SIG_IGN);
- 	quitsave = signal(SIGQUIT, SIG_IGN);
- 	pid = waitpid(pid, (int *)&pstat, 0);
  	(void)sigsetmask(omask);
  	(void)signal(SIGINT, intsave);
  	(void)signal(SIGQUIT, quitsave);
! 	return(pid == -1 ? -1 : pstat.w_status);
  }
--- 46,77 ----
  system(command)
  	const char *command;
  {
  	pid_t pid;
! 	int omask, pstat;
  	sig_t intsave, quitsave;
  
  	if (!command)		/* just checking... */
  		return(1);
  
+ 	intsave = signal(SIGINT, SIG_IGN);
+ 	quitsave = signal(SIGQUIT, SIG_IGN);
  	omask = sigblock(sigmask(SIGCHLD));
! 	switch(pid = fork()) {
  	case -1:			/* error */
! 		break;
  	case 0:				/* child */
  		(void)sigsetmask(omask);
+ 		(void)signal(SIGINT, intsave);
+ 		(void)signal(SIGQUIT, quitsave);
  		execl(_PATH_BSHELL, "sh", "-c", command, (char *)NULL);
  		_exit(127);
+ 	default:			/* parent */
+ 		pid = waitpid(pid, &pstat, 0);
+ 		break;
  	}
  	(void)sigsetmask(omask);
  	(void)signal(SIGINT, intsave);
  	(void)signal(SIGQUIT, quitsave);
! 	return(pid == -1 ? -1 : pstat);
  }