[libvirt PATCH 2/2] lxc: eliminate leaked and dangling pointers in virLXCProcessSetupInterfaceTap
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(a)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