The reply to a DBus.Properties.Set() method call should go through the
same D-Bus connection. Thus remove the DBusConnection parameter from the
following functions:
- g_dbus_pending_property_success()
- g_dbus_pending_property_error_valist()
- g_dbus_pending_property_error()
If an interface is removed from the root path during the same mainloop
iteration that it was added we need to check for data->added before
doing the check for data->parent == NULL in the remove_interface()
function. Otherwise the added interface doesn't get removed from the
data->added list and will result in accessing freed memory:
==337== Invalid read of size 8
==337== at 0x4F65AFA: dbus_message_iter_append_basic (in /usr/lib64/libdbus-1.so.3.7.1)
==337== by 0x1247B5: append_interface (object.c:556)
==337== by 0x4C8DC5C: g_slist_foreach (gslist.c:840)
==337== by 0x1261F7: process_changes (object.c:594)
==337== by 0x126372: generic_unregister (object.c:997)
==337== by 0x4F69669: ??? (in /usr/lib64/libdbus-1.so.3.7.1)
==337== by 0x4F5CE51: dbus_connection_unregister_object_path (in /usr/lib64/libdbus-1.so.3.7.1)
==337== by 0x125E81: object_path_unref (object.c:1236)
==337== by 0x126136: g_dbus_unregister_interface (object.c:1361)
==337== by 0x14CDF0: service_exit (service.c:581)
==337== by 0x177556: plugin_cleanup (plugin.c:242)
==337== by 0x12221F: main (main.c:559)
==337== Address 0x5bc1550 is 0 bytes inside a block of size 56 free'd
==337== at 0x4A079AE: free (vg_replace_malloc.c:427)
==337== by 0x4C7850E: g_free (gmem.c:252)
==337== by 0x125DB0: remove_interface (object.c:671)
==337== by 0x125E3B: object_path_unref (object.c:1230)
==337== by 0x126136: g_dbus_unregister_interface (object.c:1361)
==337== by 0x14CDF0: service_exit (service.c:581)
==337== by 0x177556: plugin_cleanup (plugin.c:242)
==337== by 0x12221F: main (main.c:559)
If there's a pending property but its exists() callback returns false
the property should be considered invalidated and included in the
relevant list of the PropertiesChanged signal.
Minor fixes to make setter actually work:
- Add propdata in pending_property_set
- Break loop when we are removing propdata from list and we
found it
- in_args and out_args were swapped
- interface and method name arguments were swapped
If an interface is added and removed on the same mailoop iteration,
ObjectManager would try to send InterfacesAdded signal while running the
idler because the interface was added to data->added list.
This is easily reproduced by forcing an error path in a plugin
registration, like on sap_server_register(), resulting in the following
error:
==11795== Invalid read of size 4
==11795== at 0x496F592: dbus_message_iter_append_basic (dbus-message.c:2598)
==11795== by 0x117B39: append_interface (object.c:554)
==11795== by 0x48955E7: g_slist_foreach (gslist.c:840)
==11795== by 0x11923B: process_changes (object.c:592)
==11795== by 0x11956D: generic_unregister (object.c:980)
==11795== by 0x4973BAC: _dbus_object_tree_unregister_and_unlock (dbus-object-tree.c:516)
==11795== by 0x4965240: dbus_connection_unregister_object_path (dbus-connection.c:5776)
==11795== by 0x1178A5: object_path_unref (object.c:1219)
==11795== by 0x118517: g_dbus_unregister_interface (object.c:1344)
==11795== by 0x19AF5B: sap_exit (sap.c:385)
==11795== by 0x13E9E2: sap_server_register (server.c:1428)
==11795== by 0x13C092: sap_server_probe (manager.c:44)
With this patch we don't send the InterfacesAdded signal, removing it
from data->added while unregistering.
This interface is responsible for handling properties of all objects in
a given path. Right now it only registers itself, doing nothing useful.
A conversion to this new layout will be done by subsequent patches.
org.freedesktop.org.DBus.Properties spec can be found at
http://dbus.freedesktop.org/doc/dbus-specification.html#standard-interfaces-properties
When we add a signal handler with g_dbus_add_signal_watch(), this
function tries to multiplex the matches added in libdbus by checking
if there's a previous filter_data with the same fields. However, if the
field is NULL it accepts as being the same. The result is that the
following watches will use the same filter data:
watch1 = g_dbus_add_signal_watch(conn, BUS_NAME, NULL, iface, member,
cb1, data1, NULL);
watch2 = g_dbus_add_signal_watch(conn, BUS_NAME, "/path2", iface, member,
cb2, data2, NULL);
watch3 = g_dbus_add_signal_watch(conn, BUS_NAME, "/path3", iface, member,
cb3, data3, NULL);
The result is that when a signal arrives with path == "/path2", all 3
callbacks above will be called, with the same signal delivered to all of
them.
Another problem is that, if we invert the calls like below, only signals
to cb1 will never be trigerred, nonetheless it used path == NULL.
watch2 = g_dbus_add_signal_watch(conn, BUS_NAME, "/path2", iface, member,
cb2, data2, NULL);
watch1 = g_dbus_add_signal_watch(conn, BUS_NAME, NULL, iface, member,
cb1, data1, NULL);
watch3 = g_dbus_add_signal_watch(conn, BUS_NAME, "/path3", iface, member,
cb3, data3, NULL);
This is fixed by not multiplexing the matchs with filter data if any of
the fields are different, including being NULL. When a signal arrives,
if a field is NULL we accept it as a match, but not when adding the
signal handler.
When getting disconnected from the bus sometimes (maybe always?)
dbus_watch_handle() can cause the "info" context to be free'd meaning
that we should not try to access it after the call. The only member we
need access to is the connection pointer and as the code already has a
ref() call for it it's only natural to solve the issue by adding a local
variable not dependent on "info".
The backtrace of the crash fixed looks as follows:
Invalid read of size 8
at 0x121085: watch_func (mainloop.c:105)
by 0x4C72694: g_main_context_dispatch (gmain.c:2539)
by 0x4C729C7: g_main_context_iterate.isra.23 (gmain.c:3146)
by 0x4C72DC1: g_main_loop_run (gmain.c:3340)
by 0x120541: main (main.c:551)
Address 0x5bbcd90 is 16 bytes inside a block of size 24 free'd
at 0x4A079AE: free (vg_replace_malloc.c:427)
by 0x4C7837E: g_free (gmem.c:252)
by 0x4F708BF: dbus_watch_set_data (dbus-watch.c:614)
by 0x4F70938: _dbus_watch_unref (dbus-watch.c:132)
by 0x4F6E9A7: _dbus_transport_handle_watch (dbus-transport.c:884)
by 0x4F59AFB: _dbus_connection_handle_watch (dbus-connection.c:1497)
by 0x4F70AF9: dbus_watch_handle (dbus-watch.c:683)
by 0x121084: watch_func (mainloop.c:103)
by 0x4C72694: g_main_context_dispatch (gmain.c:2539)
by 0x4C729C7: g_main_context_iterate.isra.23 (gmain.c:3146)
by 0x4C72DC1: g_main_loop_run (gmain.c:3340)
by 0x120541: main (main.c:551)
If there's a signal watch that's also watching for name
(data->name_watch) currently we are trying to remove the message_filter
twice since we may have the following call chain:
filter_data_remove_callback()
filter_data_free()
g_dbus_remove_watch()
filter_data_remove_callback()
filter_data_free()
dbus_connection_remove_filter()
dbus_connection_remove_filter()
Because of this we can't currently watch for signals passing the bus
name. After this patch we don't have this issue anymore.
We fix it by removing the filter before calling filter_data_free() if we
are the last filter_data and thus avoid calling
dbus_connection_remove_filter() twice.
Even though service watches accepted a "destroy" callback, they were
being ignored. This fix properly pass them along so they are called when
the watch is removed.
Address an issue in which the daemon incorrectly handles D-Bus main
loop timeouts by only removing timeouts that are not enabled when
D-Bus requests a timeout removal.
generate_introspection_xml generates the root <node> tags with a
'name' attribute. This seems to be a valid attribute but it is not
consistent with the way the D-Bus daemon generates empty nodes.
For example if we register "/foo/bar", D-Bus daemon will generate for
"/foo" a introspection which looks like this:
<!DOCTYPE node PUBLIC "-//freedesktop//DTD D-BUS Object Introspection 1.0//EN"
"http://www.freedesktop.org/standards/dbus/1.0/introspect.dtd">
<node>
<node name="bar"/>
</node>
and generate_introspection_xml generates for "/foo/bar":
<!DOCTYPE node PUBLIC "-//freedesktop//DTD D-BUS Object Introspection 1.0//EN"
"http://www.freedesktop.org/standards/dbus/1.0/introspect.dtd">
<node name="/foo/bar">
</node>
Just don't add the 'name' attribute to the root node. The GLib
binding for D-Bus does it the same way.
Assume there is only one object registerd at "/". If we add a new
object at "/foo/bar" the introspection of "/" has to be updated. A new
node has to be added at "/".
invalidate_parent_data stops invaldating the whole path because the
boolean return value of dbus_connection_get_object_path_data is used
wrong.
If we get a TRUE just go on down in the path, if FALSE is return
dbus_connection_get_object_path_data has run out of memory.
This patch was generated by the following semantic patch
(http://coccinelle.lip6.fr/)
// <smpl>
@fix disable is_null,isnt_null1@
expression *E;
@@
- !E
+ E == NULL
// </smpl>
We where not dispatching data when a bus disconnects which cause
Disconnected signal to not be processed and thus causing the process to
either not exit or to not trigger callbacks registered with
g_dbus_set_disconnect_function.
To fix this now we always schedule a dispatch which will make sure data
still not processed will make its way to the proper handlers even if
disconnected.
