[PATCH 2/7] util: Resolve resource leak in virExec error path

John Ferlan posted 7 patches 5 years, 2 months ago
[PATCH 2/7] util: Resolve resource leak in virExec error path
Posted by John Ferlan 5 years, 2 months ago
On error, the @pidfilefd was not released

Found by Coverity

Signed-off-by: John Ferlan <jferlan@redhat.com>
---
 src/util/vircommand.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/util/vircommand.c b/src/util/vircommand.c
index e47dd6b932..1716225aeb 100644
--- a/src/util/vircommand.c
+++ b/src/util/vircommand.c
@@ -792,12 +792,14 @@ virExec(virCommandPtr cmd)
         if (virSetInherit(pidfilefd, true) < 0) {
             virReportSystemError(errno, "%s",
                                  _("Cannot disable close-on-exec flag"));
+            virPidFileReleasePath(cmd->pidfile, pidfilefd);
             goto fork_error;
         }
 
         c = '1';
         if (safewrite(pipesync[1], &c, sizeof(c)) != sizeof(c)) {
             virReportSystemError(errno, "%s", _("Unable to notify child process"));
+            virPidFileReleasePath(cmd->pidfile, pidfilefd);
             goto fork_error;
         }
         VIR_FORCE_CLOSE(pipesync[0]);
-- 
2.28.0

Re: [PATCH 2/7] util: Resolve resource leak in virExec error path
Posted by Ján Tomko 5 years, 2 months ago
On a Wednesday in 2020, John Ferlan wrote:
>On error, the @pidfilefd was not released
>
>Found by Coverity
>

The pidfile code was introduced by:
commit d146105f1e4a9e0ab179f0b78c070ea38b9d5334
Author:     Michal Prívozník <mprivozn@redhat.com>
CommitDate: 2020-03-24 15:44:23 +0100

     virCommand: Actually acquire pidfile instead of just writing it

further changed by:
commit be00118d5d9a3afb41e0edcddec823dff63a7ae1
Author:     Marc-André Lureau <marcandre.lureau@redhat.com>
CommitDate: 2020-03-25 09:04:49 +0100
     util: keep the pidfile locked

then some code movement added more error paths after the pidfile code:
commit 0bb796bda31103ebf54eefc11c471586c87b95fd
Author:     Daniel Henrique Barboza <danielhb413@gmail.com>
CommitDate: 2020-10-02 14:32:57 -0300

     vircommand.c: write child pidfile before process tuning in virExec()


IIUC the point of leaking the pidfilefd is to make sure the child holds a
lock on the pidfile - so strictly speaking we should close it in all
code paths that don't end up in a successful execv. Practically,
this already happens because the fork_error calls _exit.

Jano

>Signed-off-by: John Ferlan <jferlan@redhat.com>
>---
> src/util/vircommand.c | 2 ++
> 1 file changed, 2 insertions(+)
>
>diff --git a/src/util/vircommand.c b/src/util/vircommand.c
>index e47dd6b932..1716225aeb 100644
>--- a/src/util/vircommand.c
>+++ b/src/util/vircommand.c
>@@ -792,12 +792,14 @@ virExec(virCommandPtr cmd)
>         if (virSetInherit(pidfilefd, true) < 0) {
>             virReportSystemError(errno, "%s",
>                                  _("Cannot disable close-on-exec flag"));
>+            virPidFileReleasePath(cmd->pidfile, pidfilefd);
>             goto fork_error;
>         }
>
>         c = '1';
>         if (safewrite(pipesync[1], &c, sizeof(c)) != sizeof(c)) {
>             virReportSystemError(errno, "%s", _("Unable to notify child process"));
>+            virPidFileReleasePath(cmd->pidfile, pidfilefd);
>             goto fork_error;
>         }
>         VIR_FORCE_CLOSE(pipesync[0]);
>-- 
>2.28.0
>