From owner-svn-src-user@FreeBSD.ORG Sun Sep 14 11:21:27 2014 Return-Path: Delivered-To: svn-src-user@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 18869679; Sun, 14 Sep 2014 11:21:27 +0000 (UTC) Received: from mail105.syd.optusnet.com.au (mail105.syd.optusnet.com.au [211.29.132.249]) by mx1.freebsd.org (Postfix) with ESMTP id CEF74AE5; Sun, 14 Sep 2014 11:21:26 +0000 (UTC) Received: from c122-106-147-133.carlnfd1.nsw.optusnet.com.au (c122-106-147-133.carlnfd1.nsw.optusnet.com.au [122.106.147.133]) by mail105.syd.optusnet.com.au (Postfix) with ESMTPS id 82A171045E36; Sun, 14 Sep 2014 21:21:16 +1000 (EST) Date: Sun, 14 Sep 2014 21:21:14 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Garrett Cooper Subject: Re: svn commit: r271574 - user/ngie/add-pjdfstest/contrib/pjdfstest In-Reply-To: <201409140909.s8E99ncV087445@svn.freebsd.org> Message-ID: <20140914200428.P1559@besplex.bde.org> References: <201409140909.s8E99ncV087445@svn.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=fvDlOjIf c=1 sm=1 tr=0 a=7NqvjVvQucbO2RlWB8PEog==:117 a=PO7r1zJSAAAA:8 a=LRTG6vsd6QsA:10 a=5LnWPWDnWWAA:10 a=kj9zAlcOel0A:10 a=JzwRw_2MAAAA:8 a=gmWSS0ejdzLk3m-1I9IA:9 a=CjuIK1q_8ugA:10 Cc: src-committers@freebsd.org, svn-src-user@freebsd.org X-BeenThere: svn-src-user@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: "SVN commit messages for the experimental " user" src tree" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 14 Sep 2014 11:21:27 -0000 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