|Summary:||0.92 (and older): dbus_bus_remove_match() does not work in a filter callback|
|Product:||dbus||Reporter:||Kimmo Hämäläinen <kimmo.hamalainen>|
|Component:||core||Assignee:||Havoc Pennington <hp>|
|Status:||RESOLVED FIXED||QA Contact:||John (J5) Palmieri <johnp>|
|i915 platform:||i915 features:|
|Attachments:||proposed partial fix|
Description Kimmo Hämäläinen 2006-09-05 07:12:36 UTC
dbus_connection_dispatch() in dbus-connection.c makes a copy of the filter list before calling the filter callback but it does not check whether the called filter callback removed any callbacks from the (original) filter list using dbus_bus_remove_match(). Maybe it could be fixed as follows: 1) Make a copy of the filter list (as before). 2) Call a callback in the list copy. 3) Remove the called callback from the list copy. 4) Remove all callbacks that do no longer occur in the original list from the list copy. 5) If the list copy still has one or more functions, go to step 2. What do you think about this?
Comment 1 Havoc Pennington 2006-09-05 07:26:17 UTC
The usual way this problem is fixed is to have a way to mark the removed callback as destroyed, then check for the destroyed marker on each callback prior to invoking it from the copied list. The easiest way to mark destroyed is probably to set filter->function = NULL when the filter is removed.
Comment 2 Kimmo Hämäläinen 2006-09-06 00:46:40 UTC
Actually I think that we can only fix the case when the callback calls dbus_connection_remove_filter() to remove a filter function. I don't see how we could fix the case when the callback calls dbus_bus_remove_match(), because a message matching the match could be already in the client buffers. Or maybe I'm confused?
Comment 3 Kimmo Hämäläinen 2006-09-06 01:03:33 UTC
I think in order to fix the dbus_bus_remove_match() case, the client library would need to discard all messages matching the removed match from the buffers.
Comment 4 Kimmo Hämäläinen 2006-09-08 01:39:55 UTC
(In reply to comment #1) > The usual way this problem is fixed is to have a way to mark the removed > callback as destroyed, then check for the destroyed marker on each callback > prior to invoking it from the copied list. > > The easiest way to mark destroyed is probably to set filter->function = NULL > when the filter is removed. That seems to be already done, I just need to add the checking. I'll make a patch for this. (But fixing this bug will require more as Comment #2 says.)
Comment 5 Kimmo Hämäläinen 2006-09-08 03:04:17 UTC
I think there is also another bug in the original code: _dbus_message_filter_unref() is called for the filter list copy, but that will call the user callback, which is not probably what the user expects...
Comment 6 Kimmo Hämäläinen 2006-09-08 04:22:46 UTC
Created attachment 6882 [details] [review] proposed partial fix My previous comment turned out to be a lie, but here is a patch that fixes a possible crash, unless I have more brain damages.
Comment 7 Havoc Pennington 2006-09-08 06:41:59 UTC
looks correct, thanks.
Comment 8 John (J5) Palmieri 2006-09-08 08:24:04 UTC
Comment 9 Kimmo Hämäläinen 2006-09-11 03:40:33 UTC
Actually it was just a partial fix :) But I discussed this bug with some colleagues and realised that this cannot be fixed, because clearing up the client buffer would require knowledge about _all_ the matches, not just the one that was removed. So, this is a WONTFIX.