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

[Peter Krempa]

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.

NACK

Peter