Date: Fri, 15 May 2009 13:32:52 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Brooks Davis <brooks@freebsd.org> 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 Message-ID: <20090515122400.B15792@delplex.bde.org> In-Reply-To: <200905140650.n4E6oURU079910@svn.freebsd.org> References: <200905140650.n4E6oURU079910@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <sys/param.h>
>
> +#include <stdlib.h>
Insertion sort error.
> #include <stdio.h>
> #include "namespace.h"
> #include <err.h>
> #include "un-namespace.h"
> +#include <errno.h>
Insertion sort error.
> #include <unistd.h>
>
> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20090515122400.B15792>