Date: Sun, 15 Jul 2001 22:52:08 +1000 (EST) From: Bruce Evans <bde@zeta.org.au> To: Mike Barcroft <mike@q9media.com> Cc: audit@FreeBSD.org Subject: Re: nohup(1) enhancements patch Message-ID: <Pine.BSF.4.21.0107152237440.50666-100000@besplex.bde.org> In-Reply-To: <200107131450.f6DEoXF24866@coffee.q9media.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, 13 Jul 2001, Mike Barcroft wrote: > Bruce Evans <bde@zeta.org.au> writes: > > > I would appreciate comments on the patch at the end of this > > > message, also available at: > > > http://testbed.q9media.net/freebsd/nohup.20010713.patch > > >... > > > o Remove some FreeBSD specific access(2) cruft. > > > > This summarily blows away rev.1.2. > > Do you mean rev 1.3? If so, yes, access(2) suffers from a race, so Oops, yes. > you can't guarantee that your appending and not creating a new file. > I even thought about doing a series of open(2)s, one without O_CREAT > and one with it, but that too is a race because you can't guarentee > that the file wasn't created between your first and second open(2). > Better to be less discriptive, than lie to the user IMO. I hate access(2) too, but didn't care about this race. The more detailed message seemed to be useful enough to keep. On thinking about this some more, I see some longer races and larger bugs: - the message became out of date if another nohup process "appends" to nohup.out. - the message was misleading. nohup(1) doesn't actually append to nohup.out. It writes to the beginning, clobbering any existing output. - clobbering existing output is a bug. Both the man page and POSIX say that nohup(2) _appends_ to the file nohup.out. I didn't notice that you already fixed this (it's the most important change but you didn't mention it in the summary :-). > [snipped style bugs] > > Non-KNF-formatted comment. > > May violate POSIX's copyright. > > NetBSD and OpenBSD already have these POSIX requirements in their > source trees. What's the correct solution? Contact IEEE and see > if we can obtain permission, just leave it in, or pull it out? I don't really know. Large parts of out man pages could be improved by replacing them by the POSIX.1 or even the SUSv2 wording verbatim. SUSv2 apparently has permission to copy POSIX or vice versa. This doesn't matter so much for program source, but I feel that quoting whole paragraphs that give all the details for the subsequent C code goes a bit beyond fair use. > > > - if ((p = getenv("HOME"))) { > > > - (void)strcpy(path, p); > > > - (void)strcat(path, "/"); > > > - (void)strcat(path, FILENAME); > > > - append = !access(path, F_OK); > > > - if ((fd = open(p = path, > > > - O_RDWR|O_CREAT, S_IRUSR | S_IWUSR)) >= 0) > > > + if ((p = getenv("HOME")) != NULL && *p != '\0' && > > > + (strlen(p) + strlen(FILENAME) + 1) < sizeof(path)) { > > > + (void)snprintf(path, sizeof(path), "%s/%s", p, FILENAME); > > > > Why both check that the string fits and use snprintf()? > > I thought it was a bit easier to read, but it isn't very useful. > Reverted back to strcpy/strcat. I meant that you could replace the check by a check on the value returned by snprintf(). > nohup.20010714.patch > > o Integrate security enhancements from OpenBSD. > - Don't assume environment variable HOME is not NULL. > o Integrate standards compliance from NetBSD. > - Allow -- before the command. > - Blocking SIGQUIT isn't standards compliant. > - Proper exit(3) levels. - Actually append to nohup.out (as documented and required by standard) instead of clobbering it. > o Remove some FreeBSD specific access(2) cruft. (related to incorrect appending). > o Constify; Staticize functions; Set WARNS?=2 > o Tested on i386, and alpha. > Index: nohup/nohup.c > =================================================================== > RCS file: /home/ncvs/src/usr.bin/nohup/nohup.c,v > retrieving revision 1.5 > diff -u -r1.5 nohup.c > --- nohup/nohup.c 2000/03/26 14:46:41 1.5 > +++ nohup/nohup.c 2001/07/13 14:00:51 > @@ -57,67 +57,92 @@ > #include <string.h> > #include <unistd.h> > > -void dofile __P((void)); > +static void dofile __P((void)); > static void usage __P((void)); > > +#define FILENAME "nohup.out" > +/* Should be "/*-" (indent(1) protection for hand-formatted comment). > + * nohup shall exit with one of the following values: > + * 126 - The utility was found, but could not be invoked. > + * 127 - An error occurred in the nohup utility, or the utility could > + * not be found. > + */ > ... > + /* The nohup utility shall take the standard action for all signals > + except that SIGHUP shall be ignored. */ Still non-KNF formatting. > + /* If the standard output is a terminal, all output written to /* * If the standard output is a terminal, all output written to > + if ((p = getenv("HOME")) != NULL && *p != '\0' && > + (strlen(p) + strlen(FILENAME) + 1) < sizeof(path)) { > (void)strcpy(path, p); > (void)strcat(path, "/"); > (void)strcat(path, FILENAME); See above. > - if (append) > - (void)fprintf(stderr, "appending output to existing %s\n", p); > - else > - (void)fprintf(stderr, "sending output to %s\n", p); > + err(EXIT_MISC, NULL); > + (void)fprintf(stderr, "sending output to %s\n", p); I think "appending" is better than "sending" for all cases now. Bruce To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?Pine.BSF.4.21.0107152237440.50666-100000>