External/libvirt
1
0
Fork 0
mirror of https://gitlab.com/libvirt/libvirt.git synced 2024-07-31 14:07:16 +00:00
Code Issues Packages Projects Releases Wiki Activity

lxc: eliminate leaked and dangling pointers in virLXCProcessSetupInterfaceTap

Browse Source 
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@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
This commit is contained in:
Laine Stump 2021-01-06 13:44:40 -05:00
parent 84617bf2f8
commit 2d0bac9d58
1 changed files with 6 additions and 5 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

11
src/lxc/lxc_process.c
View File

@ -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);
}