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

[Frediano Ziglio]

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