[libvirt] [PATCH] hash: fix memory leak and regression

[Laine Stump]

On 04/29/2011 03:53 PM, Eric Blake wrote:
> Commit 1671d1d introduced a memory leak in virHashFree, and
> wholesale table corruption in virHashRemoveSet (elements not
> requested to be freed are lost).
>
> * src/util/hash.c (virHashFree): Free bucket array.
> (virHashRemoveSet): Don't lose elements.
> * tests/hashtest.c (testHashCheckForEachCount): New method.
> (testHashCheckCount): Expose the bug.
> ---
>   src/util/hash.c  |    6 +++---
>   tests/hashtest.c |   15 +++++++++++++++
>   2 files changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/src/util/hash.c b/src/util/hash.c
> index a9b09b0..5366dd6 100644
> --- a/src/util/hash.c
> +++ b/src/util/hash.c
> @@ -269,6 +269,7 @@ virHashFree(virHashTablePtr table)
>           }
>       }
>
> +    VIR_FREE(table->table);
>       VIR_FREE(table);
>   }
>
> @@ -532,13 +533,12 @@ int virHashForEach(virHashTablePtr table, virHashIterator iter, void *data)
>    * virHashRemoveSet
>    * @table: the hash table to process
>    * @iter: callback to identify elements for removal
> - * @f: callback to free memory from element payload
>    * @data: opaque data to pass to the iterator
>    *
>    * Iterates over all elements in the hash table, invoking the 'iter'
>    * callback. If the callback returns a non-zero value, the element
>    * will be removed from the hash table&  its payload passed to the
> - * callback 'f' for de-allocation.
> + * data freer callback registered at creation.
>    *
>    * Returns number of items removed on success, -1 on failure
>    */
> @@ -562,7 +562,7 @@ int virHashRemoveSet(virHashTablePtr table,
>           while (*nextptr) {
>               virHashEntryPtr entry = *nextptr;
>               if (!iter(entry->payload, entry->name, data)) {
> -                *nextptr = entry->next;
> +                nextptr =&entry->next;
>               } else {
>                   count++;
>                   if (table->dataFree)
> diff --git a/tests/hashtest.c b/tests/hashtest.c
> index 525ae06..f02b3a9 100644
> --- a/tests/hashtest.c
> +++ b/tests/hashtest.c
> @@ -61,16 +61,31 @@ testHashInit(int size)
>       return hash;
>   }
>
> +static void
> +testHashCheckForEachCount(void *payload ATTRIBUTE_UNUSED,
> +                          const void *name ATTRIBUTE_UNUSED,
> +                          void *data ATTRIBUTE_UNUSED)
> +{
> +}
>
>   static int
>   testHashCheckCount(virHashTablePtr hash, int count)
>   {
> +    int iter_count = 0;
> +
>       if (virHashSize(hash) != count) {
>           testError("\nhash contains %d instead of %d elements\n",
>                     virHashSize(hash), count);
>           return -1;
>       }
>
> +    iter_count = virHashForEach(hash, testHashCheckForEachCount, NULL);
> +    if (count != iter_count) {
> +        testError("\nhash claims to have %d elements but iteration finds %d\n",
> +                  count, iter_count);
> +        return -1;
> +    }
> +
>       return 0;
>   }

ACK.