GNOME Bugzilla – Bug 635380
gdk_event_apply_filters is unsafe against changes in filter list
Last modified: 2010-11-22 18:20:00 UTC
If a event filter function calls gdk_window_add_filter or gdk_window_remove_filter the list that gdk_event_apply_filters walks will become inconsistent. This may result in very obscure bugs and/or crashes. An example of this problem may be found in gnome-desktop where a filter is added to detect xrandr events and the callback calls a function that adds and/or removes a filter in another class to monitor _NET_WORKSPACE changes. This results in a crash when adding a monitor.
Based on some IRC comments I made: GDK event filters have lasted as low tech for a long time ... I don't know if more complexity is actually needed. They are a bit hacky in the first place. But basically, we have the know-how to create massively safe-against-reentrancy hook-list invocation ... eg., GSignal or GMainContext. It's not as simple as copying the list - that doesn't work - usually it's done by embedding a reference count on the node and temporarily increasing that as you walk the list and only removing nodes when the refcount drops to 0. And also having a removed bitflag so if you recurse - which is certainly evil from a event filter but not entirely impossible you don't invoke filters you've already removed. What I wouldn't suggest is using GHookList. My experiences with it indicate that it's harder to figure out how to use it or read the resulting code than it is to code things correctly from scratch.
Created attachment 174928 [details] [review] Make gdk_event_apply_filters safe against changes in filter list An event filter may add or remove filters itself. This patch does two things to address this case. The first is to take a temporary reference to the filter while it is being used. The second is to wait until after the filter function is run before determining the next node in the list to process. This guards against changes to the next node. It also does not run functions that have been marked as removed. Though I'm not sure if this case can arise.
Review of attachment 174928 [details] [review]: Looks right to me. ::: gdk/x11/gdkeventsource.c @@ +76,3 @@ + } + + node = tmp_list; Might be better to move the node assignment above the tmp_list assignment. I almost overlooked the purpose of node further down...
Pushed with that change and fixed a typo in win32. commit 323df2b2800383832ed3c2e43626f2c6821c33ec