From owner-freebsd-hackers Tue Apr 23 13:24:18 1996 Return-Path: owner-hackers Received: (from root@localhost) by freefall.freebsd.org (8.7.3/8.7.3) id NAA02606 for hackers-outgoing; Tue, 23 Apr 1996 13:24:18 -0700 (PDT) Received: from ref.tfs.com (ref.tfs.com [140.145.254.251]) by freefall.freebsd.org (8.7.3/8.7.3) with ESMTP id NAA02447 for ; Tue, 23 Apr 1996 13:23:39 -0700 (PDT) Received: (from julian@localhost) by ref.tfs.com (8.7.3/8.6.9) id PAA20222; Mon, 22 Apr 1996 15:44:24 -0700 (PDT) Message-Id: <199604222244.PAA20222@ref.tfs.com> Subject: Re: Flaws in system() implementation? To: jraynard@dial.pipex.com (James Raynard) Date: Mon, 22 Apr 1996 15:44:24 -0700 (PDT) From: "JULIAN Elischer" Cc: hackers@freebsd.org In-Reply-To: <199604222142.VAA00923@dial.pipex.com> from "James Raynard" at Apr 22, 96 09:42:43 pm X-Mailer: ELM [version 2.4 PL25 ME8b] MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit 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. This is the correct POSIX thing to do.. check the man page for wait. > > > 3. Returns 0 if fork() fails, when -1 seems more appropriate. POSIX says: If "command" is a null pointer, the system() function returns non-zero only if a command processor is available. If "command" is a nin null pointer the system() function returnsa the termination status of the command language interpretter in the format specified by the waitpid() function. [...] If a child process cannot be created, or if the termination status of hte command interpretter cannot be obtained system() returns -1 and sets errno to indicate the error. > > 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 the child MUST run first in vfork() > 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); > } >