Re: [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<a...@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list