[PATCHv3 3/3] lxc: fix a memory leak

[John Ferlan]

Coverity reminds us of the ancient software engineering proverb related
to being stuck with ownership because you touched the code last :-) - I
know this patch didn't cause the problem, but because the code was
touched Coverity decided to look harder and found another leak.

On 12/16/20 1:01 AM, Shi Lei wrote:
> In virLXCProcessSetupInterfaceTap, containerVeth needs to be freed on
> failure.
> 
> Signed-off-by: Shi Lei <shi_lei at massclouds.com>
> ---
>  src/lxc/lxc_process.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
> index 85d0287a..0f7c9295 100644
> --- a/src/lxc/lxc_process.c
> +++ b/src/lxc/lxc_process.c
> @@ -303,7 +303,7 @@ virLXCProcessSetupInterfaceTap(virDomainDefPtr vm,
>                                 const char *brname)
>  {
>      char *parentVeth;

Coverity complains that @parentVeth is leaked too - although it's far
more opaque and ornery.

Let's assume on input that net->ifname != NULL - that means that in
virNetDevVethCreate the call to virNetDevGenerateName and the memory in
@**ifname (e.g. @parentVeth's copy of net->ifname) will be g_free()'d
when !virNetDevExists(try) succeeds and @*ifname gets the memory from
@try. In this case @try is a g_strdup_printf of @*ifname. So this code
essentially free's memory pointed to by net->ifname, but since
@parentVeth is used in the call net->ifname is *NOT* updated which is a
second problem that Coverity did not note.

Then let's say the call to virNetDevGenerateName fails for @veth2...
Since on input @orig1 != NULL (e.g. @parentVeth != NULL), we will not
VIR_FREE(*veth1); - that's fine given the original assumption, but when
we return to virLXCProcessSetupInterfaceTap that means net->ifname will
point at memory that was g_free'd and @parentVeth will not be free'd,
thus Coverity squawks loud and proud about the resource leak - although
it doesn't complain about the fact that net->ifname now points to memory
that was free'd.

As an aside, I see no way on input @*veth2 could not be NULL so in the
cleanup path the check for @orig2 would seem to be dead code. Although,
sure future changes could alter that reality.

As a test if I replace all @parentVeth refs w/ net->ifname - then
Coverity is happy again. I will leave it up to Laine or Shi Lei to
generate the "real fix" and/or validate that my reading of the logic is
right or not ;-).

John

> -    char *containerVeth = NULL;
> +    g_autofree char *containerVeth = NULL;
>      const virNetDevVPortProfile *vport = virDomainNetGetActualVirtPortProfile(net);
>  
>      VIR_DEBUG("calling vethCreate()");
> @@ -357,7 +357,7 @@ virLXCProcessSetupInterfaceTap(virDomainDefPtr vm,
>          virDomainConfNWFilterInstantiate(vm->name, vm->uuid, net, false) < 0)
>          return NULL;
>  
> -    return containerVeth;
> +    return g_steal_pointer(&containerVeth);
>  }
>  
>  
>