[Spice-devel] [spice PATCH] Lock the pixmap cache for the fill_bits function 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 5deb30b..fbebb67 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -6749,6 +6749,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;
@@ -6770,6 +6771,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,
@@ -6787,6 +6789,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;
}

@@ -6801,6 +6804,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: {
@@ -6832,6 +6836,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,
@@ -6849,6 +6854,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);
}
@@ -6862,11 +6868,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;
}

@@ -6997,7 +7004,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) {
@@ -8419,7 +8426,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