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 _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel