On Thu, Oct 22, 2015 at 11:17 AM, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
>
>>
>> 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
Indeed. I agree with your point.
Let's wait for Alon's answer.
Best Regards,
--
Fabiano Fidêncio
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel