[Spice-devel] [PATCH] Avoid NULL-dereference if canvas_get_image errors out

[Christophe Fergeau]

On Fri, Jun 29, 2018 at 05:21:22PM +0200, Christophe de Dinechin wrote:
> From: Christophe de Dinechin <dinechin at redhat.com>
> 
> In some error cases, canvas_get_image may return NULL.
> When this happens, calls like pixman_image_get_width(s)
> will crash. Add additional error paths to deal with
> these cases.

I assume you don't have a reproducer for this, this is just something
you noticed while looking at that code?

> 
> Signed-off-by: Christophe de Dinechin <dinechin at redhat.com>
> ---
>  common/canvas_base.c | 38 +++++++++++++++++++++++++++++++++++---
>  1 file changed, 35 insertions(+), 3 deletions(-)
> 
> diff --git a/common/canvas_base.c b/common/canvas_base.c
> index 6bf6e5d..bbaaf96 100644
> --- a/common/canvas_base.c
> +++ b/common/canvas_base.c
> @@ -3072,6 +3072,7 @@ static void canvas_draw_stroke(SpiceCanvas *spice_canvas, SpiceRect *bbox,
>              gc.tile = canvas_get_image(canvas,
>                                         stroke->brush.u.pattern.pat,
>                                         FALSE);
> +            spice_return_if_fail(gc.tile != NULL);

I'd favour g_return_if_fail(), I'd really like to kill these various
spice_* calls mirroring glib API. And spice_return_if_fail() will
abort() I think.

>          }
>          gc.tile_offset_x = stroke->brush.u.pattern.pos.x;
>          gc.tile_offset_y = stroke->brush.u.pattern.pos.y;
> @@ -3158,6 +3159,10 @@ static void canvas_draw_rop3(SpiceCanvas *spice_canvas, SpiceRect *bbox,
>      heigth = bbox->bottom - bbox->top;
> 
>      d = canvas_get_image_from_self(spice_canvas, bbox->left, bbox->top, width, heigth, FALSE);
> +    if (d == NULL) {
> +        spice_critical("no rop3 destination");

spice_critical calls abort() too

> +        goto d_error;
> +    }
>      surface_canvas = canvas_get_surface(canvas, rop3->src_bitmap);
>      if (surface_canvas) {
>          s = surface_canvas->ops->get_image(surface_canvas, FALSE);
> @@ -3176,10 +3181,14 @@ static void canvas_draw_rop3(SpiceCanvas *spice_canvas, SpiceRect *bbox,
>          src_pos.x = rop3->src_area.left;
>          src_pos.y = rop3->src_area.top;
>      }
> +    if (s == NULL) {
> +        spice_critical("no rop3 source");
> +        goto s_error;
> +    }
>      if (pixman_image_get_width(s) - src_pos.x < width ||
>          pixman_image_get_height(s) - src_pos.y < heigth) {
>          spice_critical("bad src bitmap size");
> -        return;
> +        goto s_error;
>      }
>      if (rop3->brush.type == SPICE_BRUSH_TYPE_PATTERN) {
>          SpiceCanvas *_surface_canvas;
> @@ -3191,6 +3200,12 @@ static void canvas_draw_rop3(SpiceCanvas *spice_canvas, SpiceRect *bbox,
>          } else {
>              p = canvas_get_image(canvas, rop3->brush.u.pattern.pat, FALSE);
>          }
> +        if (p == NULL) {
> +            spice_critical("no rop3 pattern");
> +            /* degrade to color only */
> +            do_rop3_with_color(rop3->rop3, d, s, &src_pos, rop3->brush.u.color);

Could you test this? If not, I feel a bit uncomfortable adding such a
fallback.

> +            goto p_error;
> +        }
>          SpicePoint pat_pos;
> 
>          pat_pos.x = (bbox->left - rop3->brush.u.pattern.pos.x) % pixman_image_get_width(p);
> @@ -3200,14 +3215,16 @@ static void canvas_draw_rop3(SpiceCanvas *spice_canvas, SpiceRect *bbox,
>      } else {
>          do_rop3_with_color(rop3->rop3, d, s, &src_pos, rop3->brush.u.color);
>      }
> +p_error:

Do we need 3 labels with very similar names? I'd think something like
this could work?

error:
    if (s != NULL) {
      pixman_image_unref(s);

      spice_canvas->ops->blit_image(spice_canvas, &dest_region, d,
                                    bbox->left,
                                    bbox->top);
    }

    if (d != NULL) {
      pixman_image_unref(d);
    }

    pixman_region32_fini(&dest_region);

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20180703/66c94d73/attachment.sig>