[libvirt] [PATCH] virGetGroupList: always include the primary group

The change from initgroups to virGetGroupList/setgroups in cab36cfe71ba83b71e536ba5c98e596f02b697b0 dropped the primary group from processes group list iff the passed in group to virGetGroupList differs from the user's primary group. So always include the primary group to bring back the old behaviour. Debian has the kvm group as primary group but uses libvirt-qemu:libvirt-qemu as user:group to run the kvm process so without this change the /dev/kvm is inaccesible. --- src/util/virutil.c | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/src/util/virutil.c b/src/util/virutil.c index 3abcd53..12e1467 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -983,29 +983,41 @@ virGetGroupID(const char *group, gid_t *gid) } -/* 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. */ +/* Compute the list of primary and 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) { int ret = -1; char *user = NULL; + gid_t primary; *list = NULL; if (uid == (uid_t)-1) return 0; - if (virGetUserEnt(uid, &user, - gid == (gid_t)-1 ? &gid : NULL, NULL) < 0) + if (virGetUserEnt(uid, &user, &primary, NULL) < 0) return -1; - ret = mgetgroups(user, gid, list); - if (ret < 0) + ret = mgetgroups(user, primary, list); + if (ret < 0) { virReportSystemError(errno, _("cannot get group list for '%s'"), user); + goto cleanup; + } + + if (gid != (gid_t)-1) { + if (VIR_REALLOC_N(*list, ++ret) < 0) { + VIR_FREE(*list); + goto cleanup; + } + (*list)[ret-1] = gid; + } + +cleanup: VIR_FREE(user); return ret; } -- 1.8.3.2

On 08/05/2013 04:09 AM, Guido Günther wrote:
The change from initgroups to virGetGroupList/setgroups in cab36cfe71ba83b71e536ba5c98e596f02b697b0 dropped the primary group from processes group list iff the passed in group to virGetGroupList differs from the user's primary group.
So always include the primary group to bring back the old behaviour.
Debian has the kvm group as primary group but uses libvirt-qemu:libvirt-qemu as user:group to run the kvm process so without this change the /dev/kvm is inaccesible.
s/inaccesible/inaccessible/
--- src/util/virutil.c | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-)
+ + if (gid != (gid_t)-1) { + if (VIR_REALLOC_N(*list, ++ret) < 0) { + VIR_FREE(*list); + goto cleanup; + } + (*list)[ret-1] = gid; + }
This may allow gid to appear in the list more than once - I'd feel a bit more comfortable if you expanded the list only if you already validated that gid is not in the list. Also, using VIR_APPEND_ELEMENT would be nicer than VIR_REALLOC_N and manual list size manipulation. Looking forward to v2. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

The change from initgroups to virGetGroupList/setgroups in cab36cfe71ba83b71e536ba5c98e596f02b697b0 dropped the primary group from processes group list iff the passed in group to virGetGroupList differs from the user's primary group. So always include the primary group to bring back the old behaviour. Debian has the kvm group as primary group but uses libvirt-qemu:libvirt-qemu as user:group to run the kvm process so without this change the /dev/kvm is inaccessible. --- src/util/virutil.c | 38 +++++++++++++++++++++++++++++--------- 1 file changed, 29 insertions(+), 9 deletions(-) diff --git a/src/util/virutil.c b/src/util/virutil.c index 3abcd53..5500c9e 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -983,29 +983,49 @@ virGetGroupID(const char *group, gid_t *gid) } -/* 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. */ +/* Compute the list of primary and 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) { int ret = -1; char *user = NULL; + gid_t primary; *list = NULL; if (uid == (uid_t)-1) return 0; - if (virGetUserEnt(uid, &user, - gid == (gid_t)-1 ? &gid : NULL, NULL) < 0) + if (virGetUserEnt(uid, &user, &primary, NULL) < 0) return -1; - ret = mgetgroups(user, gid, list); - if (ret < 0) + ret = mgetgroups(user, primary, list); + if (ret < 0) { virReportSystemError(errno, _("cannot get group list for '%s'"), user); + goto cleanup; + } + + if (gid != (gid_t)-1) { + size_t n = ret; + + for (int i = 0; i < ret; i++) { + if((*list)[i] == gid) + goto cleanup; + } + if (VIR_APPEND_ELEMENT(*list, n, gid) < 0) { + ret = -1; + VIR_FREE(*list); + goto cleanup; + } else { + ret = n; + } + } + +cleanup: VIR_FREE(user); return ret; } -- 1.8.3.2

On 08/06/2013 10:36 AM, Guido Günther wrote:
The change from initgroups to virGetGroupList/setgroups in cab36cfe71ba83b71e536ba5c98e596f02b697b0 dropped the primary group from processes group list iff the passed in group to virGetGroupList differs from the user's primary group.
So always include the primary group to bring back the old behaviour.
Debian has the kvm group as primary group but uses libvirt-qemu:libvirt-qemu as user:group to run the kvm process so without this change the /dev/kvm is inaccessible. ---
+ for (int i = 0; i < ret; i++) { + if((*list)[i] == gid)
space after 'if' (hmm, that should be part of syntax-check) ACK with that fixed. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On Tue, Aug 06, 2013 at 10:54:36AM -0600, Eric Blake wrote:
On 08/06/2013 10:36 AM, Guido Günther wrote:
The change from initgroups to virGetGroupList/setgroups in cab36cfe71ba83b71e536ba5c98e596f02b697b0 dropped the primary group from processes group list iff the passed in group to virGetGroupList differs from the user's primary group.
So always include the primary group to bring back the old behaviour.
Debian has the kvm group as primary group but uses libvirt-qemu:libvirt-qemu as user:group to run the kvm process so without this change the /dev/kvm is inaccessible. ---
+ for (int i = 0; i < ret; i++) { + if((*list)[i] == gid)
space after 'if' (hmm, that should be part of syntax-check)
ACK with that fixed.
Fixed and pushed. Thanks, -- Guido
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake
-
Guido Günther