----- Forwarded message from Fritjof Bornebusch <frit...@alokat.org> -----
Date: Sat, 26 Sep 2015 22:00:58 +0200 From: Fritjof Bornebusch <frit...@alokat.org> To: Michael Reed <m.r...@mykolab.com> Cc: tech@openbsd.org Subject: Re: [patch] lpr atoi -> strtonum On Fri, Sep 25, 2015 at 02:23:21PM -0400, Michael Reed wrote: Ping .... > Hi Fritjof, > Hi Michael, > I left one comment inline. > thanks. > On 09/25/15 08:18, Fritjof Bornebusch wrote: > > Hi, > > > > change atoi(3) -> strtonum(3) in lpr(1) and lprm(1). > > lprm(1) avoids negative numbers to be the first argument by using getopt(3), > > but supported values like 2.2. > > > > --F. > > > > > > Index: lpr/lpr.c > > =================================================================== > > RCS file: /cvs/src/usr.sbin/lpr/lpr/lpr.c,v > > retrieving revision 1.48 > > diff -u -p -r1.48 lpr.c > > --- lpr/lpr.c 9 Feb 2015 23:00:14 -0000 1.48 > > +++ lpr/lpr.c 25 Sep 2015 12:08:57 -0000 > > @@ -112,6 +112,7 @@ main(int argc, char **argv) > > char buf[PATH_MAX]; > > int i, f, ch; > > struct stat stb; > > + const char *errstr; > > > > /* > > * Simulate setuid daemon w/ PRIV_END called. > > @@ -145,11 +146,11 @@ main(int argc, char **argv) > > switch (ch) { > > > > case '#': /* n copies */ > > - if (isdigit((unsigned char)*optarg)) { > > - i = atoi(optarg); > > - if (i > 0) > > - ncopies = i; > > - } > > + i = strtonum(optarg, 0, INT_MAX, &errstr); > > + if (errstr) > > + errx(1, "invalid quantity number"); > > + if (i > 0) > > + ncopies = i; > > I might be missing something, but why silently allow -#0 ? The default value is 1 and if -#0 is called, this default value is used. Disallow -#0 silently makes this default value useless. And other BSD versions of lpr(1) uses this default value: https://svnweb.freebsd.org/base/head/usr.sbin/lpr/lpr/lpr.c?revision=275855&view=co http://gitweb.dragonflybsd.org/dragonfly.git/blob_plain/HEAD:/usr.sbin/lpr/lpr/lpr.c http://cvsweb.netbsd.org/bsdweb.cgi/~checkout~/src/usr.sbin/lpr/lpr/lpr.c?rev=1.46&content-type=text/plain&only_with_tag=MAIN So I think this behavior should not be changed. The above versions redirect negative values to 1 as well, but calling -#-1 looks wrong (what should lpr(1) print, if -1 copies are requested), so I think starting from 0 is okay. > Besides that, this isn't as informative as it could be IMO; perhaps > this is better: > > case '#': /* n copies */ > ncopies = strtonum(optarg, 1, INT_MAX, &errstr); > if (errstr) > errx(1, "number of copies %s: %s", errstr, optarg); > break; > > > break; > > > > case '4': /* troff fonts */ > > @@ -203,7 +204,9 @@ main(int argc, char **argv) > > > > case 'i': /* indent output */ > > iflag++; > > - indent = atoi(optarg); > > + indent = strtonum(optarg, 0, INT_MAX, &errstr); > > + if (errstr) > > + errx(1, "invalid number"); > > if (indent < 0) > > indent = 8; > > break; > > Index: lprm/lprm.c > > =================================================================== > > RCS file: /cvs/src/usr.sbin/lpr/lprm/lprm.c,v > > retrieving revision 1.21 > > diff -u -p -r1.21 lprm.c > > --- lprm/lprm.c 16 Jan 2015 06:40:18 -0000 1.21 > > +++ lprm/lprm.c 25 Sep 2015 12:08:57 -0000 > > @@ -77,8 +77,9 @@ main(int argc, char **argv) > > { > > struct passwd *pw; > > char *cp; > > + const char *errstr; > > long l; > > - int ch; > > + int ch, i; > > > > /* > > * Simulate setuid daemon w/ PRIV_END called. > > @@ -135,7 +136,10 @@ main(int argc, char **argv) > > if (isdigit((unsigned char)*argv[0])) { > > if (requests >= MAXREQUESTS) > > fatal("Too many requests"); > > - requ[requests++] = atoi(argv[0]); > > + i = strtonum(argv[0], 0, INT_MAX, &errstr); > > + if (errstr) > > + fatal("invalid job number"); > > + requ[requests++] = i; > > } else { > > if (users >= MAXUSERS) > > fatal("Too many users"); > > > ----- End forwarded message -----