On Fri, Sep 25, 2015 at 02:23:21PM -0400, Michael Reed wrote:
> 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");
> >
>
