Re: svn commit: r243076 - head/usr.sbin/chkgrp

2012-11-16 Thread Bruce Evans 

On Thu, 15 Nov 2012, Konstantin Belousov wrote:


On Thu, Nov 15, 2012 at 01:52:46PM -0500, Eitan Adler wrote:

On 15 November 2012 11:52, Bruce Evans b...@optusnet.com.au wrote:

strtoul(1garbage, NULL, 10) succeeds and returns value 1, but the input
is garbage.


This case is covered earlier
160 /* check that the GID is numeric */
161 if (strspn(f[2], 0123456789) != strlen(f[2])) {
162 warnx(%s: line %d: GID is not numeric, gfn, n);

So this code shall be removed, if you are introducing strtoul() to check
for errors at all.


It is indeed strange to mix methods like this, and the check is more
strict that usual since it doesn't allow leading spaces.  I think leading
spaces are normally allowed for numeric args.  This normally happens
automatically, via bad programs using atoi() and better programs using
strto*().  Extra work like the above has to be done to reject leading
spaces.


As the man page says, the EINVAL feature is unportable.  It is almost
useless, since to detect garbage after the number you have to pass an
endptr to strtoul(), and then the check for no conversion (that is,
for garbage at the beginning) is just as easy as the check for garbage
at the end.


This patch doesn't care about EINVAL or ERANGE. It just cares strtoul
returned an error.


The only possible error for strtol() on a string of digits is ERANGE.


I even considered just ignoring the error case because the data is
mostly sanity checked prior.


The prior checking doesn't detect range errors.

The later checking detects range errors on 64-bit systems but not on 32-bit
ones, since strtoul() returns ULONG_MAX for range errors and that is a
valid gid (GID_MAX) on 32-bit systems.

Bruce
___
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org

svn commit: r243076 - head/usr.sbin/chkgrp

2012-11-15 Thread Eitan Adler 
Author: eadler
Date: Thu Nov 15 15:06:03 2012
New Revision: 243076
URL: http://svnweb.freebsd.org/changeset/base/243076

Log:
  Check the range of the gid
  
  Approved by:  cperciva
  MFC after:1 week

Modified:
  head/usr.sbin/chkgrp/chkgrp.c

Modified: head/usr.sbin/chkgrp/chkgrp.c
==
--- head/usr.sbin/chkgrp/chkgrp.c   Thu Nov 15 15:06:00 2012
(r243075)
+++ head/usr.sbin/chkgrp/chkgrp.c   Thu Nov 15 15:06:03 2012
(r243076)
@@ -30,7 +30,10 @@
 __FBSDID($FreeBSD$);
 
 #include err.h
+#include errno.h
 #include ctype.h
+#include limits.h
+#include stdint.h
 #include stdio.h
 #include stdlib.h
 #include string.h
@@ -150,6 +153,18 @@ main(int argc, char *argv[])
warnx(%s: line %d: GID is not numeric, gfn, n);
e++;
}
+
+   /* check the range of the group id */
+   errno = 0;
+   unsigned long groupid = strtoul(f[2], NULL, 10);
+   if (errno != 0) {
+   warnx(%s: line %d: strtoul failed, gfn, n);
+   }
+   else if (groupid  GID_MAX) {
+   warnx(%s: line %d: group id is too large ( %ju),
+ gfn, n, (uintmax_t)GID_MAX);
+   e++;
+   }

 #if 0
/* entry is correct, so print it */
___
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org

Re: svn commit: r243076 - head/usr.sbin/chkgrp

2012-11-15 Thread Konstantin Belousov 
On Thu, Nov 15, 2012 at 03:06:03PM +, Eitan Adler wrote:
 Author: eadler
 Date: Thu Nov 15 15:06:03 2012
 New Revision: 243076
 URL: http://svnweb.freebsd.org/changeset/base/243076
 
 Log:
   Check the range of the gid
   
   Approved by:cperciva
   MFC after:  1 week
 
 Modified:
   head/usr.sbin/chkgrp/chkgrp.c
 
 Modified: head/usr.sbin/chkgrp/chkgrp.c
 ==
 --- head/usr.sbin/chkgrp/chkgrp.c Thu Nov 15 15:06:00 2012
 (r243075)
 +++ head/usr.sbin/chkgrp/chkgrp.c Thu Nov 15 15:06:03 2012
 (r243076)
 @@ -30,7 +30,10 @@
  __FBSDID($FreeBSD$);
  
  #include err.h
 +#include errno.h
  #include ctype.h
 +#include limits.h
 +#include stdint.h
  #include stdio.h
  #include stdlib.h
  #include string.h
 @@ -150,6 +153,18 @@ main(int argc, char *argv[])
   warnx(%s: line %d: GID is not numeric, gfn, n);
   e++;
   }
 +
 + /* check the range of the group id */
 + errno = 0;
 + unsigned long groupid = strtoul(f[2], NULL, 10);
And this violates style.
The checks for strtoul failure are not exhaustive.

 + if (errno != 0) {
 + warnx(%s: line %d: strtoul failed, gfn, n);
 + }
 + else if (groupid  GID_MAX) {
 + warnx(%s: line %d: group id is too large ( %ju),
 +   gfn, n, (uintmax_t)GID_MAX);
 + e++;
 + }
   
  #if 0
   /* entry is correct, so print it */


pgpihrDFv7YB2.pgp
Description: PGP signature

Re: svn commit: r243076 - head/usr.sbin/chkgrp

2012-11-15 Thread Eitan Adler 
On 15 November 2012 10:30, Konstantin Belousov kostik...@gmail.com wrote:
 On Thu, Nov 15, 2012 at 03:06:03PM +, Eitan Adler wrote:
 Author: eadler
 Date: Thu Nov 15 15:06:03 2012
 New Revision: 243076
 URL: http://svnweb.freebsd.org/changeset/base/243076

 Log:
   Check the range of the gid

   Approved by:cperciva
   MFC after:  1 week

 Modified:
   head/usr.sbin/chkgrp/chkgrp.c

 Modified: head/usr.sbin/chkgrp/chkgrp.c
 ==
 --- head/usr.sbin/chkgrp/chkgrp.c Thu Nov 15 15:06:00 2012
 (r243075)
 +++ head/usr.sbin/chkgrp/chkgrp.c Thu Nov 15 15:06:03 2012
 (r243076)
 @@ -30,7 +30,10 @@
  __FBSDID($FreeBSD$);

  #include err.h
 +#include errno.h
  #include ctype.h
 +#include limits.h
 +#include stdint.h
  #include stdio.h
  #include stdlib.h
  #include string.h
 @@ -150,6 +153,18 @@ main(int argc, char *argv[])
   warnx(%s: line %d: GID is not numeric, gfn, n);
   e++;
   }
 +
 + /* check the range of the group id */
 + errno = 0;
 + unsigned long groupid = strtoul(f[2], NULL, 10);
 And this violates style.
 The checks for strtoul failure are not exhaustive.

from the strtoul man page:


...  In all cases, errno is set to ERANGE.
If no conversion could be performed, 0 is returned and
the global variable errno is set to EINVAL (the last feature is not por-
table across all platforms).
===

What is missing? Is there a case where strtoul fails but errno == 0 ?


-- 
Eitan Adler
Source, Ports, Doc committer
Bugmeister, Ports Security teams
___
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org

Re: svn commit: r243076 - head/usr.sbin/chkgrp

2012-11-15 Thread Bruce Evans 

On Thu, 15 Nov 2012, Eitan Adler wrote:


On 15 November 2012 10:30, Konstantin Belousov kostik...@gmail.com wrote:

On Thu, Nov 15, 2012 at 03:06:03PM +, Eitan Adler wrote:

Author: eadler
Date: Thu Nov 15 15:06:03 2012
New Revision: 243076
URL: http://svnweb.freebsd.org/changeset/base/243076

Log:
  Check the range of the gid

  Approved by:cperciva
  MFC after:  1 week

Modified:
  head/usr.sbin/chkgrp/chkgrp.c

Modified: head/usr.sbin/chkgrp/chkgrp.c
==
--- head/usr.sbin/chkgrp/chkgrp.c Thu Nov 15 15:06:00 2012(r243075)
+++ head/usr.sbin/chkgrp/chkgrp.c Thu Nov 15 15:06:03 2012(r243076)
@@ -30,7 +30,10 @@
 __FBSDID($FreeBSD$);

 #include err.h
+#include errno.h
 #include ctype.h
+#include limits.h
+#include stdint.h
 #include stdio.h
 #include stdlib.h
 #include string.h
@@ -150,6 +153,18 @@ main(int argc, char *argv[])
  warnx(%s: line %d: GID is not numeric, gfn, n);
  e++;
  }
+
+ /* check the range of the group id */
+ errno = 0;
+ unsigned long groupid = strtoul(f[2], NULL, 10);

And this violates style.


I see about 5 style violations here and 5 in other lines.


The checks for strtoul failure are not exhaustive.


from the strtoul man page:


...  In all cases, errno is set to ERANGE.
If no conversion could be performed, 0 is returned and
the global variable errno is set to EINVAL (the last feature is not por-
table across all platforms).
===

What is missing? Is there a case where strtoul fails but errno == 0 ?


strtoul(1garbage, NULL, 10) succeeds and returns value 1, but the input
is garbage.

As the man page says, the EINVAL feature is unportable.  It is almost
useless, since to detect garbage after the number you have to pass an
endptr to strtoul(), and then the check for no conversion (that is,
for garbage at the beginning) is just as easy as the check for garbage
at the end.

The error handling is poor.  First, warnx() is used instead of warn() so
as to not tell the user about the errno that we are depending on.  Fully
correct and laborious strto*() error reporting has to use warnx() only
for the case(s) where the input is garbage but strtoul() doesn't set
errno.  High quality strto*() error reporting also avoids using
strerror(errno) in other cases to get a uniform style of error messages.
The only portable other case is ERANGE for a range error.  strerror(errno)
is quite good for that.  However, we do another range check later and
should use the same error message for both.  strerror(EINVAL) is cryptic
for garbage at the beginning of the input as well as unportable.  Second,
we continue using the garbage value after detecting that it is garbage.

strtonum() and expand_number() handle some of these problems internally
but create others.  For example, if you don't like the error string created
by strtonum(), then you have to parse it to see what it says in order to
write a better one.  This is much more work than doing your own range
checking.

Bruce
___
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org

Re: svn commit: r243076 - head/usr.sbin/chkgrp

2012-11-15 Thread Eitan Adler 
On 15 November 2012 11:52, Bruce Evans b...@optusnet.com.au wrote:
 strtoul(1garbage, NULL, 10) succeeds and returns value 1, but the input
 is garbage.

This case is covered earlier
160 /* check that the GID is numeric */
161 if (strspn(f[2], 0123456789) != strlen(f[2])) {
162 warnx(%s: line %d: GID is not numeric, gfn, n);

 As the man page says, the EINVAL feature is unportable.  It is almost
 useless, since to detect garbage after the number you have to pass an
 endptr to strtoul(), and then the check for no conversion (that is,
 for garbage at the beginning) is just as easy as the check for garbage
 at the end.

This patch doesn't care about EINVAL or ERANGE. It just cares strtoul
returned an error.
I even considered just ignoring the error case because the data is
mostly sanity checked prior.


-- 
Eitan Adler
Source, Ports, Doc committer
Bugmeister, Ports Security teams
___
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org

Re: svn commit: r243076 - head/usr.sbin/chkgrp

2012-11-15 Thread Konstantin Belousov 
On Thu, Nov 15, 2012 at 01:52:46PM -0500, Eitan Adler wrote:
 On 15 November 2012 11:52, Bruce Evans b...@optusnet.com.au wrote:
  strtoul(1garbage, NULL, 10) succeeds and returns value 1, but the input
  is garbage.
 
 This case is covered earlier
 160 /* check that the GID is numeric */
 161 if (strspn(f[2], 0123456789) != strlen(f[2])) {
 162 warnx(%s: line %d: GID is not numeric, gfn, n);
So this code shall be removed, if you are introducing strtoul() to check
for errors at all.

 
  As the man page says, the EINVAL feature is unportable.  It is almost
  useless, since to detect garbage after the number you have to pass an
  endptr to strtoul(), and then the check for no conversion (that is,
  for garbage at the beginning) is just as easy as the check for garbage
  at the end.
 
 This patch doesn't care about EINVAL or ERANGE. It just cares strtoul
 returned an error.
 I even considered just ignoring the error case because the data is
 mostly sanity checked prior.
 
 
 -- 
 Eitan Adler
 Source, Ports, Doc committer
 Bugmeister, Ports Security teams


pgpihrnUBdMdS.pgp
Description: PGP signature