util: keep the pidfile locked

Unfortunately, advisory record locking lose the lock if any fd refering
to the file is closed. There doesn't seem to be a way to preserve the
lock atomically. We could eventually retake the lock if low pidfilefd
is required.

This fixes processes being leaked, as they are not killed in
virPidFileForceCleanupPath() if the lock can be taken. Here also, we may
consider this is not good enough, as a process may leak by simply
closing the pidfilefd.

Fixes commit d146105f1e4a9e0ab179f0b78c070ea38b9d5334 ("virCommand:
Actually acquire pidfile instead of just writing it")

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

diff --git a/src/util/vircommand.c b/src/util/vircommand.c
index 77078d09fbb4ee8aa101259a7a80c0309d9d41c6..b84fb4094821532dd165d5bdd999e33d106a4968 100644 (file)
--- a/src/util/vircommand.c
+++ b/src/util/vircommand.c
@@ -797,8 +797,7 @@ virExec(virCommandPtr cmd)
         virProcessSetMaxCoreSize(0, cmd->maxCore) < 0)
         goto fork_error;
     if (cmd->pidfile) {
-        VIR_AUTOCLOSE pidfilefd = -1;
-        int newpidfilefd = -1;
+        int pidfilefd = -1;
         char c;
 
         pidfilefd = virPidFileAcquirePath(cmd->pidfile, false, getpid());
@@ -818,14 +817,7 @@ virExec(virCommandPtr cmd)
         VIR_FORCE_CLOSE(pipesync[0]);
         VIR_FORCE_CLOSE(pipesync[1]);
 
-        /* This is here only to move the pidfilefd
-         * to the lowest possible number. */
-        if ((newpidfilefd = dup(pidfilefd)) < 0) {
-            virReportSystemError(errno, "%s", _("Unable to dup FD"));
-            goto fork_error;
-        }
-
-        /* newpidfilefd is intentionally leaked. */
+        /* pidfilefd is intentionally leaked. */
     }
 
     if (cmd->hook) {

diff --git a/tests/commanddata/test4.log b/tests/commanddata/test4.log
index 5820f28307051bf3300115f3b343d29a5d05e729..24a37a7e9663c1f016db07ce8b21d0e0a951c3de 100644 (file)
--- a/tests/commanddata/test4.log
+++ b/tests/commanddata/test4.log
@@ -9,7 +9,7 @@ ENV:USER=test
 FD:0
 FD:1
 FD:2
-FD:3
+FD:5
 DAEMON:yes
 CWD:/
 UMASK:0022