[Libosinfo] [PATCH 6/8] list: Fix replacement in osinfo_list_add

[Daniel P. Berrange]

On Thu, Nov 05, 2015 at 05:20:44PM +0100, Christophe Fergeau wrote:
> OsinfoList maintains a mapping between a const char *entity_id and an
> OsinfoEntity object. It does so by using both a GPtrArray and a
> GHashTable.
> 
> When an entity is added to an OsinfoList, a pointer from the
> OsinfoEntity is used as the key (through osinfo_entity_get_id()) and
> the OsinfoEntity is g_object_ref'ed and used as a value.
> The OsinfoEntity is also appended to the array mentioned previously.
> The use of g_hash_table_new_full() ensures that the OsinfoEntity will be
> unref'ed when it's removed from the hash table.
> 
> However, when trying to add an entity with the same id as one already
> present in the OsinfoList, several issues occur:
> - g_hash_table_insert() is used, which means the preexisting entity with
>   the same id is going to get removed, and thus g_object_unref'ed.
>   However, the _old_ key will still be used by the hash table, which
>   means that if the OsinfoList was holding the last reference to the
>   old OsinfoEntity, the key stored in the hash table is going to
>   point to freed memory
> - similarly, we make no attempt at removing the preexisting entity from
>   the GPtrArray, so this will also contain stale data
> 
> This commit makes sure we remove entities with the same id from the
> GPtrArray when this situation occurs. It uses g_hash_table_replace()
> rather than g_hash_table_insert() which replaces both the key and the
> value if the hash table already contains the same key.
> 
> This fixes some valgrind issues or crashes that occurred when running
> make check in a libosinfo tree with 2 databases, one in the old layout
> and one using the new layout.
> ---
>  osinfo/osinfo_list.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/osinfo/osinfo_list.c b/osinfo/osinfo_list.c
> index 6e32387..c829a62 100644
> --- a/osinfo/osinfo_list.c
> +++ b/osinfo/osinfo_list.c
> @@ -237,12 +237,17 @@ OsinfoEntity *osinfo_list_find_by_id(OsinfoList *list, const gchar *id)
>   */
>  void osinfo_list_add(OsinfoList *list, OsinfoEntity *entity)
>  {
> +    OsinfoEntity *preexisting;
>      g_return_if_fail(G_TYPE_CHECK_INSTANCE_TYPE(entity, list->priv->elementType));
>  
>      g_object_ref(entity);
> +    preexisting = osinfo_list_find_by_id(list, osinfo_entity_get_id(entity));
> +    if (preexisting != NULL) {
> +        g_ptr_array_remove(list->priv->array, preexisting);
> +    }
>      g_ptr_array_add(list->priv->array, entity);
> -    g_hash_table_insert(list->priv->entities,
> -                        (gchar *)osinfo_entity_get_id(entity), entity);
> +    g_hash_table_replace(list->priv->entities,
> +                         (gchar *)osinfo_entity_get_id(entity), entity);
>  }

This makes sense purely from the POV of correctness of the API, but
I think there is another bug hiding here somewhere, because we AFAIR
we should never get in a situation where we are replacing an existing
entity in the list. When loading entities from the XML, we always
check to see if the entity already exists, and if so re-use it rather
than creating a new one. IOW, while this fix is valid, fixing it is
masking what I think might be a more serious flaw elsewhere, that I
would see to identify.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|