From 6f7e2fca1030e38f3a82f09b30a4839923f17c66 Mon Sep 17 00:00:00 2001

From: Sandy Stutsman <sstutsma@redhat.com>

Date: Fri, 26 Jun 2015 11:59:13 -0400

Subject: [PATCH] Lock the pixmap image cache for the entire fill_bits call

Locking the individual calls that access the pixmap cache in fill_bits is

not adequately thread safe.  Often a windows guest with multiple monitors

will be sending the same image via different threads.  Both threads can

be in fill_bits at the same time making changes to the cache for the same

image.  This can result in images being deleted before all the client

channels are finished with them or with the same image being send multiple

times.  Here's what can happen with out the lock in fill_bits

On the server in red_worker.c:fill_bits

 Thread 1 calls pixmap_cache_hit for Image A and finds it isn't in cache

 Thread 2 calls pixmap_cache_hit for Image A and finds it isn't in cache

 Thread 1 adds Image 1 to pixmap_cache (1x)

 Thread 2 adds Image 1 to pixmap_cache (2x)

On the client

 Channel 1 adds Image A to image_cache (1x)

 Channel 2 replaces Image A in image_cache (1x)

On server

 Thread 1 sends Image A rendering commands

 Thread N removes Image A from pixmap_cache (image remains - 1x)

 Thread 2 sends Image A rendering commands

On client

 Channe1 renders from Image A

 Channel N removes Image a from image_cache (image is completely removed)

 Channel2 render command hangs waiting for Image A

---

 server/red_client_shared_cache.h | 24 ++++++++++++------------

 server/red_worker.c              | 23 +++++++++++++++--------

 2 files changed, 27 insertions(+), 20 deletions(-)

diff --git a/server/red_client_shared_cache.h b/server/red_client_shared_cache.h

index 821ee18..7feb28e 100644

--- a/server/red_client_shared_cache.h

+++ b/server/red_client_shared_cache.h

@@ -36,13 +36,12 @@

 #define CHANNEL_FROM_RCC(rcc) SPICE_CONTAINEROF((rcc)->channel, CHANNEL, common.base);

-static int FUNC_NAME(hit)(CACHE *cache, uint64_t id, int *lossy, DisplayChannelClient *dcc)

+static int FUNC_NAME(unlocked_hit)(CACHE *cache, uint64_t id, int *lossy, DisplayChannelClient *dcc)

 {

     NewCacheItem *item;

     uint64_t serial;

     serial = red_channel_client_get_message_serial(&dcc->common.base);

-    pthread_mutex_lock(&cache->lock);

     item = cache->hash_table[CACHE_HASH_KEY(id)];

     while (item) {

@@ -57,15 +56,22 @@ static int FUNC_NAME(hit)(CACHE *cache, uint64_t id, int *lossy, DisplayChannelC

         }

         item = item->next;

     }

-    pthread_mutex_unlock(&cache->lock);

     return !!item;

 }

-static int FUNC_NAME(set_lossy)(CACHE *cache, uint64_t id, int lossy)

+static int FUNC_NAME(hit)(CACHE *cache, uint64_t id, int *lossy, DisplayChannelClient *dcc)

 {

-    NewCacheItem *item;

+    int hit;

     pthread_mutex_lock(&cache->lock);

+    hit = FUNC_NAME(unlocked_hit)(cache,id,lossy, dcc);

+    pthread_mutex_unlock(&cache->lock);

+    return hit;

+}

+

+static int FUNC_NAME(unlocked_set_lossy)(CACHE *cache, uint64_t id, int lossy)

+{

+    NewCacheItem *item;

     item = cache->hash_table[CACHE_HASH_KEY(id)];

@@ -76,11 +82,10 @@ static int FUNC_NAME(set_lossy)(CACHE *cache, uint64_t id, int lossy)

         }

         item = item->next;

     }

-    pthread_mutex_unlock(&cache->lock);

     return !!item;

 }

-static int FUNC_NAME(add)(CACHE *cache, uint64_t id, uint32_t size, int lossy, DisplayChannelClient *dcc)

+static int FUNC_NAME(unlocked_add)(CACHE *cache, uint64_t id, uint32_t size, int lossy, DisplayChannelClient *dcc)

 {

     NewCacheItem *item;

     uint64_t serial;

@@ -91,15 +96,12 @@ static int FUNC_NAME(add)(CACHE *cache, uint64_t id, uint32_t size, int lossy, D

     item = spice_new(NewCacheItem, 1);

     serial = red_channel_client_get_message_serial(&dcc->common.base);

-    pthread_mutex_lock(&cache->lock);

-

     if (cache->generation != dcc->CACH_GENERATION) {

         if (!dcc->pending_pixmaps_sync) {

             red_channel_client_pipe_add_type(

                 &dcc->common.base, PIPE_ITEM_TYPE_PIXMAP_SYNC);

             dcc->pending_pixmaps_sync = TRUE;

         }

-        pthread_mutex_unlock(&cache->lock);

         free(item);

         return FALSE;

     }

@@ -112,7 +114,6 @@ static int FUNC_NAME(add)(CACHE *cache, uint64_t id, uint32_t size, int lossy, D

         if (!(tail = (NewCacheItem *)ring_get_tail(&cache->lru)) ||

                                                    tail->sync[dcc->common.id] == serial) {

             cache->available += size;

-            pthread_mutex_unlock(&cache->lock);

             free(item);

             return FALSE;

         }

@@ -144,7 +145,6 @@ static int FUNC_NAME(add)(CACHE *cache, uint64_t id, uint32_t size, int lossy, D

     memset(item->sync, 0, sizeof(item->sync));

     item->sync[dcc->common.id] = serial;

     cache->sync[dcc->common.id] = serial;

-    pthread_mutex_unlock(&cache->lock);

     return TRUE;

 }

diff --git a/server/red_worker.c b/server/red_worker.c

index 955cac2..93e3398 100644

--- a/server/red_worker.c

+++ b/server/red_worker.c

@@ -6750,9 +6750,9 @@ static inline void red_display_add_image_to_pixmap_cache(RedChannelClient *rcc,

     if ((image->descriptor.flags & SPICE_IMAGE_FLAGS_CACHE_ME)) {

         spice_assert(image->descriptor.width * image->descriptor.height > 0);

         if (!(io_image->descriptor.flags & SPICE_IMAGE_FLAGS_CACHE_REPLACE_ME)) {

-            if (pixmap_cache_add(dcc->pixmap_cache, image->descriptor.id,

-                                 image->descriptor.width * image->descriptor.height, is_lossy,

-                                 dcc)) {

+            if (pixmap_cache_unlocked_add(dcc->pixmap_cache, image->descriptor.id,

+                                          image->descriptor.width * image->descriptor.height, is_lossy,

+                                          dcc)) {

                 io_image->descriptor.flags |= SPICE_IMAGE_FLAGS_CACHE_ME;

                 dcc->send_data.pixmap_cache_items[dcc->send_data.num_pixmap_cache_items++] =

                                                                                image->descriptor.id;

@@ -6797,11 +6797,12 @@ static FillBitsType fill_bits(DisplayChannelClient *dcc, SpiceMarshaller *m,

     if (simage->descriptor.flags & SPICE_IMAGE_FLAGS_HIGH_BITS_SET) {

         image.descriptor.flags = SPICE_IMAGE_FLAGS_HIGH_BITS_SET;

     }

+    pthread_mutex_lock(&dcc->pixmap_cache->lock);

     if ((simage->descriptor.flags & SPICE_IMAGE_FLAGS_CACHE_ME)) {

         int lossy_cache_item;

-        if (pixmap_cache_hit(dcc->pixmap_cache, image.descriptor.id,

-                             &lossy_cache_item, dcc)) {

+        if (pixmap_cache_unlocked_hit(dcc->pixmap_cache, image.descriptor.id,

+                                      &lossy_cache_item, dcc)) {

             dcc->send_data.pixmap_cache_items[dcc->send_data.num_pixmap_cache_items++] =

                                                                                image.descriptor.id;

             if (can_lossy || !lossy_cache_item) {

@@ -6818,10 +6819,11 @@ static FillBitsType fill_bits(DisplayChannelClient *dcc, SpiceMarshaller *m,

                 spice_assert(bitmap_palette_out == NULL);

                 spice_assert(lzplt_palette_out == NULL);

                 stat_inc_counter(display_channel->cache_hits_counter, 1);

+                pthread_mutex_unlock(&dcc->pixmap_cache->lock);

                 return FILL_BITS_TYPE_CACHE;

             } else {

-                pixmap_cache_set_lossy(dcc->pixmap_cache, simage->descriptor.id,

-                                       FALSE);

+                pixmap_cache_unlocked_set_lossy(dcc->pixmap_cache, simage->descriptor.id,

+                                                FALSE);

                 image.descriptor.flags |= SPICE_IMAGE_FLAGS_CACHE_REPLACE_ME;

             }

         }

@@ -6835,6 +6837,7 @@ static FillBitsType fill_bits(DisplayChannelClient *dcc, SpiceMarshaller *m,

         surface_id = simage->u.surface.surface_id;

         if (!validate_surface(worker, surface_id)) {

             rendering_incorrect("SPICE_IMAGE_TYPE_SURFACE");

+            pthread_mutex_unlock(&dcc->pixmap_cache->lock);

             return FILL_BITS_TYPE_SURFACE;

         }

@@ -6849,6 +6852,7 @@ static FillBitsType fill_bits(DisplayChannelClient *dcc, SpiceMarshaller *m,

                              &bitmap_palette_out, &lzplt_palette_out);

         spice_assert(bitmap_palette_out == NULL);

         spice_assert(lzplt_palette_out == NULL);

+        pthread_mutex_unlock(&dcc->pixmap_cache->lock);

         return FILL_BITS_TYPE_SURFACE;

     }

     case SPICE_IMAGE_TYPE_BITMAP: {

@@ -6879,6 +6883,7 @@ static FillBitsType fill_bits(DisplayChannelClient *dcc, SpiceMarshaller *m,

             }

             spice_marshaller_add_ref_chunks(m, bitmap->data);

+            pthread_mutex_unlock(&dcc->pixmap_cache->lock);

             return FILL_BITS_TYPE_BITMAP;

         } else {

             red_display_add_image_to_pixmap_cache(rcc, simage, &image,

@@ -6896,6 +6901,7 @@ static FillBitsType fill_bits(DisplayChannelClient *dcc, SpiceMarshaller *m,

             }

             spice_assert(!comp_send_data.is_lossy || can_lossy);

+            pthread_mutex_unlock(&dcc->pixmap_cache->lock);

             return (comp_send_data.is_lossy ? FILL_BITS_TYPE_COMPRESS_LOSSY :

                                               FILL_BITS_TYPE_COMPRESS_LOSSLESS);

         }

@@ -6909,11 +6915,12 @@ static FillBitsType fill_bits(DisplayChannelClient *dcc, SpiceMarshaller *m,

         spice_assert(bitmap_palette_out == NULL);

         spice_assert(lzplt_palette_out == NULL);

         spice_marshaller_add_ref_chunks(m, image.u.quic.data);

+        pthread_mutex_unlock(&dcc->pixmap_cache->lock);

         return FILL_BITS_TYPE_COMPRESS_LOSSLESS;

     default:

         spice_error("invalid image type %u", image.descriptor.type);

     }

-

+    pthread_mutex_unlock(&dcc->pixmap_cache->lock);

     return 0;

 }

-- 

2.4.4