3:30 p.m.
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