Re: [libvirt] [PATCH] util: Forbid calling hash APIs from iterator callback

Calling most hash APIs is not safe from inside of an iterator callback. Exceptions are APIs that do not modify the hash table and removing current hash entry from virHashFroEach callback. This patch make all APIs which are not safe fail instead of just relying on the callback being nice not calling any unsafe APIs. --- src/util/hash.c | 39 +++++++++++++++++++++++++++++++-------- 1 files changed, 31 insertions(+), 8 deletions(-) diff --git a/src/util/hash.c b/src/util/hash.c index 2a9a9cf..1a300f6 100644 --- a/src/util/hash.c +++ b/src/util/hash.c @@ -54,6 +54,10 @@ struct _virHashTable { struct _virHashEntry *table; int size; int nbElems; + /* True iff we are iterating over hash entries. */ + bool iterating; + /* Pointer to the current entry during iteration. */ + virHashEntryPtr current; virHashDataFree dataFree; virHashKeyCode keyCode; virHashKeyEqual keyEqual; @@ -313,7 +317,7 @@ virHashAddOrUpdateEntry(virHashTablePtr table, const void *name, char *new_name; bool found; - if ((table == NULL) || (name == NULL)) + if ((table == NULL) || (name == NULL) || table->iterating) return (-1); /* @@ -513,6 +517,8 @@ virHashRemoveEntry(virHashTablePtr table, const void *name) for (entry = &(table->table[key]); entry != NULL; entry = entry->next) { if (table->keyEqual(entry->name, name)) { + if (table->iterating && table->current != entry) + return -1;

if (table->dataFree && (entry->payload != NULL)) table->dataFree(entry->payload, entry->name); entry->payload = NULL; @@ -548,28 +554,35 @@ virHashRemoveEntry(virHashTablePtr table, const void *name) * @data: opaque data to pass to the iterator * * Iterates over every element in the hash table, invoking the - * 'iter' callback. The callback is allowed to remove the element using - * virHashRemoveEntry but calling other virHash* functions is prohibited. + * 'iter' callback. The callback is allowed to remove the current element + * using virHashRemoveEntry but calling other virHash* functions is prohibited. * * Returns number of items iterated over upon completion, -1 on failure */ -int virHashForEach(virHashTablePtr table, virHashIterator iter, void *data) { +int virHashForEach(virHashTablePtr table, virHashIterator iter, void *data) +{ int i, count = 0; - if (table == NULL || iter == NULL) + if (table == NULL || iter == NULL || table->iterating) return (-1); + table->iterating = true; + table->current = NULL; for (i = 0 ; i < table->size ; i++) { virHashEntryPtr entry = table->table + i; while (entry) { virHashEntryPtr next = entry->next; if (entry->valid) { + table->current = entry; iter(entry->payload, entry->name, data); + table->current = NULL; count++; } entry = next; } } + table->iterating = false; + return (count); } @@ -590,9 +603,11 @@ int virHashForEach(virHashTablePtr table, virHashIterator iter, void *data) { int virHashRemoveSet(virHashTablePtr table, virHashSearcher iter, const void *data) { int i, count = 0; - if (table == NULL || iter == NULL) + if (table == NULL || iter == NULL || table->iterating) return (-1); + table->iterating = true; + table->current = NULL; for (i = 0 ; i < table->size ; i++) { virHashEntryPtr prev = NULL; virHashEntryPtr entry = &(table->table[i]); @@ -629,6 +644,8 @@ int virHashRemoveSet(virHashTablePtr table, virHashSearcher iter, const void *da } } } + table->iterating = false; + return (count); } @@ -646,18 +663,24 @@ int virHashRemoveSet(virHashTablePtr table, virHashSearcher iter, const void *da void *virHashSearch(virHashTablePtr table, virHashSearcher iter, const void *data) { int i; - if (table == NULL || iter == NULL) + if (table == NULL || iter == NULL || table->iterating)

return (NULL); + table->iterating = true; + table->current = NULL; for (i = 0 ; i < table->size ; i++) { virHashEntryPtr entry = table->table + i; while (entry) { if (entry->valid) { - if (iter(entry->payload, entry->name, data)) + if (iter(entry->payload, entry->name, data)) { + table->iterating = false; return entry->payload; + } } entry = entry->next; } } + table->iterating = false; + return (NULL); }