[frit...@alokat.org: Re: [patch] lpr atoi -> strtonum]
- 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 - 1.48
> > +++ lpr/lpr.c 25 Sep 2015 12:08:57 -
> > @@ -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, );
> > + 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=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=text/plain_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, );
> 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, );
> > + 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 - 1.21
> > +++ lprm/lprm.c 25 Sep 2015 12:08:57 -
> > @@ -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, );
> > + if (errstr)
> > + fatal("invalid job number");
> > + requ[requests++] = i;
> > } else {
> > if (users >= MAXUSERS)
> > fatal("Too many users");
> >
>
- End forwarded message -
Re: [patch] lpr atoi -> strtonum
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 - 1.48
> > +++ lpr/lpr.c 25 Sep 2015 12:08:57 -
> > @@ -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, );
> > + 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=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=text/plain_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, );
> 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, );
> > + 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 - 1.21
> > +++ lprm/lprm.c 25 Sep 2015 12:08:57 -
> > @@ -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, );
> > + if (errstr)
> > + fatal("invalid job number");
> > + requ[requests++] = i;
> > } else {
> > if (users >= MAXUSERS)
> > fatal("Too many users");
> >
>
[patch] lpr atoi -> strtonum
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 - 1.48
+++ lpr/lpr.c 25 Sep 2015 12:08:57 -
@@ -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, );
+ if (errstr)
+ errx(1, "invalid quantity number");
+ if (i > 0)
+ ncopies = i;
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, );
+ 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 - 1.21
+++ lprm/lprm.c 25 Sep 2015 12:08:57 -
@@ -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, );
+ if (errstr)
+ fatal("invalid job number");
+ requ[requests++] = i;
} else {
if (users >= MAXUSERS)
fatal("Too many users");
