Bug 8132

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>
Severity:	normal
Priority:	high	Keywords:	patch
Version:	unspecified
Hardware:	x86 (IA32)
OS:	Linux (All)
Whiteboard:
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

Committed, thanks.

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.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.