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. -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list