[Spice-devel] [PATCH 2/9] server/red_worker: red_draw_qxl_drawable: protect from NULL dereference in case of buggy driver (or recording)

[Fabiano Fidêncio]

On Thu, Oct 22, 2015 at 11:17 AM, Frediano Ziglio <fziglio at redhat.com> wrote:
>
>>
>> On Wed, Oct 21, 2015 at 2:15 PM, Frediano Ziglio <fziglio at redhat.com> wrote:
>> >
>> >>
>> >> From: Alon Levy <alon at pobox.com>
>> >>
>> >> ---
>> >>  server/red_worker.c | 5 +++++
>> >>  1 file changed, 5 insertions(+)
>> >>
>> >> diff --git a/server/red_worker.c b/server/red_worker.c
>> >> index ef529f1..225c272 100644
>> >> --- a/server/red_worker.c
>> >> +++ b/server/red_worker.c
>> >> @@ -4203,6 +4203,11 @@ static void red_draw_qxl_drawable(RedWorker
>> >> *worker,
>> >> Drawable *drawable)
>> >>
>> >>      image_cache_aging(&worker->image_cache);
>> >>
>> >> +    if (!canvas) {
>> >> +        spice_warning("ignoring drawable to destroyed surface %d\n",
>> >> drawable->surface_id);
>> >> +        return;
>> >> +    }
>> >> +
>> >>      region_add(&surface->draw_dirty_region,
>> >>      &drawable->red_drawable->bbox);
>> >>
>> >>      switch (drawable->red_drawable->type) {
>> >
>> > This is quite odd... when a surface is freed all drawables referring to
>> > that
>> > surface are freed. So if this happens it means that the memory status is
>> > not
>> > correct. I would replace perhaps with an assert instead.
>>
>> Thinking about the future "me" looking at this code and probably
>> bisecting to this commit I would keep it as a spice_warning().
>>
>
> Well, perhaps a spice_assert would remove writing a specific message int the
> log but a comment or replacing with spice_critical would solve your complaints.
>
> My point is that this condition should not happen even with buggy driver or
> recording. The drawable is checked when received (if has a valid surface) so
> if refer to an invalid surface should be discarded and not added to list/tree
> for later processing. If is from the list/tree and refer to an invalid surface
> this means that we failed to remove the drawable when the surface was freed
> which is a program bug. This is why I would suggest a program abortion to avoid
> possibly memory corruptions.
>
> Also consider that the last security patches (which are some weeks ago) should
> improve these checks and this patch is some years old so could be that the issue
> apply only to old code.
>
> Frediano

Indeed. I agree with your point.
Let's wait for Alon's  answer.

Best Regards,
-- 
Fabiano Fidêncio