[Spice-devel] [spice PATCH] Lock the pixmap image cache for the entiry fill_bits call

[Sandy Stutsman]

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

Addresses: https://bugzilla.redhat.com/show_bug.cgi?id=1194354
---
 server/red_client_shared_cache.h | 20 ++++++++++----------
 server/red_worker.c              | 13 ++++++++++---
 2 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/server/red_client_shared_cache.h b/server/red_client_shared_cache.h
index 821ee18..4932f2d 100644
--- a/server/red_client_shared_cache.h
+++ b/server/red_client_shared_cache.h
@@ -42,7 +42,6 @@ static int FUNC_NAME(hit)(CACHE *cache, uint64_t id, int *lossy, DisplayChannelC
     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(locked_hit)(CACHE *cache, uint64_t id, int *lossy, DisplayChannelClient *dcc)
+{
+    int hit;
+    pthread_mutex_lock(&cache->lock);
+    hit = FUNC_NAME(hit)(cache,id,lossy, dcc);
+    pthread_mutex_unlock(&cache->lock);
+    return hit;
+}
+
 static int FUNC_NAME(set_lossy)(CACHE *cache, uint64_t id, int lossy)
 {
     NewCacheItem *item;
-    pthread_mutex_lock(&cache->lock);
 
     item = cache->hash_table[CACHE_HASH_KEY(id)];
 
@@ -76,7 +82,6 @@ static int FUNC_NAME(set_lossy)(CACHE *cache, uint64_t id, int lossy)
         }
         item = item->next;
     }
-    pthread_mutex_unlock(&cache->lock);
     return !!item;
 }
 
@@ -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);
+        }ixmap_cache
         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 58a7d00..2e4a94d 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -6748,6 +6748,7 @@ 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;
@@ -6769,6 +6770,7 @@ 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,
@@ -6786,6 +6788,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;
         }
 
@@ -6800,6 +6803,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: {
@@ -6831,6 +6835,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,
@@ -6848,6 +6853,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);
         }
@@ -6861,11 +6867,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;
 }
 
@@ -6996,7 +7003,7 @@ static int is_bitmap_lossy(RedChannelClient *rcc, SpiceImage *image, SpiceRect *
         int is_hit_lossy;
 
         out_data->id = image->descriptor.id;
-        if (pixmap_cache_hit(dcc->pixmap_cache, image->descriptor.id,
+        if (pixmap_cache_locked_hit(dcc->pixmap_cache, image->descriptor.id,
                              &is_hit_lossy, dcc)) {
             out_data->type = BITMAP_DATA_TYPE_CACHE;
             if (is_hit_lossy) {
@@ -8418,7 +8425,7 @@ static inline void display_channel_send_free_list(RedChannelClient *rcc)
          * But all this message pixmaps cache references used its old serial.
          * we use pixmap_cache_items to collect these pixmaps, and we update their serial
          * by calling pixmap_cache_hit. */
-        pixmap_cache_hit(dcc->pixmap_cache, dcc->send_data.pixmap_cache_items[i],
+        pixmap_cache_locked_hit(dcc->pixmap_cache, dcc->send_data.pixmap_cache_items[i],
                          &dummy, dcc);
     }
 
-- 
1.9.5.msysgit.0