[libvirt] [PATCH 1/2] util: add virGetGroupList
Laine Stump
laine at laine.org
Wed May 22 16:49:27 UTC 2013
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.
>
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
> configure.ac | 4 +--
> src/libvirt_private.syms | 1 +
> src/util/virutil.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++
> src/util/virutil.h | 2 ++
> 4 files changed, 98 insertions(+), 2 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index 2055637..d20b90e 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -205,8 +205,8 @@ AC_CHECK_SIZEOF([long])
>
> dnl Availability of various common functions (non-fatal if missing),
> dnl and various less common threadsafe functions
> -AC_CHECK_FUNCS_ONCE([cfmakeraw geteuid getgid getgrnam_r getmntent_r \
> - getpwuid_r getuid initgroups kill mmap newlocale posix_fallocate \
> +AC_CHECK_FUNCS_ONCE([cfmakeraw geteuid getgid getgrnam_r getgrouplist \
> + getmntent_r getpwuid_r getuid initgroups kill mmap newlocale posix_fallocate \
> posix_memalign prlimit regexec sched_getaffinity setns setrlimit symlink])
>
> dnl Availability of pthread functions (if missing, win32 threading is
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index a81a865..3d9d89f 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1933,6 +1933,7 @@ virGetDeviceID;
> virGetDeviceUnprivSGIO;
> virGetFCHostNameByWWN;
> virGetGroupID;
> +virGetGroupList;
> virGetGroupName;
> virGetHostname;
> virGetUnprivSGIOSysfsPath;
> diff --git a/src/util/virutil.c b/src/util/virutil.c
> index 239fba8..a3cb64f 100644
> --- a/src/util/virutil.c
> +++ b/src/util/virutil.c
> @@ -993,6 +993,99 @@ virGetGroupID(const char *group, gid_t *gid)
> return 0;
> }
>
> +
> +/* Compute the list of supplementary groups associated with @uid, and
> + * including @gid in the list (unless it is -1), storing a malloc'd
> + * result into @list. Return the size of the list on success, or -1
> + * on failure with error reported and errno set. May not be called
> + * between fork and exec. */
> +int
> +virGetGroupList(uid_t uid, gid_t gid, gid_t **list)
> +{
> + char *buf = NULL;
> + gid_t *groups = NULL;
> + int ngroups = 0;
> + int ret = -1;
> +
> + *list = NULL;
> + if (uid == (uid_t)-1)
> + return 0;
> +
> +# ifdef HAVE_GETGROUPLIST
> + {
> + struct passwd pwd, *pwd_result;
> + size_t bufsize;
> + int rc;
> +
> + bufsize = sysconf(_SC_GETPW_R_SIZE_MAX);
> + if (bufsize == -1)
> + bufsize = 16384;
> +
> + if (VIR_ALLOC_N(buf, bufsize) < 0) {
> + virReportOOMError();
> + goto error;
> + }
> + while ((rc = getpwuid_r(uid, &pwd, buf, bufsize,
> + &pwd_result)) == ERANGE) {
> + if (VIR_RESIZE_N(buf, bufsize, bufsize, bufsize) < 0) {
> + virReportOOMError();
> + goto error;
> + }
> + }
> +
> + if (rc) {
> + virReportSystemError(rc, _("cannot getpwuid_r(%u)"),
> + (unsigned int) uid);
> + goto error;
> + }
> +
> + if (!pwd_result) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("getpwuid_r failed to retrieve data "
> + "for uid '%u'"),
> + (unsigned int) uid);
> + goto error;
> + }
Up to here, it's just direct movement from virSetUIDGID.
Now we get to the new stuff:
> +
> + if (gid == (gid_t)-1)
> + gid = pwd.pw_gid;
The main use of the gid in the args is to specify one group that will
not be included in the returned group, and it's usually set to the group
listed in the pw record (got that from the manpage :-), so this makes sense.
> +
> + /* 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).
> +
> + if (VIR_ALLOC_N(groups, ngroups) < 0) {
> + virReportOOMError();
> + goto error;
> + }
> +
> + if (getgrouplist(pwd.pw_name, gid, groups, &ngroups) < 0) {
> + virReportSystemError(errno,
> + _("cannot getgrouplist(\"%s\", %d)"),
> + pwd.pw_name, (unsigned int) gid);
> + goto error;
> + }
> + }
> +# endif
> +
> + if (!ngroups && gid != (gid_t)-1) {
> + if (VIR_ALLOC_N(groups, 1) < 0) {
> + virReportOOMError();
> + goto error;
> + }
> + groups[ngroups++] = gid;
A reasonable fallback.
> + }
> +
> + *list = groups;
> + groups = NULL;
> + ret = ngroups;
> +
> +error:
> + VIR_FREE(buf);
> + VIR_FREE(groups);
> + return ret;
> +}
> +
> +
> /* Set the real and effective uid and gid to the given values, and call
> * initgroups so that the process has all the assumed group membership of
> * that uid. return 0 on success, -1 on failure (the original system error
> diff --git a/src/util/virutil.h b/src/util/virutil.h
> index 280a18d..6480b07 100644
> --- a/src/util/virutil.h
> +++ b/src/util/virutil.h
> @@ -115,6 +115,8 @@ char *virGetUserCacheDirectory(void);
> char *virGetUserRuntimeDirectory(void);
> char *virGetUserName(uid_t uid);
> char *virGetGroupName(gid_t gid);
> +int virGetGroupList(uid_t uid, gid_t group, gid_t **groups)
> + ATTRIBUTE_NONNULL(3);
> int virGetUserID(const char *name,
> uid_t *uid) ATTRIBUTE_RETURN_CHECK;
> int virGetGroupID(const char *name,
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).
More information about the libvir-list
mailing list