[libvirt PATCH 2/2] lxc: eliminate leaked and dangling pointers in virLXCProcessSetupInterfaceTap

[Laine Stump]

The two scenarios were found by Coverity after a seemingly-unrelated
change to virLXCProcessSetupInterfaceTap() (in commit ecfc2d5f43), and
explained by John Ferlan here:

https://www.redhat.com/archives/libvir-list/2020-December/msg00810.html

To re-explain:

a) On entry to virLXCProcessSetupInterfaceTap() if net->ifname != NULL
   then a copy of net->ifname is made into parentVeth, and a reference
   to *that* pointer is sent down to virNetDevVethCreate().

b) If parentVeth (aka net->ifname) is a template name (e.g. "blah%d"),
   then virNetDevVethCreate() calls virNetDevGenerateName(), and if
   virNetDevGenerateName() successfully generates a usable name
   (e.g. "blah27") then it will free the original template string
   (which is pointed to by net->ifname and by parentVeth), then
   replace the pointer in parentVeth with a pointer to the new
   string. Note that net->ifname still points to the now-freed
   template string.

c) returning back up to virLXCProcessSetupInterfaceTap(), we check if
   net->ifname == NULL - it *isn't* (still contains stale pointer to
   template string), so we don't replace it with the pointer to the new
   string that is in parentVeth.

d) Result: the new string is leaked once we return from
   virLXCProcessSetupInterfaceTap(), while there is a dangling pointer
   to the old string in net->ifname.

There is also a leak if there is a failure somewhere between steps (b)
and (c) above - the failure cleanup in virNetDevVethCreate() will only
free the newly-generated parentVeth string if the original pointer was
NULL (narrator: "It wasn't."). But it's a new string allocated by
virNetDevGenerateName(), not the original string from net->ifname, so
it really does need to be freed.

The solution is to make a copy of the entire original string into a
g_autofree pointer, then iff everything is successful we g_free() the
original net->ifname and replace it by stealing the string returned by
virNetDevVethCreate().

Signed-off-by: Laine Stump <laine at redhat.com>
---
 src/lxc/lxc_process.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
index f6932c248b..2266e2cc40 100644
--- a/src/lxc/lxc_process.c
+++ b/src/lxc/lxc_process.c
@@ -302,20 +302,17 @@ virLXCProcessSetupInterfaceTap(virDomainDefPtr vm,
                                virDomainNetDefPtr net,
                                const char *brname)
 {
-    char *parentVeth;
+    g_autofree char *parentVeth = NULL;
     g_autofree char *containerVeth = NULL;
     const virNetDevVPortProfile *vport = virDomainNetGetActualVirtPortProfile(net);
 
     VIR_DEBUG("calling vethCreate()");
-    parentVeth = net->ifname;
+    parentVeth = g_strdup(net->ifname);
 
     if (virNetDevVethCreate(&parentVeth, &containerVeth) < 0)
         return NULL;
     VIR_DEBUG("parentVeth: %s, containerVeth: %s", parentVeth, containerVeth);
 
-    if (net->ifname == NULL)
-        net->ifname = parentVeth;
-
     if (virNetDevSetMAC(containerVeth, &net->mac) < 0)
         return NULL;
 
@@ -355,6 +352,10 @@ virLXCProcessSetupInterfaceTap(virDomainDefPtr vm,
         virDomainConfNWFilterInstantiate(vm->name, vm->uuid, net, false) < 0)
         return NULL;
 
+    /* success is guaranteed, so update the interface object */
+    g_free(net->ifname);
+    net->ifname = g_steal_pointer(&parentVeth);
+
     return g_steal_pointer(&containerVeth);
 }
 
-- 
2.29.2