Re: [libvirt] [PATCH] daemon: fix fd leak on libvirtd

On 12/22/2011 01:28 AM, ajia(a)redhat.com wrote: s/decriptor/descriptor/ s/doesen't/doesn't/ fd's leaked at exit are sometimes the sign of a more serious bug lasting through the life of a program, but in this particular case, I'm not convinced. First, let's get a better picture of daemonForkIntoBackground. On success, this function calls fork() twice, meaning we are dealing with three processes: parent - fork intermediate child, wait for grandchild to exist, then exit intermediate child - prepare to daemonize, fork grandchild, then exit grandchild - now running as the daemon - return the fd On error cases, we must ensure that _exactly one_ process returns -1 to the caller, so that the caller can report the failure. However, note that this can be _any one_ of the three processes; with the implicit understanding that if we ever get past a successful fork(), then the process that does not return must exit without any further user-visible actions (not even an error message). That is, anywhere that we fail before fork(), the parent is responsible for returning an error, but anywhere after a fork(), the parent must call _exit(). This one's wrong. statuspipe is uninitialized, which means going to error here would risk closing an arbitrary fd. This one's wrong. It should go to cleanup, otherwise we leak stdinfd and stdoutfd. See below about _exit... This one's good. This one's iffy both before and after your change - virPidWait returns -1 if the child had non-zero status, but in the child, we returned -1 instead of calling _exit(1), which means we end up with both sides of fork() returning to the caller, and thus double error output. The solution is to fix the child to exit with non-zero status, and transfer the error reporting burden to the parent in that case. This one's pointless to all but valgrind, since _exit() will close it anyway (that is, we're leaking just before exit, but big deal - we know we're exiting; leaks are only bad if you keep on executing for a long time afterwards). But since cleaning it up makes valgrind analysis easier, I'm not opposed to it; however, that means we should also avoid the fd leak before the intermediate child calls _exit. Ouch - we have an independent bug here. We did fprintf(), but not fflush(), and since we call _exit() instead of exit(), the error never gets printed! I'm proposing this as a more-complete v2. In the new control flow, we have the following possible paths: parent errors out before fork() -> return -1 with no leaks otherwise the parent fork()s, and: child errors out before fork() -> child calls _exit(1) and parent returns -1 child successfully calls fork() -> child calls _exit(0), grandchild returns fd, and: caller in grandchild detects error, so grandchild writes failure message to pipe before calling _exit, and parent calls exit(1) caller in grandchild writes success to pipe then continues, and parent calls exit(0) Oh, and while I was at it, I made things slightly more robust if you invoke libvirtd with stdin or stdout closed (a rare event, but easy enough to work around); we don't want to close our just-created stdinfd handed to the grandchild just because we happened to open("/dev/null") and get back a standard descriptor. v2 to follow. -- Eric Blake eblake(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org