[libvirt] [PATCH] daemon: Plug memory leaks

[Alex Jia]

On 04/12/2012 04:37 PM, Daniel P. Berrange wrote:
> On Thu, Apr 12, 2012 at 03:24:08PM +0800, Alex Jia wrote:
>> Detected by valgrind. Leaks are introduced in commit 6e6e9be.
>>
>> * daemon/libvirtd-config.c (macro GET_CONF_STR): fix memory leaks.
>>
>> How to reproduce?
>>
>> % make&&  make -C tests check TESTS=libvirtdconftest
>> % cd tests&&  valgrind -v --leak-check=full ./libvirtdconftest
>>
>> actual result:
>>
>> ==11008== 185 bytes in 5 blocks are definitely lost in loss record 3 of 5
>> ==11008==    at 0x4A05FDE: malloc (vg_replace_malloc.c:236)
>> ==11008==    by 0x39CF07F6E1: strdup (strdup.c:43)
>> ==11008==    by 0x406626: daemonConfigLoadOptions (libvirtd-config.c:438)
>> ==11008==    by 0x406800: daemonConfigLoadData (libvirtd-config.c:492)
>> ==11008==    by 0x403CCF: testCorrupt (libvirtdconftest.c:110)
>> ==11008==    by 0x404FAD: virtTestRun (testutils.c:145)
>> ==11008==    by 0x403A34: mymain (libvirtdconftest.c:219)
>> ==11008==    by 0x404687: virtTestMain (testutils.c:700)
>> ==11008==    by 0x39CF01ECDC: (below main) (libc-start.c:226)
>> ==11008==
>> ==11008== LEAK SUMMARY:
>> ==11008==    definitely lost: 185 bytes in 5 blocks
>>
>> Signed-off-by: Alex Jia<ajia at redhat.com>
>> ---
>>   daemon/libvirtd-config.c |    1 +
>>   1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/daemon/libvirtd-config.c b/daemon/libvirtd-config.c
>> index 4d041f0..0a4323a 100644
>> --- a/daemon/libvirtd-config.c
>> +++ b/daemon/libvirtd-config.c
>> @@ -152,6 +152,7 @@ checkType (virConfValuePtr p, const char *filename,
>>                   virReportOOMError();                                    \
>>                   goto error;                                             \
>>               }                                                           \
>> +            VIR_FREE(data->var_name);                                   \
>>           }                                                               \
>>       } while (0)
> Errr, no. Please look at the context of what's going on, rather than
> just blindly adding code to free a block at the point where valgrind
> shows the allocation.
>
> The full context to this diff
>
> #define GET_CONF_STR(conf, filename, var_name)                          \
>      do {                                                                \
>          virConfValuePtr p = virConfGetValue (conf, #var_name);          \
>          if (p) {                                                        \
>              if (checkType (p, filename, #var_name, VIR_CONF_STRING)<  0) \
>                  goto error;                                             \
>              VIR_FREE(data->var_name);                                   \
>              if (!(data->var_name = strdup (p->str))) {                  \
>                  virReportOOMError();                                    \
>                  goto error;                                             \
>              }                                                           \
>          }                                                               \
>      } while (0)
>
>
> Your suggested change means we would be free'ing the config file before
> we have even used it.
>
> If you look at this line:
Yeah.
>> ==11008==    by 0x406626: daemonConfigLoadOptions (libvirtd-config.c:438)
> You'll see it matches:
>
>      GET_CONF_STR (conf, filename, host_uuid);
>
> There are no leak reports for other uses of GET_CONF_STR, so this suggests
> that 'host_uuid' is not being freed. Check daemonConfigFree and you'll see
> this is indeed missing
Yeah, it's a little weird, the previous codes also haven't free 
'host_uuid', but valgrind hasn't complains memory leak.

Thanks for your review,
Alex
> Daniel