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> --- 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