[Spice-devel] [PATCH 16/18] dcc: change some assert

[Frediano Ziglio]

> 
> From: Marc-André Lureau <marcandre.lureau at gmail.com>
> 
> ---
>  server/dcc.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/server/dcc.c b/server/dcc.c
> index bb6001e..8e25c67 100644
> --- a/server/dcc.c
> +++ b/server/dcc.c
> @@ -343,7 +343,8 @@ static RedGlzDrawable
> *get_glz_drawable(DisplayChannelClient *dcc, Drawable *dra
>     NOTE - the caller should set the glz_instance returned by the encoder by
>     itself.*/
>  static GlzDrawableInstanceItem *add_glz_drawable_instance(RedGlzDrawable
>  *glz_drawable)
>  {
> -    spice_assert(glz_drawable->instances_count <
> MAX_GLZ_DRAWABLE_INSTANCES);
> +    spice_return_val_if_fail(glz_drawable->instances_count <
> MAX_GLZ_DRAWABLE_INSTANCES, NULL);
> +
>      // NOTE: We assume the additions are performed consecutively, without
>      removals in the middle
>      GlzDrawableInstanceItem *ret = glz_drawable->instances_pool +
>      glz_drawable->instances_count;
>      glz_drawable->instances_count++;

This will lead to a core on caller. Better the assert which stop with a proper error.

> @@ -363,11 +364,12 @@ int dcc_compress_image_glz(DisplayChannelClient *dcc,
>                             SpiceImage *dest, SpiceBitmap *src, Drawable
>                             *drawable,
>                             compress_send_data_t* o_comp_data)
>  {
> +    spice_return_val_if_fail(bitmap_fmt_is_rgb(src->format), FALSE);
> +
>      DisplayChannel *display_channel = DCC_TO_DC(dcc);
>  #ifdef COMPRESS_STAT
>      stat_time_t start_time = stat_now(display_channel->glz_stat.clock);
>  #endif
> -    spice_assert(bitmap_fmt_is_rgb(src->format));
>      GlzData *glz_data = &dcc->glz_data;
>      ZlibData *zlib_data;
>      LzImageType type = MAP_BITMAP_FMT_TO_LZ_IMAGE_TYPE[src->format];
> @@ -498,7 +500,7 @@ int dcc_compress_image_lz(DisplayChannelClient *dcc,
>      } else {
>          /* masks are 1BIT bitmaps without palettes, but they are not
>          compressed
>           * (see fill_mask) */
> -        spice_assert(src->palette);
> +        spice_return_val_if_fail(src->palette, FALSE);
>          dest->descriptor.type = SPICE_IMAGE_TYPE_LZ_PLT;
>          dest->u.lz_plt.data_size = size;
>          dest->u.lz_plt.flags = src->flags & SPICE_BITMAP_FLAGS_TOP_DOWN;

These would make sense if upper layer could deal with it.
Actually is not!
When dcc_compress_image is called is expected to have the compressed image set
but in these cases image is not initialized properly causing potentially
crashes or leaking of information.

> @@ -976,7 +978,7 @@ int dcc_pixmap_cache_unlocked_add(DisplayChannelClient
> *dcc, uint64_t id,
>      uint64_t serial;
>      int key;
>  
> -    spice_assert(size > 0);
> +    spice_return_val_if_fail(size > 0, FALSE);
>  
>      item = spice_new(NewCacheItem, 1);
>      serial = red_channel_client_get_message_serial(RED_CHANNEL_CLIENT(dcc));

This could make sense but if spice-server accept an image where width * height == 0
(size is unsigned so this condition is the same as size == 0) there should be no
warning/error/abortion, if it does not accept such images (which I think is the case)
it means a corruption happen and assert is perfectly fine.

> @@ -1005,7 +1007,7 @@ int dcc_pixmap_cache_unlocked_add(DisplayChannelClient
> *dcc, uint64_t id,
>  
>          now = &cache->hash_table[BITS_CACHE_HASH_KEY(tail->id)];
>          for (;;) {
> -            spice_assert(*now);
> +            spice_return_val_if_fail(*now, FALSE);
>              if (*now == tail) {
>                  *now = tail->next;
>                  break;

This is a ring, if you get a NULL there is a memory/structure corruption so
assert is fine.

Frediano