11:13 p.m.
The old semantics of virFork() violates the priciple of good
usability: it requires the caller to check the pid argument
after use, *even when virFork returned -1*, in order to properly
abort a child process that failed setup done immediately after
fork() - that is, the caller must call _exit() in the child.
While uses in virfile.c did this correctly, uses in 'virsh
lxc-enter-namespace' and 'virt-login-shell' would happily return
from the calling function in both the child and the parent,
leading to very confusing results. [Thankfully, I found the
problem by inspection, and can't actually trigger the double
return on error without an LD_PRELOAD library.]
It is much better if the semantics of virFork are impossible
to abuse. Looking at virFork(), the parent could only ever
return -1 with a non-negative pid if it misused pthread_sigmask,
but this never happens. Up until this patch series, the child
could return -1 with non-negative pid if it fails to set up
signals correctly, but we recently fixed that to make the child
call _exit() at that point instead of forcing the caller to do
it. Thus, the return value and contents of the pid argument are
now redundant (a -1 return now happens only for failure to fork,
a child 0 return only happens for a successful 0 pid, and a
parent 0 return only happens for a successful non-zero pid),
so we might as well return the pid directly rather than an
integer of whether it succeeded or failed; this is also good
from the interface design perspective as users are already
familiar with fork() semantics.
One last change in this patch: before returning the pid directly,
I found cases where using virProcessWait unconditionally on a
cleanup path of a virFork's -1 pid return would be nicer if there
were a way to avoid it overwriting an earlier message. While
such paths are a bit harder to come by with my change to a direct
pid return, I decided to keep the virProcessWait change in this
patch.
* src/util/vircommand.h (virFork): Change signature.
* src/util/vircommand.c (virFork): Guarantee that child will only
return on success, to simplify callers. Return pid rather than
status, now that the situations are always the same.
(virExec): Adjust caller, also avoid open-coding process death.
* src/util/virprocess.c (virProcessWait): Tweak semantics when pid
is -1.
(virProcessRunInMountNamespace): Adjust caller.
* src/util/virfile.c (virFileAccessibleAs, virFileOpenForked)
(virDirCreate): Likewise.
* tools/virt-login-shell.c (main): Likewise.
* tools/virsh-domain.c (cmdLxcEnterNamespace): Likewise.
* tests/commandtest.c (test23): Likewise.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/util/vircommand.c | 120 ++++++++++++++++++-----------------------------
src/util/vircommand.h | 2 +-
src/util/virfile.c | 23 ++-------
src/util/virprocess.c | 31 ++++++------
tests/commandtest.c | 12 ++---
tools/virsh-domain.c | 7 +--
tools/virt-login-shell.c | 12 +++--
7 files changed, 81 insertions(+), 126 deletions(-)
diff --git a/src/util/vircommand.c b/src/util/vircommand.c
index 415b8c3..db4166f 100644
--- a/src/util/vircommand.c
+++ b/src/util/vircommand.c
@@ -197,28 +197,28 @@ virCommandFDSet(virCommandPtr cmd,
/**
* virFork:
- * @pid - a pointer to a pid_t that will receive the return value from
- * fork()
*
* Wrapper around fork() that avoids various race/deadlock conditions.
*
- * on return from virFork(), if *pid < 0, the fork failed and there is
- * no new process. Otherwise, just like fork(), if *pid == 0, it is the
- * child process returning, and if *pid > 0, it is the parent.
- *
- * Even if *pid >= 0, if the return value from virFork() is < 0, it
- * indicates a failure that occurred in the parent or child process
- * after the fork. In this case, the child process should call
- * _exit(EXIT_CANCELED) after doing any additional error reporting.
+ * Like fork(), there are several return possibilities:
+ * 1. No child was created: the return is -1, errno is set, and an error
+ * message has been reported. The semantics of virWaitProcess() recognize
+ * this to avoid clobbering the error message from here.
+ * 2. This is the parent: the return is > 0. The parent can now attempt
+ * to interact with the child (but be aware that unlike raw fork(), the
+ * child may not return - some failures in the child result in this
+ * function calling _exit(EXIT_CANCELED) if the child cannot be set up
+ * correctly).
+ * 3. This is the child: the return is 0. If this happens, the parent
+ * is also guaranteed to return.
*/
-int
-virFork(pid_t *pid)
+pid_t
+virFork(void)
{
sigset_t oldmask, newmask;
struct sigaction sig_action;
- int saved_errno, ret = -1;
-
- *pid = -1;
+ int saved_errno;
+ pid_t pid;
/*
* Need to block signals now, so that child process can safely
@@ -226,54 +226,47 @@ virFork(pid_t *pid)
*/
sigfillset(&newmask);
if (pthread_sigmask(SIG_SETMASK, &newmask, &oldmask) != 0) {
- saved_errno = errno;
virReportSystemError(errno,
"%s", _("cannot block signals"));
- goto cleanup;
+ return -1;
}
/* Ensure we hold the logging lock, to protect child processes
* from deadlocking on another thread's inherited mutex state */
virLogLock();
- *pid = fork();
+ pid = fork();
saved_errno = errno; /* save for caller */
/* Unlock for both parent and child process */
virLogUnlock();
- if (*pid < 0) {
+ if (pid < 0) {
/* attempt to restore signal mask, but ignore failure, to
- avoid obscuring the fork failure */
+ * avoid obscuring the fork failure */
ignore_value(pthread_sigmask(SIG_SETMASK, &oldmask, NULL));
virReportSystemError(saved_errno,
"%s", _("cannot fork child process"));
- goto cleanup;
- }
-
- if (*pid) {
+ errno = saved_errno;
+ } else if (pid) {
/* parent process */
/* Restore our original signal mask now that the child is
- safely running */
- if (pthread_sigmask(SIG_SETMASK, &oldmask, NULL) != 0) {
- saved_errno = errno; /* save for caller */
- virReportSystemError(errno, "%s", _("cannot unblock
signals"));
- goto cleanup;
- }
- ret = 0;
+ * safely running. Only documented failures are EFAULT (not
+ * possible, since we are using just-grabbed mask) or EINVAL
+ * (not possible, since we are using correct arguments). */
+ ignore_value(pthread_sigmask(SIG_SETMASK, &oldmask, NULL));
} else {
-
/* child process */
int logprio;
size_t i;
- /* Remove any error callback so errors in child now
- get sent to stderr where they stand a fighting chance
- of being seen / logged */
+ /* Remove any error callback so errors in child now get sent
+ * to stderr where they stand a fighting chance of being seen
+ * and logged */
virSetErrorFunc(NULL, NULL);
virSetErrorLogPriorityFunc(NULL);
@@ -284,37 +277,30 @@ virFork(pid_t *pid)
virLogSetDefaultPriority(logprio);
/* Clear out all signal handlers from parent so nothing
- unexpected can happen in our child once we unblock
- signals */
+ * unexpected can happen in our child once we unblock
+ * signals */
sig_action.sa_handler = SIG_DFL;
sig_action.sa_flags = 0;
sigemptyset(&sig_action.sa_mask);
for (i = 1; i < NSIG; i++) {
- /* Only possible errors are EFAULT or EINVAL
- The former wont happen, the latter we
- expect, so no need to check return value */
-
- sigaction(i, &sig_action, NULL);
+ /* Only possible errors are EFAULT or EINVAL The former
+ * won't happen, the latter we expect, so no need to check
+ * return value */
+ ignore_value(sigaction(i, &sig_action, NULL));
}
- /* Unmask all signals in child, since we've no idea
- what the caller's done with their signal mask
- and don't want to propagate that to children */
+ /* Unmask all signals in child, since we've no idea what the
+ * caller's done with their signal mask and don't want to
+ * propagate that to children */
sigemptyset(&newmask);
if (pthread_sigmask(SIG_SETMASK, &newmask, NULL) != 0) {
- saved_errno = errno; /* save for caller */
virReportSystemError(errno, "%s", _("cannot unblock
signals"));
virDispatchError(NULL);
_exit(EXIT_CANCELED);
}
- ret = 0;
}
-
-cleanup:
- if (ret < 0)
- errno = saved_errno;
- return ret;
+ return pid;
}
/*
@@ -412,7 +398,7 @@ virExec(virCommandPtr cmd)
int tmpfd;
char *binarystr = NULL;
const char *binary = NULL;
- int forkRet, ret;
+ int ret;
struct sigaction waxon, waxoff;
gid_t *groups = NULL;
int ngroups;
@@ -489,17 +475,13 @@ virExec(virCommandPtr cmd)
if ((ngroups = virGetGroupList(cmd->uid, cmd->gid, &groups)) < 0)
goto cleanup;
- forkRet = virFork(&pid);
+ pid = virFork();
if (pid < 0) {
goto cleanup;
}
if (pid) { /* parent */
- if (forkRet < 0) {
- goto cleanup;
- }
-
VIR_FORCE_CLOSE(null);
if (cmd->outfdptr && *cmd->outfdptr == -1) {
VIR_FORCE_CLOSE(pipeout[1]);
@@ -521,13 +503,6 @@ virExec(virCommandPtr cmd)
/* child */
ret = EXIT_CANCELED;
- if (forkRet < 0) {
- /* The fork was successful, but after that there was an error
- * in the child (which was already logged).
- */
- goto fork_error;
- }
-
openmax = sysconf(_SC_OPEN_MAX);
if (openmax < 0) {
virReportSystemError(errno, "%s",
@@ -598,12 +573,10 @@ virExec(virCommandPtr cmd)
if (pid > 0) {
if (cmd->pidfile && (virPidFileWritePath(cmd->pidfile, pid)
< 0)) {
- kill(pid, SIGTERM);
- usleep(500*1000);
- kill(pid, SIGTERM);
- virReportSystemError(errno,
- _("could not write pidfile %s for %d"),
- cmd->pidfile, pid);
+ if (virProcessKillPainfully(pid, true) >= 0)
+ virReportSystemError(errno,
+ _("could not write pidfile %s for
%d"),
+ cmd->pidfile, pid);
goto fork_error;
}
_exit(EXIT_SUCCESS);
@@ -785,10 +758,9 @@ virExec(virCommandPtr cmd ATTRIBUTE_UNUSED)
return -1;
}
-int
-virFork(pid_t *pid)
+pid_t
+virFork(void)
{
- *pid = -1;
errno = ENOTSUP;
return -1;
diff --git a/src/util/vircommand.h b/src/util/vircommand.h
index ec6185d..7485edc 100644
--- a/src/util/vircommand.h
+++ b/src/util/vircommand.h
@@ -33,7 +33,7 @@ typedef virCommand *virCommandPtr;
* call any function that is not async-signal-safe. */
typedef int (*virExecHook)(void *data);
-int virFork(pid_t *pid) ATTRIBUTE_RETURN_CHECK;
+pid_t virFork(void) ATTRIBUTE_RETURN_CHECK;
int virRun(const char *const*argv, int *status) ATTRIBUTE_RETURN_CHECK;
diff --git a/src/util/virfile.c b/src/util/virfile.c
index 6fb7d6f..6da564b 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -1730,7 +1730,7 @@ virFileAccessibleAs(const char *path, int mode,
if (ngroups < 0)
return -1;
- forkRet = virFork(&pid);
+ pid = virFork();
if (pid < 0) {
VIR_FREE(groups);
@@ -1741,6 +1741,7 @@ virFileAccessibleAs(const char *path, int mode,
VIR_FREE(groups);
if (virProcessWait(pid, &status, false) < 0) {
/* virProcessWait() already reported error */
+ errno = EINTR;
return -1;
}
@@ -1836,7 +1837,6 @@ virFileOpenForked(const char *path, int openflags, mode_t mode,
int waitret, status, ret = 0;
int fd = -1;
int pair[2] = { -1, -1 };
- int forkRet;
gid_t *groups;
int ngroups;
@@ -1858,7 +1858,7 @@ virFileOpenForked(const char *path, int openflags, mode_t mode,
return ret;
}
- forkRet = virFork(&pid);
+ pid = virFork();
if (pid < 0)
return -errno;
@@ -1866,15 +1866,8 @@ virFileOpenForked(const char *path, int openflags, mode_t mode,
/* child */
- VIR_FORCE_CLOSE(pair[0]); /* preserves errno */
- if (forkRet < 0) {
- /* error encountered and logged in virFork() after the fork. */
- ret = -errno;
- goto childerror;
- }
-
/* set desired uid/gid, then attempt to create the file */
-
+ VIR_FORCE_CLOSE(pair[0]);
if (virSetUIDGID(uid, gid, groups, ngroups) < 0) {
ret = -errno;
goto childerror;
@@ -2145,7 +2138,7 @@ virDirCreate(const char *path,
if (ngroups < 0)
return -errno;
- int forkRet = virFork(&pid);
+ pid = virFork();
if (pid < 0) {
ret = -errno;
@@ -2175,13 +2168,7 @@ parenterror:
/* child */
- if (forkRet < 0) {
- /* error encountered and logged in virFork() after the fork. */
- goto childerror;
- }
-
/* set desired uid/gid, then attempt to create the directory */
-
if (virSetUIDGID(uid, gid, groups, ngroups) < 0) {
ret = -errno;
goto childerror;
diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index 72e9950..3620041 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -157,12 +157,16 @@ virProcessAbort(pid_t pid)
* @exitstatus: optional status collection
* @raw: whether to pass non-normal status back to caller
*
- * Wait for a child process to complete. If @exitstatus is NULL, then the
- * child must exit normally with status 0. Otherwise, if @raw is false,
- * the child must exit normally, and @exitstatus will contain the final
- * exit status (no need for the caller to use WEXITSTATUS()). If @raw is
- * true, then the result of wait() is returned in @exitstatus, and the
- * caller must use WIFEXITED() and friends to decipher the child's status.
+ * Wait for a child process to complete. If @pid is -1, do nothing, but
+ * return -1 (useful for error cleanup, and assumes an earlier message was
+ * already issued). All other pids issue an error message on failure.
+ *
+ * If @exitstatus is NULL, then the child must exit normally with status 0.
+ * Otherwise, if @raw is false, the child must exit normally, and
+ * @exitstatus will contain the final exit status (no need for the caller
+ * to use WEXITSTATUS()). If @raw is true, then the result of waitpid() is
+ * returned in @exitstatus, and the caller must use WIFEXITED() and friends
+ * to decipher the child's status.
*
* Returns 0 on a successful wait. Returns -1 on any error waiting for
* completion, or if the command completed with a status that cannot be
@@ -175,8 +179,9 @@ virProcessWait(pid_t pid, int *exitstatus, bool raw)
int status;
if (pid <= 0) {
- virReportSystemError(EINVAL, _("unable to wait for process %lld"),
- (long long) pid);
+ if (pid != -1)
+ virReportSystemError(EINVAL, _("unable to wait for process %lld"),
+ (long long) pid);
return -1;
}
@@ -956,16 +961,8 @@ virProcessRunInMountNamespace(pid_t pid,
return -1;
}
- ret = virFork(&child);
-
- if (ret < 0 || child < 0) {
- if (child == 0)
- _exit(EXIT_CANCELED);
-
- /* parent */
- virProcessAbort(child);
+ if ((child = virFork()) < 0)
goto cleanup;
- }
if (child == 0) {
VIR_FORCE_CLOSE(errfd[0]);
diff --git a/tests/commandtest.c b/tests/commandtest.c
index b329bfb..cb78a3c 100644
--- a/tests/commandtest.c
+++ b/tests/commandtest.c
@@ -972,12 +972,10 @@ test23(const void *unused ATTRIBUTE_UNUSED)
int status = -1;
pid_t pid;
- if (virFork(&pid) < 0)
- goto cleanup;
- if (pid < 0)
+ if ((pid = virFork()) < 0)
goto cleanup;
if (pid == 0) {
- if (virFork(&pid) < 0)
+ if ((pid = virFork()) < 0)
_exit(EXIT_FAILURE);
if (pid == 0)
_exit(42);
@@ -994,12 +992,10 @@ test23(const void *unused ATTRIBUTE_UNUSED)
goto cleanup;
}
- if (virFork(&pid) < 0)
- goto cleanup;
- if (pid < 0)
+ if ((pid = virFork()) < 0)
goto cleanup;
if (pid == 0) {
- if (virFork(&pid) < 0)
+ if ((pid = virFork()) < 0)
_exit(EXIT_FAILURE);
if (pid == 0) {
raise(SIGKILL);
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 5cd3271..4400d18 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -8198,9 +8198,10 @@ cmdLxcEnterNamespace(vshControl *ctl, const vshCmd *cmd)
}
/* Fork once because we don't want to affect
- * virsh's namespace itself
+ * virsh's namespace itself, and because user namespace
+ * can only be changed in single-threaded process
*/
- if (virFork(&pid) < 0)
+ if ((pid = virFork()) < 0)
goto cleanup;
if (pid == 0) {
if (setlabel &&
@@ -8221,7 +8222,7 @@ cmdLxcEnterNamespace(vshControl *ctl, const vshCmd *cmd)
/* Fork a second time because entering the
* pid namespace only takes effect after fork
*/
- if (virFork(&pid) < 0)
+ if ((pid = virFork()) < 0)
_exit(255);
if (pid == 0) {
execv(cmdargv[0], cmdargv);
diff --git a/tools/virt-login-shell.c b/tools/virt-login-shell.c
index 819cc5c..666facf 100644
--- a/tools/virt-login-shell.c
+++ b/tools/virt-login-shell.c
@@ -309,7 +309,10 @@ main(int argc, char **argv)
if (virDomainGetSecurityLabel(dom, seclabel) < 0)
goto cleanup;
- if (virFork(&cpid) < 0)
+ /* Fork once because we don't want to affect virt-login-shell's
+ * namespace itself. FIXME: is this necessary?
+ */
+ if ((cpid = virFork()) < 0)
goto cleanup;
if (cpid == 0) {
@@ -318,9 +321,6 @@ main(int argc, char **argv)
int openmax = sysconf(_SC_OPEN_MAX);
int fd;
- /* Fork once because we don't want to affect
- * virt-login-shell's namespace itself
- */
if (virSetUIDGID(0, 0, NULL, 0) < 0)
return EXIT_FAILURE;
@@ -355,7 +355,9 @@ main(int argc, char **argv)
VIR_MASS_CLOSE(tmpfd);
}
- if (virFork(&ccpid) < 0)
+ /* A fork is required to create new process in correct pid
+ * namespace. */
+ if ((ccpid = virFork()) < 0)
return EXIT_FAILURE;
if (ccpid == 0) {
--
1.8.5.3