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

2015-08-01 Thread Baptiste Daroussin
Author: bapt
Date: Sat Aug  1 08:35:20 2015
New Revision: 286148
URL: https://svnweb.freebsd.org/changeset/base/286148

Log:
  Use strtoumax instead of strtoul

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

Modified: head/usr.sbin/chkgrp/chkgrp.c
==
--- head/usr.sbin/chkgrp/chkgrp.c   Sat Aug  1 07:51:48 2015
(r286147)
+++ head/usr.sbin/chkgrp/chkgrp.c   Sat Aug  1 08:35:20 2015
(r286148)
@@ -32,6 +32,7 @@ __FBSDID($FreeBSD$);
 #include err.h
 #include errno.h
 #include ctype.h
+#include inttypes.h
 #include limits.h
 #include stdint.h
 #include stdio.h
@@ -169,9 +170,9 @@ main(int argc, char *argv[])
 
/* check the range of the group id */
errno = 0;
-   gid = strtoul(f[2], NULL, 10);
+   gid = strtoumax(f[2], NULL, 10);
if (errno != 0) {
-   warnx(%s: line %d: strtoul failed, gfn, n);
+   warnx(%s: line %d: strtoumax failed, gfn, n);
} else if (gid  GID_MAX) {
warnx(%s: line %d: group id is too large (%ju  %ju),
gfn, n, (uintmax_t)gid, (uintmax_t)GID_MAX);
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org


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

2015-08-01 Thread Bruce Evans

On Sat, 1 Aug 2015, Baptiste Daroussin wrote:


Log:
 Use strtoumax instead of strtoul


This does nothing good, and breaks 32-bit arches.


Modified: head/usr.sbin/chkgrp/chkgrp.c
==
--- head/usr.sbin/chkgrp/chkgrp.c   Sat Aug  1 07:51:48 2015
(r286147)
+++ head/usr.sbin/chkgrp/chkgrp.c   Sat Aug  1 08:35:20 2015
(r286148)
@@ -169,9 +170,9 @@ main(int argc, char *argv[])

/* check the range of the group id */
errno = 0;
-   gid = strtoul(f[2], NULL, 10);
+   gid = strtoumax(f[2], NULL, 10);


gid still has type u_long (spelled verbosely as unsigned long.)  This
used to work on arches where gid_t is no larger than u_long, which is
the case on all supported arches.  Now the value returned by strtoumax()
is corrupted by blindly assigning it to u_long.  E.g., 0x10001
becomes 1.


if (errno != 0) {
-   warnx(%s: line %d: strtoul failed, gfn, n);
+   warnx(%s: line %d: strtoumax failed, gfn, n);


This has a less usual subset of common bugs in using the strtoul() family.
The most common bug is to not check ERANGE.  This does check ERANGE, but
doesn't check for garbage following the integer (it doesn't even pass
endptr).  It uses a POSIX extension to check that the integer has some
digits.


} else if (gid  GID_MAX) {


This used to work.  Now it checks the corrupted value.


warnx(%s: line %d: group id is too large (%ju  %ju),
gfn, n, (uintmax_t)gid, (uintmax_t)GID_MAX);


This format is bogus for printing `gid'.  gid should have type gid_t, but
actually has type u_long.  u_long should be printed using %lu and no
cast.

GID_MAX is undocumented, so no one knows its type.  Its type should
be gid_t, but is actually u_int.  gid_t is logically different again
-- it is __uint32_t.  GID_MAX is defined in sys/limits.h, so there are
namespace problems in matching its type with that of gid_t.  The
current implementation is best until gid_t is expanded.

So casting GID_MAX to uintmax_t is good for safety and portability.
However, chckgrp.c already assumed that gid_t is no larger than u_long.
Otherwise it would have been broken in the reverse way to this commit
-- values would have been clamped by strtoul().  The bounds checking
would have worked, but large values would have been unsupported.  The
old value might as well have cast to u_long.  strtoumax() can support
any size for gid_t.  After fixing all the other type errors, the types
in the warnx() become correct.

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