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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> 
> On Wed, Oct 21, 2015 at 2:15 PM, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
> >
> >>
> >> From: Alon Levy <alon@xxxxxxxxx>
> >>
> >> ---
> >>  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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]