Date: Sun, 14 Sep 2014 21:21:14 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Garrett Cooper <ngie@freebsd.org> Cc: src-committers@freebsd.org, svn-src-user@freebsd.org Subject: Re: svn commit: r271574 - user/ngie/add-pjdfstest/contrib/pjdfstest Message-ID: <20140914200428.P1559@besplex.bde.org> In-Reply-To: <201409140909.s8E99ncV087445@svn.freebsd.org> References: <201409140909.s8E99ncV087445@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, 14 Sep 2014, Garrett Cooper wrote:
> Log:
> Redo r271374 by using the right typecast for comparing i and ngroups
It is still quite broken (more broken than without the cast, since the
cast breaks the warning).
> Modified: user/ngie/add-pjdfstest/contrib/pjdfstest/pjdfstest.c
> ==============================================================================
> --- user/ngie/add-pjdfstest/contrib/pjdfstest/pjdfstest.c Sun Sep 14 09:06:35 2014 (r271573)
> +++ user/ngie/add-pjdfstest/contrib/pjdfstest/pjdfstest.c Sun Sep 14 09:09:49 2014 (r271574)
> @@ -1007,7 +1007,7 @@ set_gids(char *gids)
> assert(gidset != NULL);
> for (i = 0, g = strtok(gids, ","); g != NULL;
> g = strtok(NULL, ","), i++) {
> - if (i >= ngroups) {
> + if (i >= (unsigned)ngroups) {
ngroups has type long. In the unlikely event that it larger than UINT_MAX,
the cast breaks its value, giving an actual bug where there was none before.
The previous bug was to cast it to int. This breaks its value in the
unlikely event that it is larger than INT_MAX, but didn't break the
warning since i is bogusly unsigned so there was still a sign mismatch.
There are many nearby type errors. Many have already occurred:
% static void
% set_gids(char *gids)
% {
% gid_t *gidset;
gid_t is typedefed and is fairly opaque, so it is difficult to work with.
This is not really a bug (design error). However, it is a bug (foot
shooting) to use anything except int for counters of gids.
% long ngroups;
long is needed to hold the result of sysconf(). This is a bug (design
error). long is usually unnecessary. It is insufficent in general.
Ints work for most things, but in general intmax_t, uintmax_t or long
double is needed. The API should have used int so as not to make the
usual case harder.
% char *g, *endp;
% unsigned i;
Type error (foot shooting). Types that are not int are hard to work with.
%
% ngroups = sysconf(_SC_NGROUPS_MAX);
% assert(ngroups > 0);
This is correct to match the APIs so far. The value must be assigned to
a long before it is checked. The error handling is unportable. Oops,
this isn't correct. The error return value is -1. That really can't
happen. It is much more likely that a value of 0 is returned. This
can happen for old versions of POSIX. In current versions of POSIX,
it can't happen. But since this program doesn't support it and it
checks for another case that can't happen, it should check for both
cases.
ngroups should be assigned to a variable with a more usable type (i.e.,
int) at this point, after checking that it fits. This variable should
be spelled ngroups and the long variable should be spelled something
else.
% gidset = malloc(sizeof(*gidset) * ngroups);
This overflows if ngroups is very large. The check that it fits in
an int can be strengthend to check that this doesn't overflow.
% assert(gidset != NULL);
malloc() can't fail, especially if malloc() is broken and doesn't
respect RLIMIT_DATA. Its failure is about as likely sysconf() returning
-1, or overflow in the above.
The handling of this error when it is detected it is worse than a null
pointer trap.
If ngroups is 0, then a sucessful malloc() might return NULL. This is
detected as failure.
POSIX gives an example with similar code except for no error checking at
all. It also adds 1 to the result returned by sysconf(). This can
overflow too, in theory. I'm not sure what this addition is for. It
would handle the case where sysconf() returns 0 without much extra code,
by preventing passing 0 to malloc() and getting an ambiguous NULL return
for that. There are some complications involving whether the normal
gid is counted in the supplementary gids. The extra 1 may be for counting
it. FreeBSD used to have lots of off by 1 errors related to this. Not
adding 1 here might be unportable to POSIX systems that don't handle the
gid count like FreeBSD does now.
See ps/fmt.c:shquot() for the complications needed to use sysconf()
correctly for {ARG_MAX}. I'd like to have a library function to hide
some of these complications. IIRC, I asked for {NGROUPS} calculations
to use a specialized wrapper. {NGROUPS} is used more often than
{ARG_MAX}. Only the error handling for this is difficult. abort()
isn't enough for a library function. It might be enough for an
ngroups() function to return 0 or 1 on error and for callers to
ignore the error possibility (arrange to fail safe and act as if
there are no groups on error). An ngroups_alloc() function could
allocate the storage.
% for (i = 0, g = strtok(gids, ","); g != NULL;
% g = strtok(NULL, ","), i++) {
% if (i >= ngroups) {
Foot-shooting from i being unsigned begins here. unsigned i is neither
necessary or sufficient to reach long ngroups. This was detected indirectly
as a sign mismatch. If i were int, then the error wouldn't be detected.
'i' would still be unable to reach ngroups if ngroups were preposterously
large and long is larger than int.
% fprintf(stderr, "too many gids\n");
% exit(1);
% }
% gidset[i] = strtol(g, &endp, 0);
Unsigned types are valid array indexes, but I don't like them for that.
They are neither necessary or sufficient in general. Differences between
pointers into arrays have type ptrdiff_t. This is signed, and indexes
should be no different (except there is no requirement for ptrdiff_t to
actually work; it may be 16 bits on 64-bit systems). On 32-bit systems,
unsigned is larger than necessary for a general index, but on 64-bit
systems it is smaller than necessary. Here even a 16-bit int i is
large enough, since we assume that ngroups is not preposterously large.
% if (*endp != '\0' && !isspace((unsigned char)*endp)) {
% fprintf(stderr, "invalid gid '%s' - number expected\n",
% g);
% exit(1);
% }
% }
% if (setgroups(i, gidset) < 0) {
% fprintf(stderr, "cannot change groups: %s\n", strerror(errno));
% exit(1);
% }
Type error. setgroups() hasn't been poisoned by typedefs. It still takes
an int count. If i is actually large enough to need to be unsigned, then
it stops working when it overflows to a negative value here.
% if (setegid(gidset[0]) < 0) {
% fprintf(stderr, "cannot change effective gid: %s\n",
% strerror(errno));
% exit(1);
% }
% free(gidset);
% }
'int i' is often poisoned to 'unsigned i' or 'size_t i' to break warnings
about comparison of i with size_t variables where the size_t variables
often have small values. That doesn't apply in this function. The
program just does consistent foot shooting using 'unsigned i;'. Except,
'unsigned i;' is a style bug. The program uses the verbose style
'unsigned int i;' everywhere else. It doesn't use the concise KNF
'u_int i;' anywhere.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140914200428.P1559>