> On 3 Jul 2018, at 11:47, Christophe Fergeau <cfergeau@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?
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:
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. */
That being said, I obviously I did not test all paths.
>
>>
>> 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.
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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel