[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

[libvirt] virStoragePoolSourceFree(S) does not free S



I was surprised to see that virStoragePoolSourceFree(S) does not free S.
The other three vir*Free functions in storage_conf *do* free S.
There are at least three places where this causes leaks.
Here's one:

    virStoragePoolSourcePtr
    virStoragePoolDefParseSourceString(virConnectPtr conn,
                                       const char *srcSpec,
                                       int pool_type)
    {
        xmlDocPtr doc = NULL;
        xmlNodePtr node = NULL;
        xmlXPathContextPtr xpath_ctxt = NULL;
        virStoragePoolSourcePtr def = NULL, ret = NULL;

        doc = xmlReadDoc((const xmlChar *)srcSpec, "srcSpec.xml", NULL,
                         XML_PARSE_NOENT | XML_PARSE_NONET |
                         XML_PARSE_NOERROR | XML_PARSE_NOWARNING);

        if (doc == NULL) {
            virStorageReportError(conn, VIR_ERR_XML_ERROR,
                                  "%s", _("bad <source> spec"));
            goto cleanup;
        }

        xpath_ctxt = xmlXPathNewContext(doc);
        if (xpath_ctxt == NULL) {
            virReportOOMError(conn);
            goto cleanup;
        }

        if (VIR_ALLOC(def) < 0) {               <<<====== def is alloc'd
            virReportOOMError(conn);
            goto cleanup;
        }

        node = virXPathNode(conn, "/source", xpath_ctxt);
        if (!node) {
            virStorageReportError(conn, VIR_ERR_XML_ERROR,
                                  "%s", _("root element was not source"));
            goto cleanup;
        }

        if (virStoragePoolDefParseSource(conn, xpath_ctxt, def, pool_type,
                                         node) < 0)
            goto cleanup;

        ret = def;
        def = NULL;
    cleanup:
        if (def)
            virStoragePoolSourceFree(def);    <<<<======== yet not freed
        xmlFreeDoc(doc);
        xmlXPathFreeContext(xpath_ctxt);

        return ret;
    }

One fix might be to call VIR_FREE(def) in the "if (def)..."
block, but that seems short-sighted, since the name
virStoragePoolSourceFree makes me think *it* should be
doing the whole job.

However, if I make the logical change to virStoragePoolSourceFree,

diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 62b8394..ffe38cc 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -291,6 +291,7 @@ virStoragePoolSourceFree(virStoragePoolSourcePtr source) {
         VIR_FREE(source->auth.chap.login);
         VIR_FREE(source->auth.chap.passwd);
     }
+    VIR_FREE(source);
 }

that causes "make check" to fail miserably due to heap corruption,
as reported by valgrind:

==30311== Invalid free() / delete / delete[]
==30311==    at 0x4A04D72: free (vg_replace_malloc.c:325)
==30311==    by 0x42950C: virFree (memory.c:177)
==30311==    by 0x4A2687: virStoragePoolSourceFree (storage_conf.c:294)
==30311==    by 0x4A26BD: virStoragePoolDefFree (storage_conf.c:304)
==30311==    by 0x4A2723: virStoragePoolObjFree (storage_conf.c:319)
==30311==    by 0x4A27A2: virStoragePoolObjListFree (storage_conf.c:334)
==30311==    by 0x4757AD: storageDriverShutdown (storage_driver.c:246)
==30311==    by 0x4C1E3F: virStateCleanup (libvirt.c:909)
==30311==    by 0x4103C5: qemudCleanup (libvirtd.c:2402)
==30311==    by 0x41273B: main (libvirtd.c:3196)
==30311==  Address 0x5c48c38 is 56 bytes inside a block of size 184 alloc'd
==30311==    at 0x4A04481: calloc (vg_replace_malloc.c:418)
==30311==    by 0x4293F5: virAlloc (memory.c:100)
==30311==    by 0x4A330E: virStoragePoolDefParseXML (storage_conf.c:615)
==30311==    by 0x4A3A21: virStoragePoolDefParseNode (storage_conf.c:762)
==30311==    by 0x4A3BE6: virStoragePoolDefParse (storage_conf.c:810)
==30311==    by 0x4A3C7F: virStoragePoolDefParseFile (storage_conf.c:834)
==30311==    by 0x4A5816: virStoragePoolObjLoad (storage_conf.c:1495)
==30311==    by 0x4A5BDB: virStoragePoolLoadAllConfigs (storage_conf.c:1575)
==30311==    by 0x475598: storageDriverStartup (storage_driver.c:164)
==30311==    by 0x4C1D9D: virStateInitialize (libvirt.c:888)
==30311==    by 0x4125E9: main (libvirtd.c:3153)

I tracked the problem down to this definition in src/conf/storage_conf.h:

    typedef struct _virStoragePoolDef virStoragePoolDef;
    typedef virStoragePoolDef *virStoragePoolDefPtr;
    struct _virStoragePoolDef {
        /* General metadata */
        char *name;
        unsigned char uuid[VIR_UUID_BUFLEN];
        int type; /* virStoragePoolType */

        unsigned long long allocation;
        unsigned long long capacity;
        unsigned long long available;

        virStoragePoolSource source;   <== this is a *STRUCT*, not a ptr
        virStoragePoolTarget target;   <== Likewise
    };

Hence, when calling virStoragePoolDefFree (defined like this)

    void
    virStoragePoolDefFree(virStoragePoolDefPtr def) {
        if (!def)
            return;

        VIR_FREE(def->name);

        virStoragePoolSourceFree(&def->source);

That function must *not* call VIR_FREE on &def->source,
because as valgrind tells us, above, that is in the middle
of a virStoragePoolDef buffer.

Thus, the naive patch seems to be the way to go,
unless you want to change the struct members to be pointers
(and adjust all uses -- much more invasive).
There are other leaks associated with similar uses of
virStoragePoolSourceFree just like this one.
Here's a patch that fixes them all at once.
If you agree in principle, I'll write a longer commit log
than the one below:

[note that there's no need to guard uses of
virStoragePoolSourceFree against NULL args, so I removed those. ]

>From a53efcee7ec74948a2cf4c1c02902419856b0154 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering redhat com>
Date: Fri, 5 Feb 2010 17:09:43 +0100
Subject: [PATCH] plug four virStoragePoolSourceFree-related leaks

* src/conf/storage_conf.c (virStoragePoolDefParseSourceString):
* src/storage/storage_backend_fs.c:
(virStorageBackendFileSystemNetFindPoolSourcesFunc):
(virStorageBackendFileSystemNetFindPoolSources):
* src/test/test_driver.c (testStorageFindPoolSources):
---
 src/conf/storage_conf.c          |    4 ++--
 src/storage/storage_backend_fs.c |    8 ++++----
 src/test/test_driver.c           |    3 ++-
 3 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 62b8394..b98b8e9 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -524,8 +524,8 @@ virStoragePoolDefParseSourceString(virConnectPtr conn,
     ret = def;
     def = NULL;
 cleanup:
-    if (def)
-        virStoragePoolSourceFree(def);
+    virStoragePoolSourceFree(def);
+    VIR_FREE(def);
     xmlFreeDoc(doc);
     xmlXPathFreeContext(xpath_ctxt);

diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
index 0d1c7a7..70e91a0 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -174,8 +174,8 @@ virStorageBackendFileSystemNetFindPoolSourcesFunc(virConnectPtr conn,
     src = NULL;
     ret = 0;
 cleanup:
-    if (src)
-        virStoragePoolSourceFree(src);
+    virStoragePoolSourceFree(src);
+    VIR_FREE(src);
     return ret;
 }

@@ -236,8 +236,8 @@ virStorageBackendFileSystemNetFindPoolSources(virConnectPtr conn,
     for (i = 0; i < state.list.nsources; i++)
         virStoragePoolSourceFree(&state.list.sources[i]);

-    if (source)
-        virStoragePoolSourceFree(source);
+    virStoragePoolSourceFree(source);
+    VIR_FREE(source);

     VIR_FREE(state.list.sources);

diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 2122a1b..8e5ecf2 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -1,7 +1,7 @@
 /*
  * test.c: A "mock" hypervisor for use by application unit tests
  *
- * Copyright (C) 2006-2009 Red Hat, Inc.
+ * Copyright (C) 2006-2010 Red Hat, Inc.
  * Copyright (C) 2006 Daniel P. Berrange
  *
  * This library is free software; you can redistribute it and/or
@@ -3763,6 +3763,7 @@ testStorageFindPoolSources(virConnectPtr conn,

 cleanup:
     virStoragePoolSourceFree(source);
+    VIR_FREE(source);
     return ret;
 }

--
1.7.0.rc1.204.gb96e


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]