Re: [libvirt] [PATCH 1/2] util: add virGetGroupList
On 05/22/2013 10:49 AM, Laine Stump wrote:
On 05/21/2013 11:24 PM, Eric Blake wrote:
> Since neither getpwuid_r() nor initgroups() are safe to call in
> between fork and exec (they obtain a mutex, but if some other
> thread in the parent also held the mutex at the time of the fork,
> the child will deadlock), we have to split out the functionality
> that is unsafe. This patch adds a nice wrapper around
> getgrouplist, which does the lookup by uid instead of name and
> which mallocs the result; at least glibc's initgroups() uses
> getgrouplist under the hood.
>
> * configure.ac (AC_CHECK_FUNCS_ONCE): Check for getgrouplist.
> * src/util/virutil.h (virGetGroupList): New declaration.
> * src/util/virutil.c (virGetGroupList): New function.
> * src/libvirt_private.syms (virutil.h): Export it.
>
> +
> + /* Figure out what size list to expect */
> + getgrouplist(pwd.pw_name, gid, groups, &ngroups);
Do we need to be concerned about the "BUGS" info in the manpage?
BUGS
In glibc versions before 2.3.3, the implementation of
this function contains a buffer-overrun bug: it returns
the complete list of groups for user in the array
groups, even when the number of groups exceeds *ngroups.
Is anyone running that vintage of glibc? It sounds *kind of* like it
could hit that if ngroups is 0 (but doesn't specifically say that).
So I did some digging, and gnulib has an 'mgetgroups' module
(unfortunately GPL, but maybe I could get it relaxed if we wanted to use
that instead) that does the lookup by name rather than by uid_t. Here's
what gnulib's code says about the issue:
#if HAVE_GETGROUPLIST
/* We prefer to use getgrouplist if available, because it has better
performance characteristics.
In glibc 2.3.2, getgrouplist is buggy. If you pass a zero as the
length of the output buffer, getgrouplist will still write to the
buffer. Contrary to what some versions of the getgrouplist
manpage say, this doesn't happen with nonzero buffer sizes.
Therefore our usage here just avoids a zero sized buffer. */
if (username)
{
enum { N_GROUPS_INIT = 10 };
max_n_groups = N_GROUPS_INIT;
g = realloc_groupbuf (NULL, max_n_groups);
So it looks like I could indeed work around the bug by providing a
non-zero buffer in my probing version, and even go so far as to
pre-allocate a reasonable size buffer to get by with a single
getgrouplist() call if the given uid is in a small number of groups to
begin with (and what do you know - our most common "victim" uid will be
"qemu", which on a default-install Fedora system is uid 107 and a member
of exactly two groups: kvm(36) and qemu(107)).
Version 2 coming up with that idea incorporated.
> +
> + if (!ngroups && gid != (gid_t)-1) {
> + if (VIR_ALLOC_N(groups, 1) < 0) {
> + virReportOOMError();
> + goto error;
> + }
> + groups[ngroups++] = gid;
A reasonable fallback.
The gnulib module actually goes one further, and crawls through
getgrent() looking for any group that mentions uid (that is, instead of
ignoring supplementary groups, it actually tries a slower method of
getting at them). Maybe I'll pursue getting the gnulib module relaxed
to LGPL after all.
ACK. I'm curious about whether the bug listed in the manpage could ever
hit us, though (surely it can't be *that* bad, or if it is then everyone
will have a patch for it).
I'll do a v2 that either works around the bug, or which delegates to the
gnulib module (depending on response on the gnulib list about a license
relax request).
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 621 bytes)