[libvirt] [PATCH 0/2] Make a wrapper for fork()
This was partly prompted by DV's suggestion last week. The first of these patches creates a new function called virFork() which behaves (almost) like fork() but takes care of some important details that pretty much any call to fork() should be doing. The 2nd switches three fork-calling functions in util.c over to using virFork() instead of fork(). In the future, except for odd circumstances, code that needs to fork should call virFork() instead, and if there is anything determined to be universally necessary at fork-time, it should be added to virFork() rather than to the callers of virFork(); hopefully this will ease maintenance and reduce replicated bugs. (Note that, while this is just an overall "code health" patch, a couple bug fix patches I'll be submitting either tomorrow or Thursday will assume it as a prerequisite).
virFork() contains bookkeeping that must be done any time a process forks. Currently this includes: 1) Call virLogLock() prior to fork() and virLogUnlock() just after, to avoid a deadlock if some other thread happens to hold that lock during the fork. 2) Reset the logging hooks and send all child process log messages to stderr. 3) Block all signals prior to fork(), then either a) reset the signal mask for the parent process, or b) clear the signal mask for the child process. util.c, util.h: add virFork() function, based on what is currently done in __virExec(). --- src/util/util.c | 115 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ src/util/util.h | 1 + 2 files changed, 116 insertions(+), 0 deletions(-) diff --git a/src/util/util.c b/src/util/util.c index cdab300..f508f6b 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -296,6 +296,121 @@ static int virClearCapabilities(void) } #endif + +/* virFork() - fork a new process while avoiding various race/deadlock conditions + + @pid - a pointer to a pid_t that will receive the return value from + fork() + + 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(1) after doing any additional error reporting. + + */ +int virFork(pid_t *pid) { + sigset_t oldmask, newmask; + struct sigaction sig_action; + int saved_errno, ret = -1; + + *pid = -1; + + /* + * Need to block signals now, so that child process can safely + * kill off caller's signal handlers without a race. + */ + sigfillset(&newmask); + if (pthread_sigmask(SIG_SETMASK, &newmask, &oldmask) != 0) { + saved_errno = errno; + virReportSystemError(errno, + "%s", _("cannot block signals")); + goto cleanup; + } + + /* Ensure we hold the logging lock, to protect child processes + * from deadlocking on another thread's inherited mutex state */ + virLogLock(); + + *pid = fork(); + saved_errno = errno; /* save for caller */ + + /* Unlock for both parent and child process */ + virLogUnlock(); + + if (*pid < 0) { + virReportSystemError(saved_errno, + "%s", _("cannot fork child process")); + goto cleanup; + } + + 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; + + } else { + + /* child process */ + + int logprio; + int i; + + /* Remove any error callback so errors in child now + get sent to stderr where they stand a fighting chance + of being seen / logged */ + virSetErrorFunc(NULL, NULL); + + /* Make sure any hook logging is sent to stderr, since child + * process may close the logfile FDs */ + logprio = virLogGetDefaultPriority(); + virLogReset(); + virLogSetDefaultPriority(logprio); + + /* Clear out all signal handlers from parent so nothing + 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); + } + + /* 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")); + goto cleanup; + } + ret = 0; + } + +cleanup: + if (ret < 0) + errno = saved_errno; + return ret; +} + /* * @argv argv to exec * @envp optional environment to use for exec diff --git a/src/util/util.h b/src/util/util.h index 4207508..8460100 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -81,6 +81,7 @@ int virRun(const char *const*argv, int *status) ATTRIBUTE_RETURN_CHECK; int virRunWithHook(const char *const*argv, virExecHook hook, void *data, int *status) ATTRIBUTE_RETURN_CHECK; +int virFork(pid_t *pid); int virFileReadLimFD(int fd, int maxlen, char **buf) ATTRIBUTE_RETURN_CHECK; -- 1.6.6.1
On Wed, Feb 17, 2010 at 02:29:27PM -0500, Laine Stump wrote:
virFork() contains bookkeeping that must be done any time a process forks. Currently this includes:
1) Call virLogLock() prior to fork() and virLogUnlock() just after, to avoid a deadlock if some other thread happens to hold that lock during the fork.
2) Reset the logging hooks and send all child process log messages to stderr.
3) Block all signals prior to fork(), then either a) reset the signal mask for the parent process, or b) clear the signal mask for the child process.
util.c, util.h: add virFork() function, based on what is currently done in __virExec(). --- src/util/util.c | 115 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ src/util/util.h | 1 + 2 files changed, 116 insertions(+), 0 deletions(-)
diff --git a/src/util/util.c b/src/util/util.c index cdab300..f508f6b 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -296,6 +296,121 @@ static int virClearCapabilities(void) } #endif
+ +/* virFork() - fork a new process while avoiding various race/deadlock conditions + + @pid - a pointer to a pid_t that will receive the return value from + fork() + + 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(1) after doing any additional error reporting. + + */ +int virFork(pid_t *pid) { + sigset_t oldmask, newmask; + struct sigaction sig_action; + int saved_errno, ret = -1; + + *pid = -1; + + /* + * Need to block signals now, so that child process can safely + * kill off caller's signal handlers without a race. + */ + sigfillset(&newmask); + if (pthread_sigmask(SIG_SETMASK, &newmask, &oldmask) != 0) { + saved_errno = errno; + virReportSystemError(errno, + "%s", _("cannot block signals")); + goto cleanup; + } + + /* Ensure we hold the logging lock, to protect child processes + * from deadlocking on another thread's inherited mutex state */ + virLogLock(); + + *pid = fork(); + saved_errno = errno; /* save for caller */ + + /* Unlock for both parent and child process */ + virLogUnlock(); + + if (*pid < 0) { + virReportSystemError(saved_errno, + "%s", _("cannot fork child process")); + goto cleanup; + }
Tiny bug here, in that we forget to unblock the parent's signals in this error path.
+ + 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; + + } else { + + /* child process */ + + int logprio; + int i; + + /* Remove any error callback so errors in child now + get sent to stderr where they stand a fighting chance + of being seen / logged */ + virSetErrorFunc(NULL, NULL); + + /* Make sure any hook logging is sent to stderr, since child + * process may close the logfile FDs */ + logprio = virLogGetDefaultPriority(); + virLogReset(); + virLogSetDefaultPriority(logprio); + + /* Clear out all signal handlers from parent so nothing + 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); + } + + /* 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")); + goto cleanup; + } + ret = 0; + } + +cleanup: + if (ret < 0) + errno = saved_errno; + return ret; +} + /* * @argv argv to exec * @envp optional environment to use for exec diff --git a/src/util/util.h b/src/util/util.h index 4207508..8460100 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -81,6 +81,7 @@ int virRun(const char *const*argv, int *status) ATTRIBUTE_RETURN_CHECK; int virRunWithHook(const char *const*argv, virExecHook hook, void *data, int *status) ATTRIBUTE_RETURN_CHECK; +int virFork(pid_t *pid);
int virFileReadLimFD(int fd, int maxlen, char **buf) ATTRIBUTE_RETURN_CHECK;
Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On 02/18/2010 07:50 AM, Daniel P. Berrange wrote:
On Wed, Feb 17, 2010 at 02:29:27PM -0500, Laine Stump wrote:
virFork() contains bookkeeping that must be done any time a process forks. Currently this includes:
1) Call virLogLock() prior to fork() and virLogUnlock() just after, to avoid a deadlock if some other thread happens to hold that lock during the fork.
2) Reset the logging hooks and send all child process log messages to stderr.
3) Block all signals prior to fork(), then either a) reset the signal mask for the parent process, or b) clear the signal mask for the child process.
util.c, util.h: add virFork() function, based on what is currently done in __virExec(). --- src/util/util.c | 115 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ src/util/util.h | 1 + 2 files changed, 116 insertions(+), 0 deletions(-)
diff --git a/src/util/util.c b/src/util/util.c index cdab300..f508f6b 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -296,6 +296,121 @@ static int virClearCapabilities(void) } #endif
+ +/* virFork() - fork a new process while avoiding various race/deadlock conditions + + @pid - a pointer to a pid_t that will receive the return value from + fork() + + 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(1) after doing any additional error reporting. + + */ +int virFork(pid_t *pid) { + sigset_t oldmask, newmask; + struct sigaction sig_action; + int saved_errno, ret = -1; + + *pid = -1; + + /* + * Need to block signals now, so that child process can safely + * kill off caller's signal handlers without a race. + */ + sigfillset(&newmask); + if (pthread_sigmask(SIG_SETMASK,&newmask,&oldmask) != 0) { + saved_errno = errno; + virReportSystemError(errno, + "%s", _("cannot block signals")); + goto cleanup; + } + + /* Ensure we hold the logging lock, to protect child processes + * from deadlocking on another thread's inherited mutex state */ + virLogLock(); + + *pid = fork(); + saved_errno = errno; /* save for caller */ + + /* Unlock for both parent and child process */ + virLogUnlock(); + + if (*pid< 0) { + virReportSystemError(saved_errno, + "%s", _("cannot fork child process")); + goto cleanup; + }
Tiny bug here, in that we forget to unblock the parent's signals in this error path.
Yow! That actually seems potentially catastrophic to me - if fork() ever fails, libvirtd would ignore all signals until it was kill -9'd. My only defense is that I was replicating existing behavior ;-) Fixed patch coming up.
For __virExec() this should be a semantic NOP. virFileCreate() and virDirCreate() gain the code to reset the logging and properly deal with the signal handling race condition. --- src/util/util.c | 100 ++++++++++++++---------------------------------------- 1 files changed, 26 insertions(+), 74 deletions(-) diff --git a/src/util/util.c b/src/util/util.c index f508f6b..ae8c461 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -448,20 +448,6 @@ __virExec(const char *const*argv, int pipeerr[2] = {-1,-1}; int childout = -1; int childerr = -1; - int logprio; - sigset_t oldmask, newmask; - struct sigaction sig_action; - - /* - * Need to block signals now, so that child process can safely - * kill off caller's signal handlers without a race. - */ - sigfillset(&newmask); - if (pthread_sigmask(SIG_SETMASK, &newmask, &oldmask) != 0) { - virReportSystemError(errno, - "%s", _("cannot block signals")); - return -1; - } if ((null = open("/dev/null", O_RDONLY)) < 0) { virReportSystemError(errno, @@ -528,17 +514,9 @@ __virExec(const char *const*argv, childerr = null; } - /* Ensure we hold the logging lock, to protect child processes - * from deadlocking on another threads inheirited mutex state */ - virLogLock(); - pid = fork(); - - /* Unlock for both parent and child process */ - virLogUnlock(); + int forkRet = virFork(&pid); if (pid < 0) { - virReportSystemError(errno, - "%s", _("cannot fork child process")); goto cleanup; } @@ -553,12 +531,8 @@ __virExec(const char *const*argv, *errfd = pipeerr[0]; } - /* Restore our original signal mask now child is safely - running */ - if (pthread_sigmask(SIG_SETMASK, &oldmask, NULL) != 0) { - virReportSystemError(errno, - "%s", _("cannot unblock signals")); - return -1; + if (forkRet < 0) { + goto cleanup; } *retpid = pid; @@ -567,38 +541,10 @@ __virExec(const char *const*argv, /* child */ - /* Remove any error callback too, so errors in child now - get sent to stderr where they stand a fighting chance - of being seen / logged */ - virSetErrorFunc(NULL, NULL); - - /* Make sure any hook logging is sent to stderr, since virExec will - * close any unknown FDs (including logging handlers) before launching - * the new process */ - logprio = virLogGetDefaultPriority(); - virLogReset(); - virLogSetDefaultPriority(logprio); - - /* Clear out all signal handlers from parent so nothing - 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); - - /* 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) { - virReportSystemError(errno, - "%s", _("cannot unblock signals")); + if (forkRet < 0) { + /* The fork was sucessful, but after that there was an error + * in the child (which was already logged). + */ _exit(1); } @@ -1367,14 +1313,10 @@ int virFileCreate(const char *path, mode_t mode, * following dance avoids problems caused by root-squashing * NFS servers. */ - virLogLock(); - pid = fork(); - virLogUnlock(); + int forkRet = virFork(&pid); if (pid < 0) { ret = errno; - virReportSystemError(errno, - _("cannot fork o create file '%s'"), path); return ret; } @@ -1426,7 +1368,15 @@ parenterror: return ret; } - /* child - set desired uid/gid, then attempt to create the file */ + + /* child */ + + if (forkRet < 0) { + /* error encountered and logged in virFork() after the fork. */ + goto childerror; + } + + /* set desired uid/gid, then attempt to create the file */ if ((gid != 0) && (setgid(gid) != 0)) { ret = errno; @@ -1477,15 +1427,10 @@ int virDirCreate(const char *path, mode_t mode, return virDirCreateSimple(path, mode, uid, gid, flags); } - virLogLock(); - pid = fork(); - virLogUnlock(); + int forkRet = virFork(&pid); if (pid < 0) { ret = errno; - virReportSystemError(errno, - _("cannot fork to create directory '%s'"), - path); return ret; } @@ -1535,7 +1480,14 @@ parenterror: return ret; } - /* child - set desired uid/gid, then attempt to create the directory */ + /* 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 ((gid != 0) && (setgid(gid) != 0)) { ret = errno; -- 1.6.6.1
participants (2)
-
Daniel P. Berrange -
Laine Stump