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

[Christophe de Dinechin]

> On 3 Jul 2018, at 15:51, Christophe Fergeau <cfergeau at redhat.com> wrote:
> 
> 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.

I think we never had a discussion of what we really want in each case, and that’s causing the confusion.

First, a meta-rule. Like you, there is a lot in SPICE code I don’t like. When in doubt, I try to use consistency as a guide. If the code uses spice_critical in that file, my changes will use spice_critical. That’s the case for canvas_base.c.

Because of this consistency meta-rule, I am very much against changing on a case-by-case basis, i.e. introducing the first g_critical, in particular knowing that g_critical and spice_critical are not strictly equivalent. I’d much rather do a global change so that we understand the effects. Frediano’s recent “have you even tested this” message shows that it’s probably the correct approach. If I can’t see a change after changing all instances of spice_return_if_fail to g_return_if_fail, but Frediano immediately sees something wrong to the point of “screaming", we certainly don’t want to change only *one* of them, in particular an infrequently travelled path, because that’s calling for 

Then, on the specific point you raised. As I understand it, spice_critical *may* abort, but it does not always abort, it depends on the abort flag. Therefore, it is correct to free the resource in case it returns. Do you disagree with any part of that rationale (i.e. either disagree that spice_critical may return, or that we should free if it does)?

Finally, on removing spice_blah and move towards g_blah, I agree with the sentiment, but as explained above, I’d rather do that as a wholesale operation rather than piecemeal, it seems much less dangerous to me.


Thanks
Christophe




> 
> Christophe
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel