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; }