Re: [PATCH] red_parse_qxl: fix throwing away drawables that have masks

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Dec 13, 2012 at 08:29:48AM -0500, Yonit Halperin wrote:
> On 12/13/2012 06:42 AM, Alon Levy wrote:
> >All looks good except the assert. We should be removing them whenever they are guest trigerable - maybe I'm not following the code, but if in red_get_image sees no pallete in the qxl struct then palette will be NULL, which means it's guest trigerable.
> 
> Hi,
> The assert is for "no palette" after the image was parsed by red_parse_qxl.
> red_parse_qxl is responsible for the validity tests, so in the point of the
> assert you are referring to, it is already a server bug and not a guest bug.

My bad, also sorry for the long delay, the assert is fine,

ACK.

> 
> Regards,
> Yonit.
> >
> >>Non rgb bitmaps are allowed to not have a palette in case they
> >>are masks (which are 1BIT bitmaps).
> >>
> >>Related: rhbz#864982
> >>---
> >>  server/red_parse_qxl.c |   27 +++++++++++++--------------
> >>  server/red_worker.c    |    8 +++-----
> >>  2 files changed, 16 insertions(+), 19 deletions(-)
> >>
> >>diff --git a/server/red_parse_qxl.c b/server/red_parse_qxl.c
> >>index 82463e1..4b39029 100644
> >>--- a/server/red_parse_qxl.c
> >>+++ b/server/red_parse_qxl.c
> >>@@ -367,7 +367,7 @@ static int bitmap_consistent(SpiceBitmap *bitmap)
> >>  static const int BITMAP_FMT_IS_RGB[] = {0, 0, 0, 0, 0, 0, 1, 1, 1,
> >>  1, 1};
> >>
> >>  static SpiceImage *red_get_image(RedMemSlotInfo *slots, int
> >>  group_id,
> >>-                                 QXLPHYSICAL addr, uint32_t flags)
> >>+                                 QXLPHYSICAL addr, uint32_t flags,
> >>int is_mask)
> >>  {
> >>      RedDataChunk chunks;
> >>      QXLImage *qxl;
> >>@@ -401,7 +401,7 @@ static SpiceImage *red_get_image(RedMemSlotInfo
> >>*slots, int group_id,
> >>      switch (red->descriptor.type) {
> >>      case SPICE_IMAGE_TYPE_BITMAP:
> >>          red->u.bitmap.format = qxl->bitmap.format;
> >>-        if (!bitmap_fmt_is_rgb(qxl->bitmap.format) &&
> >>!qxl->bitmap.palette) {
> >>+        if (!bitmap_fmt_is_rgb(qxl->bitmap.format) &&
> >>!qxl->bitmap.palette && !is_mask) {
> >>              spice_warning("guest error: missing palette on bitmap
> >>              format=%d\n",
> >>                            red->u.bitmap.format);
> >>              goto error;
> >>@@ -529,8 +529,7 @@ static void red_get_brush_ptr(RedMemSlotInfo
> >>*slots, int group_id,
> >>          }
> >>          break;
> >>      case SPICE_BRUSH_TYPE_PATTERN:
> >>-        red->u.pattern.pat = red_get_image(slots, group_id,
> >>qxl->u.pattern.pat, flags);
> >>-        red_get_point_ptr(&red->u.pattern.pos, &qxl->u.pattern.pos);
> >>+        red->u.pattern.pat = red_get_image(slots, group_id,
> >>qxl->u.pattern.pat, flags, FALSE);
> >>          break;
> >>      }
> >>  }
> >>@@ -549,7 +548,7 @@ static void red_get_qmask_ptr(RedMemSlotInfo
> >>*slots, int group_id,
> >>  {
> >>      red->flags  = qxl->flags;
> >>      red_get_point_ptr(&red->pos, &qxl->pos);
> >>-    red->bitmap = red_get_image(slots, group_id, qxl->bitmap,
> >>flags);
> >>+    red->bitmap = red_get_image(slots, group_id, qxl->bitmap, flags,
> >>TRUE);
> >>  }
> >>
> >>  static void red_put_qmask(SpiceQMask *red)
> >>@@ -574,7 +573,7 @@ static void red_put_fill(SpiceFill *red)
> >>  static void red_get_opaque_ptr(RedMemSlotInfo *slots, int group_id,
> >>                                 SpiceOpaque *red, QXLOpaque *qxl,
> >>                                 uint32_t flags)
> >>  {
> >>-    red->src_bitmap     = red_get_image(slots, group_id,
> >>qxl->src_bitmap, flags);
> >>+    red->src_bitmap     = red_get_image(slots, group_id,
> >>qxl->src_bitmap, flags, FALSE);
> >>     red_get_rect_ptr(&red->src_area, &qxl->src_area);
> >>     red_get_brush_ptr(slots, group_id, &red->brush, &qxl->brush,
> >>     flags);
> >>     red->rop_descriptor = qxl->rop_descriptor;
> >>@@ -592,7 +591,7 @@ static void red_put_opaque(SpiceOpaque *red)
> >>  static int red_get_copy_ptr(RedMemSlotInfo *slots, int group_id,
> >>                              SpiceCopy *red, QXLCopy *qxl, uint32_t
> >>                              flags)
> >>  {
> >>-    red->src_bitmap      = red_get_image(slots, group_id,
> >>qxl->src_bitmap, flags);
> >>+    red->src_bitmap      = red_get_image(slots, group_id,
> >>qxl->src_bitmap, flags, FALSE);
> >>      if (!red->src_bitmap) {
> >>          return 1;
> >>      }
> >>@@ -612,7 +611,7 @@ static void red_put_copy(SpiceCopy *red)
> >>  static void red_get_blend_ptr(RedMemSlotInfo *slots, int group_id,
> >>                               SpiceBlend *red, QXLBlend *qxl,
> >>                               uint32_t flags)
> >>  {
> >>-    red->src_bitmap      = red_get_image(slots, group_id,
> >>qxl->src_bitmap, flags);
> >>+    red->src_bitmap      = red_get_image(slots, group_id,
> >>qxl->src_bitmap, flags, FALSE);
> >>     red_get_rect_ptr(&red->src_area, &qxl->src_area);
> >>     red->rop_descriptor  = qxl->rop_descriptor;
> >>     red->scale_mode      = qxl->scale_mode;
> >>@@ -629,7 +628,7 @@ static void
> >>red_get_transparent_ptr(RedMemSlotInfo *slots, int group_id,
> >>                                      SpiceTransparent *red,
> >>                                      QXLTransparent *qxl,
> >>                                      uint32_t flags)
> >>  {
> >>-    red->src_bitmap      = red_get_image(slots, group_id,
> >>qxl->src_bitmap, flags);
> >>+    red->src_bitmap      = red_get_image(slots, group_id,
> >>qxl->src_bitmap, flags, FALSE);
> >>     red_get_rect_ptr(&red->src_area, &qxl->src_area);
> >>     red->src_color       = qxl->src_color;
> >>     red->true_color      = qxl->true_color;
> >>@@ -646,7 +645,7 @@ static void
> >>red_get_alpha_blend_ptr(RedMemSlotInfo *slots, int group_id,
> >>  {
> >>      red->alpha_flags = qxl->alpha_flags;
> >>      red->alpha       = qxl->alpha;
> >>-    red->src_bitmap  = red_get_image(slots, group_id,
> >>qxl->src_bitmap, flags);
> >>+    red->src_bitmap  = red_get_image(slots, group_id,
> >>qxl->src_bitmap, flags, FALSE);
> >>      red_get_rect_ptr(&red->src_area, &qxl->src_area);
> >>  }
> >>
> >>@@ -655,7 +654,7 @@ static void
> >>red_get_alpha_blend_ptr_compat(RedMemSlotInfo *slots, int group_id,
> >>                                             uint32_t flags)
> >>  {
> >>      red->alpha       = qxl->alpha;
> >>-    red->src_bitmap  = red_get_image(slots, group_id,
> >>qxl->src_bitmap, flags);
> >>+    red->src_bitmap  = red_get_image(slots, group_id,
> >>qxl->src_bitmap, flags, FALSE);
> >>      red_get_rect_ptr(&red->src_area, &qxl->src_area);
> >>  }
> >>
> >>@@ -690,12 +689,12 @@ static void
> >>red_get_composite_ptr(RedMemSlotInfo *slots, int group_id,
> >>  {
> >>      red->flags = qxl->flags;
> >>
> >>-    red->src_bitmap = red_get_image(slots, group_id, qxl->src,
> >>flags);
> >>+    red->src_bitmap = red_get_image(slots, group_id, qxl->src,
> >>flags, FALSE);
> >>      if (get_transform(slots, group_id, qxl->src_transform,
> >>      &red->src_transform))
> >>          red->flags |= SPICE_COMPOSITE_HAS_SRC_TRANSFORM;
> >>
> >>      if (qxl->mask) {
> >>-        red->mask_bitmap = red_get_image(slots, group_id, qxl->mask,
> >>flags);
> >>+        red->mask_bitmap = red_get_image(slots, group_id, qxl->mask,
> >>flags, FALSE);
> >>          red->flags |= SPICE_COMPOSITE_HAS_MASK;
> >>          if (get_transform(slots, group_id, qxl->mask_transform,
> >>          &red->mask_transform))
> >>              red->flags |= SPICE_COMPOSITE_HAS_MASK_TRANSFORM;
> >>@@ -718,7 +717,7 @@ static void red_put_composite(SpiceComposite
> >>*red)
> >>  static void red_get_rop3_ptr(RedMemSlotInfo *slots, int group_id,
> >>                               SpiceRop3 *red, QXLRop3 *qxl, uint32_t
> >>                               flags)
> >>  {
> >>-   red->src_bitmap = red_get_image(slots, group_id, qxl->src_bitmap,
> >>flags);
> >>+   red->src_bitmap = red_get_image(slots, group_id, qxl->src_bitmap,
> >>flags, FALSE);
> >>     red_get_rect_ptr(&red->src_area, &qxl->src_area);
> >>     red_get_brush_ptr(slots, group_id, &red->brush, &qxl->brush,
> >>     flags);
> >>     red->rop3       = qxl->rop3;
> >>diff --git a/server/red_worker.c b/server/red_worker.c
> >>index 530562b..e5e3d05 100644
> >>--- a/server/red_worker.c
> >>+++ b/server/red_worker.c
> >>@@ -6091,16 +6091,14 @@ static inline int
> >>red_lz_compress_image(DisplayChannelClient *dcc,
> >>          o_comp_data->comp_buf = lz_data->data.bufs_head;
> >>          o_comp_data->comp_buf_size = size;
> >>      } else {
> >>-        if (!src->palette) {
> >>-            spice_warning("bad guest: missing palette\n");
> >>-            return FALSE;
> >>-        }
> >>+        /* masks are 1BIT bitmaps without palettes, but they are not
> >>compressed
> >>+         * (see fill_mask) */
> >>+        spice_assert(src->palette);
> >>          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;
> >>          dest->u.lz_plt.palette = src->palette;
> >>          dest->u.lz_plt.palette_id = src->palette->unique;
> >>-
> >>          o_comp_data->comp_buf = lz_data->data.bufs_head;
> >>          o_comp_data->comp_buf_size = size;
> >>
> >>--
> >>1.7.7.6
> >>
> >>_______________________________________________
> >>Spice-devel mailing list
> >>Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> >>http://lists.freedesktop.org/mailman/listinfo/spice-devel
> >>
> 
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel


[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]