External/libvirt
1
0
Fork 0
mirror of https://gitlab.com/libvirt/libvirt.git synced 2024-07-30 13:37:17 +00:00
Code Issues Packages Projects Releases Wiki Activity

virFork: simplify semantics

Browse Source 
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@redhat.com>
This commit is contained in:
Eric Blake 2013-12-21 17:54:33 -07:00
parent b9dd878ff8
commit 25f87817ab
7 changed files with 81 additions and 126 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

120
src/util/vircommand.c
View File

@ -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;

2
src/util/vircommand.h
View File

@ -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;

23
src/util/virfile.c
View File

@ -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;

31
src/util/virprocess.c
View File

@ -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]);

12
tests/commandtest.c
View File

@ -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);

7
tools/virsh-domain.c
View File

@ -8177,9 +8177,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 &&
@ -8200,7 +8201,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);

12
tools/virt-login-shell.c
View File

@ -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) {