>
> On Fri, Jun 29, 2018 at 05:21:22PM +0200, Christophe de Dinechin wrote:
> > From: Christophe de Dinechin <dinechin@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
> > 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.
>
The fact that you tried to use rop3->brush.u.pattern means that
probably rop3->brush.u.color is invalid (u is an union).
Are we trying to replace secure crashes with some other security
issues? I really don't like not tested code.
> > + 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);
why faking a draw if we don't know? Won't be better to do a
// normal path
do final stuff ...
return SUCCESS;
error:
if (x != NULL) { clean x; }
if (whatever) { other clean }
return FAILURE;
}
??
I don't like kernel-like error prone (this syntax caused multiple security
issues and leaks in the past) multiple label cleanup code.
> }
>
> if (d != NULL) {
> pixman_image_unref(d);
> }
>
> pixman_region32_fini(&dest_region);
>
> Christophe
>
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel