Based on a report by Coverity. waitpid() can leak resources if it fails with EINTR, so it should never be used without checking return status. But we already have a helper function that does that, so use it in more places. * src/lxc/lxc_controller.c (lxcPidGone): Check waitpid return value. (lxcControllerRun): Use simpler virPidAbort. * src/lxc/lxc_container.c (lxcContainerAvailable): Use safer virWaitPid. * daemon/libvirtd.c (daemonForkIntoBackground): Likewise. * tests/testutils.c (virtTestCaptureProgramOutput, virtTestMain): Likewise. * src/libvirt.c (virConnectAuthGainPolkit): Simplify with virCommand. --- v2: I decided to audit for all uses, not just the one pointed out by Coverity, and expanded the patch as a result. daemon/libvirtd.c | 6 +----- src/libvirt.c | 35 ++++++++++++----------------------- src/lxc/lxc_container.c | 5 ++--- src/lxc/lxc_controller.c | 14 +++++--------- tests/testutils.c | 7 ++++--- 5 files changed, 24 insertions(+), 43 deletions(-) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 2f0e1be..5e1fc96 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -232,18 +232,14 @@ static int daemonForkIntoBackground(const char *argv0) default: { - int got, exitstatus = 0; int ret; char status; VIR_FORCE_CLOSE(statuspipe[1]); /* We wait to make sure the first child forked successfully */ - if ((got = waitpid(pid, &exitstatus, 0)) < 0 || - got != pid || - exitstatus != 0) { + if (virPidWait(pid, NULL) < 0) return -1; - } /* Now block until the second child initializes successfully */ again: diff --git a/src/libvirt.c b/src/libvirt.c index 0b975da..a6bcee6 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -42,6 +42,7 @@ #include "intprops.h" #include "conf.h" #include "rpc/virnettlscontext.h" +#include "command.h" #ifndef WITH_DRIVER_MODULES # ifdef WITH_TEST @@ -108,34 +109,22 @@ static int initialized = 0; #if defined(POLKIT_AUTH) static int virConnectAuthGainPolkit(const char *privilege) { - const char *const args[] = { - POLKIT_AUTH, "--obtain", privilege, NULL - }; - int childpid, status, ret; + virCommandPtr cmd; + int status; + int ret = -1; - /* Root has all rights */ if (getuid() == 0) return 0; - if ((childpid = fork()) < 0) - return -1; - - if (!childpid) { - execvp(args[0], (char **)args); - _exit(-1); - } - - while ((ret = waitpid(childpid, &status, 0) == -1) && errno == EINTR); - if (ret == -1) { - return -1; - } - - if (!WIFEXITED(status) || - (WEXITSTATUS(status) != 0 && WEXITSTATUS(status) != 1)) { - return -1; - } + cmd = virCommandNewArgList(POLKIT_AUTH, "--obtain", privilege, NULL); + if (virCommandRun(cmd, &status) < 0 || + status > 1) + goto cleanup; - return 0; + ret = 0; +cleanup: + virCommandFree(cmd); + return ret; } #endif diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index e9891f7..06ccf7e 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -1229,7 +1229,6 @@ int lxcContainerAvailable(int features) int cpid; char *childStack; char *stack; - int childStatus; if (features & LXC_CONTAINER_FEATURE_USER) flags |= CLONE_NEWUSER; @@ -1251,8 +1250,8 @@ int lxcContainerAvailable(int features) VIR_DEBUG("clone call returned %s, container support is not enabled", virStrerror(errno, ebuf, sizeof ebuf)); return -1; - } else { - waitpid(cpid, &childStatus, 0); + } else if (virPidWait(cpid, NULL) < 0) { + return -1; } VIR_DEBUG("Mounted all filesystems"); diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index c4e7832..5cc7cb9 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -59,6 +59,7 @@ #include "util.h" #include "virfile.h" #include "virpidfile.h" +#include "command.h" #define VIR_FROM_THIS VIR_FROM_LXC @@ -515,7 +516,8 @@ ignorable_epoll_accept_errno(int errnum) static bool lxcPidGone(pid_t container) { - waitpid(container, NULL, WNOHANG); + if (waitpid(container, NULL, WNOHANG) < 0) + return false; if (kill(container, 0) < 0 && errno == ESRCH) @@ -1021,14 +1023,8 @@ cleanup: VIR_FORCE_CLOSE(loopDevs[i]); VIR_FREE(loopDevs); - if (container > 1) { - int status; - kill(container, SIGTERM); - if (!(waitpid(container, &status, WNOHANG) == 0 && - WIFEXITED(status))) - kill(container, SIGKILL); - waitpid(container, NULL, 0); - } + virPidAbort(container); + return rc; } diff --git a/tests/testutils.c b/tests/testutils.c index 00a8d8f..acdfdc1 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -33,6 +33,7 @@ #include "virterror_internal.h" #include "buf.h" #include "logging.h" +#include "command.h" #if TEST_OOM_TRACE # include <execinfo.h> @@ -309,7 +310,8 @@ virtTestCaptureProgramOutput(const char *const argv[], char **buf, int maxlen) VIR_FORCE_CLOSE(pipefd[1]); len = virFileReadLimFD(pipefd[0], maxlen, buf); VIR_FORCE_CLOSE(pipefd[0]); - waitpid(pid, NULL, 0); + if (virPidWait(pid, NULL) < 0) + return -1; return len; } @@ -674,8 +676,7 @@ int virtTestMain(int argc, } else { int i, status; for (i = 0 ; i < mp ; i++) { - waitpid(workers[i], &status, 0); - if (WEXITSTATUS(status) != EXIT_SUCCESS) + if (virPidWait(workers[i], NULL) < 0) ret = EXIT_FAILURE; } VIR_FREE(workers); -- 1.7.4.4

On 10/24/2011 01:58 AM, Daniel P. Berrange wrote: I guess I'd better review that, then :) OK, I split out the lxc_controller fixes (I'm saving them on my tree, though, until I know that your patches are in), and pushed the rest. -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org