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

[Daniel Veillard]

On Wed, Mar 09, 2011 at 02:20:37PM +0100, Jiri Denemark wrote:
> 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;

I think we should emit an error message to indicate why it failed !

>                  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)

  Same thing an error message must be provided

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

  I would really prefer those to be added if needed after the initial
  commit,

    thanks,

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/