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

[Laine Stump]

On 12/18/20 10:45 PM, Shi Lei wrote:
> On 2020-12-18 at 22:01, John Ferlan wrote:
>
>> 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
> Thanks! :-)
>
> As far as I can see, if the original net->ifname != NULL, it should be a user-provided
> name and NOT a template (prefix+'%d'). When @parentVeth (as the copy of net->ifname)
> is passed into virNetDevGenerateName, this function will leave it unchanged because it is
> not a template.
>
> So for now these problems will not happen. But for future, I think it's necessary to fix
> virLXCProcessSetupInterfaceTap.
>
> I agree that it's a right way to replace all *parentVeth* with net->ifname in
> virLXCProcessSetupInterfaceTap. And the *parentVeth* should be removed.


I tried something a bit different:

  https://www.redhat.com/archives/libvir-list/2021-January/msg00311.html