Second try.
Regards,
Iskren
On Sat, Mar 12, 2011 at 2:28 PM, Marty Jack <marty...@comcast.net> wrote:
> I have never encountered a system where it was believed to be desirable to
> allow something to be removed twice. It is important to keep data
> structures clean. If anything you would be more likely to see a debugging
> mode where the lists were fully checked after every insert or remove to make
> sure they are internally consistent, especially if they are important to
> keeping the system running. It's not that much different from memory
> allocation. A block is allocated, or it is free, and a double free is a
> bug.
>
> On 03/12/2011 07:01 AM, Iskren Chernev wrote:
> > Well, the actual problem is that something was removed twice. And I know
> where this place is, but I couldn't know if you would like to remove
> elements multiple times with no problems, or treat the list very carefully
> and remove elements only once. But then there should be a way to know if an
> element is already in a list or not. So if there is a way to know that, why
> not make a check inside wl_list_remove, just to make sure.
> >
> > I'll fix it the way you want it to be :)
> >
> > Regards,
> > Iskren
> >
> > On Sat, Mar 12, 2011 at 1:07 PM, Marty Jack <marty...@comcast.net<mailto:
> marty...@comcast.net>> wrote:
> >
> >
> >
> > On 03/11/2011 07:32 PM, Iskren Chernev wrote:
> > > Hello,
> > >
> > > I found a bug and fixed it with the patch :)
> > >
> > > *to reproduce:*
> > > run compositor on top of x11
> > >
> > > repeat
> > > run flower
> > > drag & drop it a little
> > > move the pointer in and out of the compositor/flower
> > > Ctrl+C the flower client
> > >
> > > it would break eventually
> > >
> > > *problem:*
> > > I found that the linked list surface->destroy_listener_list got
> corrupted at some point (it was not circular any more, strange next/prev
> etc), which causes the crash.
> > >
> > > *solution:*
> > > The problem was in wl_list_remove -- when you erase an element, you
> don't mark it as 'erased', by setting prev/next to NULL for example. Then if
> you erase it again the list becomes corrupt. I nullified the prev/next and
> check in the begining of wl_list_remove for not-in-list elements and just
> ignore them. That seems to fix it.
> > >
> > > Regards,
> > > Iskren
> > >
> > >
> > >
> > > _______________________________________________
> > > wayland-devel mailing list
> > > wayland-devel@lists.freedesktop.org <mailto:
> wayland-devel@lists.freedesktop.org>
> > > http://lists.freedesktop.org/mailman/listinfo/wayland-devel
> >
> > Unfortunately your fix only papers over the real problem, which is
> that the list got corrupted at some point. It would not actually solve the
> problem. The source of the corruption needs to be located and fixed; it
> could be removing something twice, inserting something twice, or the like.
> You could push ahead with this if you were interested.
> > _______________________________________________
> > wayland-devel mailing list
> > wayland-devel@lists.freedesktop.org <mailto:
> wayland-devel@lists.freedesktop.org>
> > http://lists.freedesktop.org/mailman/listinfo/wayland-devel
> >
> >
>
From 5bc2363f805a03c1d361c742a62535998891aad3 Mon Sep 17 00:00:00 2001
From: Iskren Chernev <iskren.cher...@gmail.com>
Date: Sat, 12 Mar 2011 02:28:04 +0200
Subject: [PATCH] Fixed bug in device focus management.
The pointer_focus_listener of a device was always removed from the
surface's destroy_listener_list, regardless of weather it was in such
a list or not. Added a function wl_list_in to check if a given list
element is part of any list. wl_list_remove sets both prev/next of the
removed element to NULL so it is clearly _not_ part of any list.
---
wayland/wayland-server.c | 6 ++++--
wayland/wayland-util.c | 8 ++++++++
wayland/wayland-util.h | 1 +
3 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/wayland/wayland-server.c b/wayland/wayland-server.c
index 036958f..74e5f01 100644
--- a/wayland/wayland-server.c
+++ b/wayland/wayland-server.c
@@ -369,7 +369,8 @@ wl_input_device_set_pointer_focus(struct wl_input_device *device,
device->pointer_focus = surface;
device->pointer_focus_time = time;
- wl_list_remove(&device->pointer_focus_listener.link);
+ if (wl_list_in(&device->pointer_focus_listener.link))
+ wl_list_remove(&device->pointer_focus_listener.link);
if (surface)
wl_list_insert(surface->destroy_listener_list.prev,
&device->pointer_focus_listener.link);
@@ -399,7 +400,8 @@ wl_input_device_set_keyboard_focus(struct wl_input_device *device,
device->keyboard_focus = surface;
device->keyboard_focus_time = time;
- wl_list_remove(&device->keyboard_focus_listener.link);
+ if (wl_list_in(&device->keyboard_focus_listener.link))
+ wl_list_remove(&device->keyboard_focus_listener.link);
if (surface)
wl_list_insert(surface->destroy_listener_list.prev,
&device->keyboard_focus_listener.link);
diff --git a/wayland/wayland-util.c b/wayland/wayland-util.c
index 3643274..735e2e1 100644
--- a/wayland/wayland-util.c
+++ b/wayland/wayland-util.c
@@ -46,6 +46,8 @@ wl_list_remove(struct wl_list *elm)
{
elm->prev->next = elm->next;
elm->next->prev = elm->prev;
+
+ elm->prev = elm->next = NULL;
}
WL_EXPORT int
@@ -70,6 +72,12 @@ wl_list_empty(struct wl_list *list)
return list->next == list;
}
+WL_EXPORT int
+wl_list_in(struct wl_list *elm)
+{
+ return elm->prev != NULL;
+}
+
WL_EXPORT void
wl_array_init(struct wl_array *array)
{
diff --git a/wayland/wayland-util.h b/wayland/wayland-util.h
index 6c1231a..d5a814c 100644
--- a/wayland/wayland-util.h
+++ b/wayland/wayland-util.h
@@ -115,6 +115,7 @@ void wl_list_init(struct wl_list *list);
void wl_list_insert(struct wl_list *list, struct wl_list *elm);
void wl_list_remove(struct wl_list *elm);
int wl_list_length(struct wl_list *list);
+int wl_list_in(struct wl_list *elm);
int wl_list_empty(struct wl_list *list);
#define __container_of(ptr, sample, member) \
--
1.7.4
_______________________________________________
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
