Re: [libvirt] [PATCHv2 3/3] util: make virSetUIDGID async-signal-safe

On 07/09/2013 09:17 PM, Eric Blake wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=964358 > > POSIX states that multi-threaded apps should not use functions > that are not async-signal-safe between fork and exec, yet we > were using getpwuid_r and initgroups. Although rare, it is > possible to hit deadlock in the child, when it tries to grab > a mutex that was already held by another thread in the parent. > I actually hit this deadlock when testing multiple domains > being started in parallel with a command hook, with the following > backtrace in the child: >

> +++ b/src/lxc/lxc_container.c > @@ -1,5 +1,5 @@ > /* > - * Copyright (C) 2008-2012 Red Hat, Inc. > + * Copyright (C) 2008-2013 Red Hat, Inc. > * Copyright (C) 2008 IBM Corp. > * > * lxc_container.c: file description > @@ -347,12 +347,17 @@ int lxcContainerWaitForContinue(int control) > */ > static int lxcContainerSetID(virDomainDefPtr def) > { > + gid_t *groups; > + int ngroups; > + > /* Only call virSetUIDGID when user namespace is enabled > * for this container. And user namespace is only enabled > * when nuidmap&ngidmap is not zero */ > > VIR_DEBUG("Set UID/GID to 0/0"); > - if (def->idmap.nuidmap && virSetUIDGID(0, 0) < 0) { > + if (def->idmap.nuidmap && > + ((ngroups = virGetGroupList(0, 0, &groups) < 0) || > + virSetUIDGID(0, 0, groups, ngroups) < 0)) { > virReportSystemError(errno, "%s", > _("setuid or setgid failed")); > return -1;

> @@ -1904,7 +1928,7 @@ parenterror: > > /* set desired uid/gid, then attempt to create the directory */ > > - if (virSetUIDGID(uid, gid) < 0) { > + if (virSetUIDGID(uid, gid, groups, ngroups) < 0) { > ret = -errno; > goto childerror; > } It's too late for me to be reviewing code - I can tell because I spent several minutes being concerned that you didn't free groups in this code path. Duh. That's really all I can say to myself :-)