> On 3 Jul 2018, at 12:11, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
>
>>
>> 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.
I was trying to recreate the fall-through scenario which I believe existed before. But I agree with you, failure and exit is cleaner.
>
>>> + 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;
> }
Again, my rationale was to try to remove closer to the orignal logic of the code.
>
> ??
>
> I don't like kernel-like error prone (this syntax caused multiple security
> issues and leaks in the past) multiple label cleanup code.
I personally don’t see an error exit with many unexercised if / branches as less “error prone”. So if I can branch to a well-exercised and maintained main branch, I’d prefer that.
But there is an agreement that the solution is ugly (including by me), So I’ll rewrite it without gotos.
>
>> }
>>
>> 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