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

[Christophe Fergeau]

On Tue, Jul 03, 2018 at 03:16:19PM +0200, Christophe de Dinechin wrote:
> >> 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.
> 
> That’s inconsistent with the rest of the file (there are currently 47
> instances of spice_return macros in canvas_base.c).
> 
> What about doing a first pass that replaces all of them with
> g_return_if_fail? That would be another patch, specifically to kill
> that macro. I sent the patches. If they get acked before I resubmit
> this one, then I’ll fix it here too.

At least in that file, that works for me, though one has to take into
account that we'd be changing abort() to 'return'.

> 
> > 
> >>         }
> >>         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
> 
> Depending on context, it may. So what? Are you suggesting to do something else?

if (d == NULL) {
    abort();
    free(d);
}
is odd, either we abort, or we cleanup and return, I would not do both.
g_critical/g_warning would be better here.

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/e25e52f9/attachment.sig>