From owner-svn-src-projects@FreeBSD.ORG Fri May 15 06:23:11 2009 Return-Path: Delivered-To: svn-src-projects@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 944131065675; Fri, 15 May 2009 06:23:11 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from fallbackmx09.syd.optusnet.com.au (fallbackmx09.syd.optusnet.com.au [211.29.132.242]) by mx1.freebsd.org (Postfix) with ESMTP id 2D7968FC08; Fri, 15 May 2009 06:23:10 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail08.syd.optusnet.com.au (mail08.syd.optusnet.com.au [211.29.132.189]) by fallbackmx09.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id n4F3WvU8021716; Fri, 15 May 2009 13:32:57 +1000 Received: from c122-107-117-19.carlnfd1.nsw.optusnet.com.au (c122-107-117-19.carlnfd1.nsw.optusnet.com.au [122.107.117.19]) by mail08.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id n4F3WrlH011506 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Fri, 15 May 2009 13:32:54 +1000 Date: Fri, 15 May 2009 13:32:52 +1000 (EST) From: Bruce Evans X-X-Sender: bde@delplex.bde.org To: Brooks Davis In-Reply-To: <200905140650.n4E6oURU079910@svn.freebsd.org> Message-ID: <20090515122400.B15792@delplex.bde.org> References: <200905140650.n4E6oURU079910@svn.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: svn-src-projects@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r192087 - in projects/ngroups: lib/libc/gen lib/libc/rpc lib/libc/sys usr.bin/id usr.bin/newgrp usr.bin/quota usr.sbin/chown usr.sbin/chroot usr.sbin/jail usr.sbin/jexec usr.sbin/lpr/lpc X-BeenThere: svn-src-projects@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the src " projects" tree" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 15 May 2009 06:23:11 -0000 On Thu, 14 May 2009, Brooks Davis wrote: > Log: > Use to value returned by sysconf(_SC_NGROUPS_MAX) in favor of > NGROUPS_MAX or NGROUPS since POSIX says that NGROUPS_MAX represents a > lower bound on sysconf(_SC_NGROUPS_MAX). Actually, POSIX says that NGROUPS_MAX is identical to sysconf(_SC_NGROUPS_MAX) if it is defined (and the sysconf() succeeds). It is _POSIX_NGROUPS_MAX that gives the lower bound. _POSIX_NGROUPS_MAX is always 8, except under old versions of POSIX that don't require supplementary groups to be supported, where it is always 0. It is a bug for a POSIX implementation to define NGROUPS_MAX if its value is not always identical to sysconf(_SC_NGROUPS_MAX), or for a POSIX application to use NGROUPS_MAX unconditionally. > Modified: projects/ngroups/lib/libc/gen/initgroups.c > ============================================================================== > --- projects/ngroups/lib/libc/gen/initgroups.c Thu May 14 06:48:38 2009 (r192086) > +++ projects/ngroups/lib/libc/gen/initgroups.c Thu May 14 06:50:30 2009 (r192087) > @@ -35,10 +35,12 @@ __FBSDID("$FreeBSD$"); > > #include > > +#include Insertion sort error. > #include > #include "namespace.h" > #include > #include "un-namespace.h" > +#include Insertion sort error. > #include > > int > @@ -46,14 +48,20 @@ initgroups(uname, agroup) > const char *uname; > gid_t agroup; > { > - int ngroups; > + int ngroups, ret; > + gid_t *groups; Insertion sort error. > + > /* > - * Provide space for one group more than NGROUPS to allow > + * Provide space for one group more than possible to allow > * setgroups to fail and set errno. > */ > - gid_t groups[NGROUPS + 1]; > + ngroups = sysconf(_SC_NGROUPS_MAX) + 1; This is missing error checking and overflow checking: - sysconf() returns -1 on error - sysconf() returns long so assigning to an int gives undefined behaviour if its value is preposterously large - adding 1 to sysconf() may cause overflow before the assignment. > + groups = malloc(sizeof(gid_t)*ngroups); This has style bugs and is missing and is missing overflow checking: - missing spaces around binary operator - the multiplication overflows if ngroups is preposterously large See ps/args.c for the messy error handing needed for a similar case (_SC_ARG_MAX for ps). For groups, your changes would be cleaner and correcter using a utility function to handle all the details. Something like the following: %%% gid_t * allocgroups(void) { long ngroups_max; ngroups_max = sysconf(_SC_NGROUPS_MAX); if (ngroups_max < 0 || ngroups_max > LONG_MAX - 1 || ngroups_max > SIZE_MAX / sizeof(gid_t) - 1) return (NULL); return (malloc(sizeof(gid_t) * (ngroups_max + 1))); } %%% > + if (groups == NULL) > + return (ENOSPC); ENOSPC means no space on a device. ENOMEM should be used. Neither ENOSPC nor ENOMEM is a documented errno for initgroups(3). The documented errors are those for setgroups(2). > > - ngroups = NGROUPS + 1; > getgrouplist(uname, agroup, groups, &ngroups); > - return (setgroups(ngroups, groups)); > + ret = setgroups(ngroups, groups); > + free(groups); > + return(ret); > } Lost the space after 'return'. > ... Similarly, except the whitespace errors are smaller. One function aborts on getgroups() failure so maybe it can't handle returning NULL on malloc() failure. Another function uses err() for malloc() failure and warn() for getgroups() failure. Of course, malloc() cannot fail and library functions cannot abort or exit :-). > Modified: projects/ngroups/lib/libc/sys/getgroups.2 > ============================================================================== > --- projects/ngroups/lib/libc/sys/getgroups.2 Thu May 14 06:48:38 2009 (r192086) > +++ projects/ngroups/lib/libc/sys/getgroups.2 Thu May 14 06:50:30 2009 (r192087) > @@ -59,7 +59,7 @@ system call > returns the actual number of groups returned in > .Fa gidset . > No more than > -.Dv NGROUPS_MAX > +.Fn sysconf _SC_NGROUPS_MAX > will ever > be returned. > If This and many other places should use POSIX notation {FOO_MAX} for limits. Saying either NGROUPS_MAX or sysconf(_SC_NGROUPS_MAX} overspecifies the behaviour and makes it hard to change. Since getroups() is a syscall and you haven't changed the kernel yet, NGROUPS_MAX is still correct here. > @@ -91,7 +91,8 @@ an invalid address. > .El > .Sh SEE ALSO > .Xr setgroups 2 , > -.Xr initgroups 3 > +.Xr initgroups 3 , > +.Xr sysconf 3 POSIX doesn't bloat man pages that refer to {FOO_MAX} with references to sysconf(). Here the cross-reference is even less needed since the reference to sysconf() is explicit in context. Bruce