Re: [libvirt] [PATCH 3/3] Replace setuid/setgid/initgroups with virSetUIDGID()

On 12/23/2010 11:39 AM, Laine Stump wrote: > This patch fixes https://bugzilla.redhat.com/show_bug.cgi?id=664406 > > If qemu is run as a different uid, it has been unable to access mode > 0660 files that are owned by a different user, but with a group that > the qemu is a member of (aside from the one group listed in the passwd > file), because initgroups() is not being called prior to the > exec. initgroups will change the group membership of the process (and > its children) to match the new uid. > > To make this happen, the setregid()/setreuid() code in > qemuSecurityDACSetProcessLabel has been replaced with a call to > virSetUIDGID(), which does both of those, plus calls initgroups. > > Similar, but not identical, code in qemudOpenAsUID() has been replaced > with virSetUIDGID(). This not only consolidates the functionality to a > single location, but also potentially fixes some as-yet unreported > bugs. > --- > src/qemu/qemu_driver.c | 44 +++++++++++++---------------------------- > src/qemu/qemu_security_dac.c | 18 +--------------- > 2 files changed, 16 insertions(+), 46 deletions(-) > + if (virSetUIDGID(uid, gid)< 0) { > + exit_code = errno; > + goto child_cleanup; Ah, I see why you needed patch 1 - patch two calls virReportSystemError in between the setting of errno and the return. However, one glitch - VIR_FREE(x) does not (yet) guarantee that errno is preserved (some poorly-written free() implementations can modify errno; glibc is nicer in trying to avoid that). You'll need to modify 1/3 accordingly. Which means ACK to this patch as perfect, but the prerequisites aren't there yet :)