[libvirt] [PATCH] conf: Fix memory leaks in virStoragePoolDefParseSource

[Alex Jia]

On 05/09/2012 04:06 PM, Peter Krempa wrote:
> On 05/09/2012 08:15 AM, Alex Jia wrote:
>> Detected by valgrind. Leaks are introduced in commit 122fa379.
>>
>> src/conf/storage_conf.c: fix memory leaks.
>>
>> How to reproduce?
>> $ make&&   make -C tests check TESTS=storagepoolxml2xmltest
>> $ cd tests&&   valgrind -v --leak-check=full ./storagepoolxml2xmltest
>>
>> actual result:
>> ==28571== LEAK SUMMARY:
>> ==28571==    definitely lost: 40 bytes in 5 blocks
>> ==28571==    indirectly lost: 0 bytes in 0 blocks
>> ==28571==      possibly lost: 0 bytes in 0 blocks
>> ==28571==    still reachable: 1,054 bytes in 21 blocks
>> ==28571==         suppressed: 0 bytes in 0 blocks
>>
>> Signed-off-by: Alex Jia<ajia at redhat.com>
>> ---
>>    src/conf/storage_conf.c |    1 +
>>    1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
>> index 0b34f28..668e679 100644
>> --- a/src/conf/storage_conf.c
>> +++ b/src/conf/storage_conf.c
>> @@ -462,6 +462,7 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt,
>>                        goto cleanup;
>>                    }
>>                }
>> +            VIR_FREE(nodeset);
>>            }
>>        }
>>
> The problem with the added line is not visible with this context, so I'm adding
> some more:
>
>       source->nhost = virXPathNodeSet("./host", ctxt,&nodeset);
>
>       if (source->nhost) {
>           if (VIR_ALLOC_N(source->hosts, source->nhost)<  0) {
>               virReportOOMError();
>               goto cleanup;
>           }
>
>           for (i = 0 ; i<  source->nhost ; i++) {
>               name = virXMLPropString(nodeset[i], "name");
>               if (name == NULL) {
>                   virStorageReportError(VIR_ERR_XML_ERROR,
>                           "%s", _("missing storage pool host name"));
>                   goto cleanup;
>               }
>               source->hosts[i].name = name;
>
>               port = virXMLPropString(nodeset[i], "port");
>               if (port) {
>                   if (virStrToLong_i(port, NULL, 10,&source->hosts[i].port)<  0) {
>                       virStorageReportError(VIR_ERR_XML_ERROR,
>                                             _("Invalid port number: %s"),
>                                             port);
>                       goto cleanup;
>                   }
>               }
> +            VIR_FREE(nodeset);
>           }
>       }
>
> You added the VIR_FREE inside the for loop, so it gets freed before the next iteration
> and might cause a segfault in the second iteration.
Although I haven't meet a segfault error, I think you're right, thanks.
> NACK
>
> Peter