Re: [spice-gtk] cache: Fix OOM due bad refcount of image

[Marc-André Lureau <marcandre.lureau@xxxxxxxxx>]

Hi

On Fri, Jul 20, 2018 at 11:56 AM, Victor Toso <victortoso@xxxxxxxxxx> wrote:
> From: 乐义华 <yueyihua@xxxxxxxxxxx>
>
> The image in cache was not being freed properly leading the client to
> crash with OOM after sometime.
>
> When image flag is SPICE_IMAGE_FLAGS_CACHE_REPLACE_ME, and if found it
> in hash table, we will increase the refcount but this image will only
> be destroyed, so the cache will usage more and more memory. In this
> circumstance, we should not increase refcount.
>
> Fixes: https://gitlab.freedesktop.org/spice/spice-gtk/issues/72
>
> Signed-off-by: 乐义华 <yueyihua@xxxxxxxxxxx>
> Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> ---
>  src/channel-display.c     |  5 ++++-
>  src/spice-channel-cache.h | 17 +++++++++++++++++
>  2 files changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/src/channel-display.c b/src/channel-display.c
> index 54f40ba..7c663cb 100644
> --- a/src/channel-display.c
> +++ b/src/channel-display.c
> @@ -788,7 +788,10 @@ static void image_put_lossy(SpiceImageCache *cache, uint64_t id,
>  static void image_replace_lossy(SpiceImageCache *cache, uint64_t id,
>                                  pixman_image_t *surface)
>  {
> -    image_put(cache, id, surface);
> +    SpiceDisplayChannelPrivate *c =
> +        SPICE_CONTAINEROF(cache, SpiceDisplayChannelPrivate, image_cache);
> +
> +    cache_replace_lossy(c->images, id, pixman_image_ref(surface), FALSE);
>  }
>
>  static pixman_image_t* image_get_lossless(SpiceImageCache *cache, uint64_t id)
> diff --git a/src/spice-channel-cache.h b/src/spice-channel-cache.h
> index 75cc2cd..fc25c35 100644
> --- a/src/spice-channel-cache.h
> +++ b/src/spice-channel-cache.h
> @@ -101,6 +101,23 @@ static inline void cache_add_lossy(display_cache *cache, uint64_t id,
>      g_hash_table_replace(cache->table, item, value);
>  }
>
> +static inline void cache_replace_lossy(display_cache *cache, uint64_t id,
> +                                       gpointer value, gboolean lossy)
> +{
> +    display_cache_item *item = cache_item_new(id, lossy);
> +
> +    // If image is currently in the table consider its reference count before replacing it
> +    if (cache->ref_counted) {
> +        display_cache_item *current_item;
> +        gpointer current_image;
> +        if (g_hash_table_lookup_extended(cache->table, &id, (gpointer*) &current_item,
> +                                         &current_image)) {
> +            item->ref_count = current_item->ref_count;
> +        }
> +    }
> +    g_hash_table_replace(cache->table, item, value);
> +}

Given the similarity with cache_add_lossy(), I wonder if the code
could be factored out in a common function.

looks good otherwise
ack

> +
>  static inline void cache_add(display_cache *cache, uint64_t id, gpointer value)
>  {
>      cache_add_lossy(cache, id, value, FALSE);
> --
> 2.17.1
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel



-- 
Marc-André Lureau
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel