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

Rejected

Frediano