Avoid NULL-dereference if canvas_get_image errors out

Submitted by Christophe de Dinechin on July 3, 2018, 1:16 p.m.

Details

Message ID 00E6BE52-301B-4E4B-B26A-E3F5E7016EBA@dinechin.org
State New
Headers show
Series "Avoid NULL-dereference if canvas_get_image errors out" ( rev: 2 ) in Spice

Not browsing as part of any series.

Commit Message

Christophe de Dinechin July 3, 2018, 1:16 p.m.
> On 3 Jul 2018, at 11:47, Christophe Fergeau <cfergeau@redhat.com> wrote:
> 
> On Fri, Jun 29, 2018 at 05:21:22PM +0200, Christophe de Dinechin wrote:
>> From: Christophe de Dinechin <dinechin@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?

During the code review of some recent patches Frediano sent, to understand the impact of returning NULL.

I did not have a reproducer, but I did some testing using using fault injection, e.g.:

SPICE_TRACES=canvas_get_image=13 spicy -p 5900 -h turbo
[snip]
[539 0.328228] canvas_get_image: Get image canvas 0x7f8c90003c00 image 0x7f8c8bc2ba50 original 0 real_get 1
[540 0.328291] spice_critical: condition `src_image != NULL’ failed

Basically with fault injection code like this:


That being said, I obviously I did not test all paths.

> 
>> 
>> Signed-off-by: Christophe de Dinechin <dinechin@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.

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.

> 
>>         }
>>         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?


> 
>> +        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.

Thinking more about it, I agree with both Frediano and you, will remove it in next iteration.

> 
>> +            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);

I find this harder to read that way. But I’ll rewrite it.


> 
> Christophe

Patch hide | download patch | download mbox

diff --git a/common/canvas_base.c b/common/canvas_base.c
index 6bf6e5d..c53e80b 100644
--- a/common/canvas_base.c
+++ b/common/canvas_base.c
@@ -1128,6 +1128,9 @@  static pixman_image_t *get_surface_from_canvas(CanvasBase *canvas,
  * you have to be able to handle any image format. This is useful to avoid
  * e.g. losing alpha when blending a argb32 image on a rgb16 surface.
  */
+
+RECORDER_DEFINE(canvas_get_image, 32, "Canvas get image");
+
 static pixman_image_t *canvas_get_image_internal(CanvasBase *canvas, SpiceImage *image,
                                                  int want_original, int real_get)
 {
@@ -1136,6 +1139,15 @@  static pixman_image_t *canvas_get_image_internal(CanvasBase *canvas, SpiceImage
     pixman_format_code_t wanted_format, surface_format;
     int saved_want_original;
 
+    record(canvas_get_image, "Get image canvas %p image %p original %d real_get %d",
+           canvas, image, want_original, real_get);
+    if (RECORDER_TRACE(canvas_get_image) > 10) {
+        RECORDER_TRACE(canvas_get_image)--;
+        if ((RECORDER_TRACE(canvas_get_image) & 0xC) == 0xC) {
+            return NULL;
+        }
+    }
+
     /* When touching, only really allocate if we need to cache, or
      * if we're loading a GLZ stream (since those need inter-thread communication
      * to happen which breaks if we don't. */

Comments

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
On Tue, Jul 03, 2018 at 03:51:58PM +0200, Christophe Fergeau wrote:
> > Depending on context, it may. So what? Are you suggesting to do something else?
> 
> if (d == NULL) {
>     abort();
>     free(d);

(obviously not 'd' here, but free(something);)

Christophe
> On 3 Jul 2018, at 15:51, Christophe Fergeau <cfergeau@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
On Tue, Jul 03, 2018 at 04:31:10PM +0200, Christophe de Dinechin wrote:
> 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.

There are 2 occurrences of spice_critical in canvas_base.c, I'd push for
changing these 2, and really discourage use of
spice_critical/spice_warning in new code.

> 
> 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)?

spice_critical by default will abort(), it used to return in spice-gtk
and abort() in spice-server, but this is no longer the case,
spice_critical aborts in both cases. You could prevent spice_critical
from aborting if you set SPICE_ABORT_LEVEL, but I don't think we should
support someone reporting a bug after changing the default behaviour of
spice_critical(). So for me spice_critical() aborts(). If that's what
you want, I would not bother with the cleanups. If that's not what you
want, I'd use g_critical(), g_warning(), spice_warning() instead, and do
the cleanup.

Christophe
> On 3 Jul 2018, at 16:58, Christophe Fergeau <cfergeau@redhat.com> wrote:
> 
> On Tue, Jul 03, 2018 at 04:31:10PM +0200, Christophe de Dinechin wrote:
>> 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.
> 
> There are 2 occurrences of spice_critical in canvas_base.c, I'd push for
> changing these 2, and really discourage use of
> spice_critical/spice_warning in new code.

Again, agree to change globally. Disagree with you to connect the discussion with this patch, and disagree to create a local inconsistency for just a single line added.

> 
>> 
>> 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)?
> 
> spice_critical by default will abort(), it used to return in spice-gtk
> and abort() in spice-server, but this is no longer the case,
> spice_critical aborts in both cases. You could prevent spice_critical
> from aborting if you set SPICE_ABORT_LEVEL, but I don't think we should
> support someone reporting a bug after changing the default behaviour of
> spice_critical().

As long as there is a case (even if it’s not the default behaviour and infrequent) where spice_critical() returns, suggesting to *remove* the cleanup is incorrect. It’s not a matter of “not supporting”, it’s a matter of you requesting me to actually go ahead and change my code to break a case that is not broken in the patch as I submitted it.


> So for me spice_critical() aborts().

No, as stated above, and as stated by me earlier, spice_critical() *may* abort, but it may also *return*. That’s sufficient ground to leave the cleanup code in place.

> If that's what
> you want, I would not bother with the cleanups.

I want spice_ctitical because that’s what is being used in the file today (irrespective of what I may otherwise think about the incredibly confusing mess of SPICE and glib logging in general). And since spice_critical() may return, I need to leave the cleanup in place at the moment.


> If that's not what you
> want, I'd use g_critical(), g_warning(), spice_warning() instead, and do
> the cleanup.

I fail to see how g_critical is any different. It also may return or exit the program depending on some external environment. Only the default is different, but I do not write code to deal only with the default case, otherwise this patch would not even exist (canvas_get_image does not return NULL by default).


Thanks
Christophe

> 
> Christophe
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
On Wed, Jul 04, 2018 at 09:22:40AM +0200, Christophe de Dinechin wrote:
> > If that's what
> > you want, I would not bother with the cleanups.
> 
> I want spice_ctitical because that’s what is being used in the file
> today (irrespective of what I may otherwise think about the incredibly
> confusing mess of SPICE and glib logging in general). And since
> spice_critical() may return, I need to leave the cleanup in place at
> the moment.

As already suggested, it will be less confusing to use spice_warning()
instead. The other users of spice_critical() might have wanted the
abort(), here we don't want it, so we should as well use something which
gives us what we want.

Christophe