Date: Sat, 1 Aug 2015 20:35:56 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Baptiste Daroussin <bapt@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r286148 - head/usr.sbin/chkgrp Message-ID: <20150801200929.F2212@besplex.bde.org> In-Reply-To: <201508010835.t718ZKiH035944@repo.freebsd.org> References: <201508010835.t718ZKiH035944@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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., 0x100000001
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150801200929.F2212>