From 3da1328b214f652fa605999029d528c967aeef7b Mon Sep 17 00:00:00 2001 From: Dan Williams Date: Mon, 24 Jan 2011 23:33:48 -0600 Subject: [PATCH] Fix handling of properties and multiple interfaces in the same introspection data Given introspection XML like this: With legacy property access disabled, properties from other interfaces should not be accessible on a given interface. Second, those properties should be accessible from their named interface. Neither of these things was the case. lookup_object_info_by_iface_cb() incorrectly checked only the first property when looking up the object info for a given interface, causing secondary interfaces to fail lookup, and thus Get/Set/GetAll requests for the secondary interfaces to fail. If legacy property access was disabled, lookup_property_name() should not have returned properties that did not match the requested interface. Third, in _dbus_gutils_wincaps_to_uscore(), a '-' should be replaced with a '_' so that requests for "foobar-baz" get converted into "foobar_baz" to match GObject property names used internally. --- .gitignore | 6 +- dbus/dbus-gobject.c | 78 ++++++++++++++- dbus/dbus-gutils.c | 2 +- test/interfaces/Makefile.am | 12 ++- test/interfaces/test-client.c | 167 +++++++++++++++++++++++++++++++ test/interfaces/test-server.c | 8 ++ test/interfaces/test-two-interfaces.c | 81 +++++++++++++++ test/interfaces/test-two-interfaces.h | 28 +++++ test/interfaces/test-two-interfaces.xml | 14 +++ 9 files changed, 387 insertions(+), 9 deletions(-) create mode 100644 test/interfaces/test-two-interfaces.c create mode 100644 test/interfaces/test-two-interfaces.h create mode 100644 test/interfaces/test-two-interfaces.xml diff --git a/.gitignore b/.gitignore index 3bbaf2a..dbf41fa 100644 --- a/.gitignore +++ b/.gitignore @@ -114,7 +114,6 @@ test/data/valid-service-files/debug-glib.service test/data/valid-service-files/test-interfaces.service.in test/data/valid-service-files/interfaces-test.service test/interfaces/test-service -test/interfaces/ test/test-service tools/Makefile tools/Makefile.in @@ -212,14 +211,13 @@ test/core/test-types test/data/valid-service-files/debug-echo.service test/data/valid-service-files/debug-glib.service test/data/valid-service-files/test-interfaces.service.in -test/interfaces/ test/test-service tools/Makefile tools/Makefile.in tools/dbus-bus-introspect.xml tools/dbus-glib-bindings.h -test/interfaces/*-dbus-binding.h -test/interfaces/*-dbus-glue.h +test/interfaces/*-bindings.h +test/interfaces/*-glue.h test/interfaces/test-server test/interfaces/test-client test/interfaces/test-goodbye-bindings.h diff --git a/dbus/dbus-gobject.c b/dbus/dbus-gobject.c index 98c08de..7e743f1 100644 --- a/dbus/dbus-gobject.c +++ b/dbus/dbus-gobject.c @@ -504,10 +504,27 @@ lookup_object_info_by_iface_cb (const DBusGObjectInfo *info, lookup_data->info = info; lookup_data->iface_type = gtype; } - else if (info->exported_properties && !strcmp (info->exported_properties, lookup_data->iface)) + else { - lookup_data->info = info; - lookup_data->iface_type = gtype; + const char *propsig; + + /* Iterate over properties until we find one with the requested interface */ + propsig = info->exported_properties; + while (propsig != NULL && *propsig) + { + const char *iface; + const char *propname; + const char *propname_uscore; + const char *access_type; + + propsig = property_iterate (propsig, info->format_version, &iface, &propname, &propname_uscore, &access_type); + if (g_strcmp0 (iface, lookup_data->iface) == 0) + { + lookup_data->info = info; + lookup_data->iface_type = gtype; + break; + } + } } return !lookup_data->info; @@ -1056,6 +1073,8 @@ lookup_property_name (GObject *object, GHashTable *shadow_props; char *shadow_prop_name = NULL, *uscore_name; GType iface_type = 0; + const char *propsig; + gboolean found = FALSE; g_assert (wincaps_propiface != NULL); g_assert (requested_propname != NULL); @@ -1066,6 +1085,39 @@ lookup_property_name (GObject *object, if (!object_info) return uscore_name; + /* Only enforce that the property was found in the object info if legacy + * property access is disabled. + */ + if (disable_legacy_property_access == TRUE) + { + /* Iterate over properties until we find one with the requested interface */ + propsig = object_info->exported_properties; + while (propsig != NULL && *propsig) + { + const char *piface; + const char *pname; + const char *puscore; + const char *access_type; + + propsig = property_iterate (propsig, object_info->format_version, &piface, &pname, &puscore, &access_type); + if (g_strcmp0 (piface, wincaps_propiface) == 0 && + (g_strcmp0 (pname, requested_propname) == 0 || + g_strcmp0 (puscore, requested_propname) == 0 || + g_strcmp0 (pname, uscore_name) == 0 || + g_strcmp0 (puscore, uscore_name) == 0)) + { + found = TRUE; + break; + } + } + + if (found == FALSE) + { + g_free (uscore_name); + return NULL; + } + } + shadow_props = (GHashTable *) g_type_get_qdata (iface_type, SHADOW_PROP_QUARK); if (shadow_props) { @@ -1171,7 +1223,16 @@ get_all_object_properties (DBusConnection *connection, p = property_iterate (p, object_info->format_version, &prop_ifname, &prop_name, &prop_uscored, &access_flags); + /* Ensure we ignore properties owned by other interfaces that might be + * in the introspection data if the object exports two interfaces in the + * same DBusGObjectInfo. + */ + if (g_strcmp0 (prop_ifname, wincaps_propiface)) + continue; + uscore_propname = lookup_property_name (object, wincaps_propiface, prop_name); + if (uscore_propname == NULL) + continue; pspec = g_object_class_find_property (G_OBJECT_GET_CLASS (object), uscore_propname); if (pspec == NULL) @@ -1995,6 +2056,17 @@ object_registration_message (DBusConnection *connection, dbus_message_iter_next (&iter); s = lookup_property_name (object, wincaps_propiface, requested_propname); + if (s == NULL) + { + ret = dbus_message_new_error_printf (message, + DBUS_ERROR_ACCESS_DENIED, + "Property \"%s\" of interface \"%s\" isn't exported (or may not exist)", + requested_propname, + wincaps_propiface); + dbus_connection_send (connection, ret, NULL); + dbus_message_unref (ret); + return DBUS_HANDLER_RESULT_HANDLED; + } if (!check_property_access (connection, message, object, wincaps_propiface, requested_propname, s, setter)) { diff --git a/dbus/dbus-gutils.c b/dbus/dbus-gutils.c index e882282..1dfe0af 100644 --- a/dbus/dbus-gutils.c +++ b/dbus/dbus-gutils.c @@ -102,7 +102,7 @@ _dbus_gutils_wincaps_to_uscore (const char *caps) } else { - g_string_append_c (str, *p); + g_string_append_c (str, (*p == '-') ? '_' : *p); } ++p; } diff --git a/test/interfaces/Makefile.am b/test/interfaces/Makefile.am index ed5bc16..3911714 100644 --- a/test/interfaces/Makefile.am +++ b/test/interfaces/Makefile.am @@ -35,6 +35,8 @@ test_service_SOURCES = \ test-dup-prop.h \ test-objects.c \ test-objects.h \ + test-two-interfaces.c \ + test-two-interfaces.h \ test-server.c test_client_SOURCES = \ @@ -46,13 +48,15 @@ BUILT_SOURCES = \ test-goodbye-glue.h \ test-dup-prop-a-glue.h \ test-dup-prop-b-glue.h \ + test-two-interfaces-glue.h \ test-song-bindings.h \ test-hello-bindings.h \ test-goodbye-bindings.h \ test-dup-prop-a-bindings.h \ test-dup-prop-a-bindings.h \ test-dup-prop-b-bindings.h \ - test-dup-prop-b-bindings.h + test-dup-prop-b-bindings.h \ + test-two-interfaces-bindings.h test-song-glue.h: test-song.xml $(top_builddir)/dbus/dbus-binding-tool$(EXEEXT) $(DBUS_BINDING_TOOL) --prefix=test_song --mode=glib-server --output=test-song-glue.h $(srcdir)/test-song.xml @@ -84,6 +88,12 @@ test-dup-prop-b-glue.h: test-dup-prop-b.xml $(top_builddir)/dbus/dbus-binding-to test-dup-prop-b-bindings.h: test-dup-prop-b.xml $(top_builddir)/dbus/dbus-binding-tool$(EXEEXT) $(DBUS_BINDING_TOOL) --prefix=test_dup_prop_b --mode=glib-client --output=test-dup-prop-b-bindings.h $(srcdir)/test-dup-prop-b.xml +test-two-interfaces-glue.h: test-two-interfaces.xml $(top_builddir)/dbus/dbus-binding-tool$(EXEEXT) + $(DBUS_BINDING_TOOL) --prefix=test_two_interfaces --mode=glib-server --output=test-two-interfaces-glue.h $(srcdir)/test-two-interfaces.xml + +test-two-interfaces-bindings.h: test-two-interfaces.xml $(top_builddir)/dbus/dbus-binding-tool$(EXEEXT) + $(DBUS_BINDING_TOOL) --prefix=test_two_interfaces --mode=glib-client --output=test-two-interfaces-bindings.h $(srcdir)/test-two-interfaces.xml + CLEANFILES = \ $(BUILT_SOURCES) \ diff --git a/test/interfaces/test-client.c b/test/interfaces/test-client.c index 9c254cc..5bb69bb 100644 --- a/test/interfaces/test-client.c +++ b/test/interfaces/test-client.c @@ -6,14 +6,19 @@ #include "test-goodbye-bindings.h" #include "test-dup-prop-a-bindings.h" #include "test-dup-prop-b-bindings.h" +#include "test-two-interfaces-bindings.h" #define TEST_NAMESPACE "org.freedesktop.DBus.GLib.Test.Interfaces" #define TEST_OBJECT_PATH "/org/freedesktop/DBus/GLib/Test/Interfaces" #define TEST_DP_OBJECT_PATH "/org/freedesktop/DBus/GLib/Test/DupPropInterfaces" +#define TEST_TWO_INTERFACES_OBJECT_PATH "/org/freedesktop/DBus/GLib/Test/TwoInterfaces" #define TEST_DP_IFACE_A "org.freedesktop.DBus.GLib.Test.Interfaces.A" #define TEST_DP_IFACE_B "org.freedesktop.DBus.GLib.Test.Interfaces.B" +#define TEST_TWO_INTERFACES_IFACE_1 "org.freedesktop.DBus.GLib.Test.Interfaces.Number1" +#define TEST_TWO_INTERFACES_IFACE_2 "org.freedesktop.DBus.GLib.Test.Interfaces.Number2" + static void test_dp_property (DBusGProxy *proxy, const char *detail, @@ -81,6 +86,126 @@ test_dp_property (DBusGProxy *proxy, g_print ("Got DupProp Interface %s property value matched expected\n", detail); } +#define DBUS_TYPE_G_MAP_OF_VARIANT (dbus_g_type_get_map ("GHashTable", G_TYPE_STRING, G_TYPE_VALUE)) +static void +test_ti_property_all (DBusGProxy *proxy, + const char *detail, + const char *iface, + const char *propname, + const char *expected, + const char *bad_propname) +{ + GError *error = NULL; + gboolean success; + GValue get_value = {0,}; + GHashTable *hash = NULL; + GHashTableIter iter; + gboolean found = FALSE; + gpointer key, data; + + success = dbus_g_proxy_call (proxy, "GetAll", &error, + G_TYPE_STRING, iface, + G_TYPE_INVALID, + DBUS_TYPE_G_MAP_OF_VARIANT, &hash, + G_TYPE_INVALID); + if (!success) + { + g_print ("Error while getting TwoInterface %s properties: %s\n", detail, error->message); + g_error_free (error); + exit(1); + } + else + g_print ("Got TwoInterface %s property with success\n", detail); + + g_hash_table_iter_init (&iter, hash); + while (g_hash_table_iter_next (&iter, &key, &data)) { + const GValue *val = data; + + + /* Check for a property name we're not supposed to find */ + if (g_strcmp0 (key, bad_propname) == 0) + { + g_print ("Found unexpected TwoInterface %s property %s\n", + detail, bad_propname); + exit(1); + } + + if (g_strcmp0 (key, propname) == 0) { + if (!G_VALUE_HOLDS_STRING (val)) + { + g_print ("Error comparing TwoInterface %s property %s: unexpected type %s\n", + detail, propname, G_VALUE_TYPE_NAME (val)); + exit(1); + } + else if (strcmp (g_value_get_string (val), expected) != 0) + { + g_print ("Error comparing TwoInterface %s property: expected %s, got %s\n", + detail, expected, g_value_get_string (&get_value)); + exit(1); + } + else + { + g_print ("Got TwoInterface %s property value matched expected\n", detail); + found = TRUE; + break; + } + } + } + + if (found == FALSE) + { + g_print ("Failed to find TwoInterface %s property %s\n", + detail, propname); + exit(1); + } +} + +static void +test_ti_property_get (DBusGProxy *proxy, + const char *detail, + const char *iface, + const char *propname, + const char *expected) +{ + GError *error = NULL; + gboolean success; + GValue get_value = {0,}; + + success = dbus_g_proxy_call (proxy, "Get", &error, + G_TYPE_STRING, iface, + G_TYPE_STRING, propname, + G_TYPE_INVALID, + G_TYPE_VALUE, &get_value, + G_TYPE_INVALID); + if (!success) + { + /* perhaps we expected failure because we know the property isn't there */ + if (expected == NULL) + return; + + g_print ("Error while getting TwoInterface %s property: %s\n", detail, error->message); + g_error_free (error); + exit(1); + } + else + g_print ("Got TwoInterface %s property with success\n", detail); + + if (!G_VALUE_HOLDS_STRING (&get_value)) + { + g_print ("Error comparing TwoInterface %s property: unexpected type %s\n", + detail, G_VALUE_TYPE_NAME (&get_value)); + exit(1); + } + else if (g_strcmp0 (g_value_get_string (&get_value), expected) != 0) + { + g_print ("Error comparing TwoInterface %s property: expected %s, got %s\n", + detail, expected, g_value_get_string (&get_value)); + exit(1); + } + else + g_print ("Got TwoInterface %s property value matched expected\n", detail); +} + int main (int argc, char **argv) @@ -91,6 +216,8 @@ main (int argc, gchar *str; gboolean success; DBusGProxy *dp_proxy; + DBusGProxy *ti_proxy; + DBusGProxy *tmp_proxy; g_type_init (); @@ -264,6 +391,46 @@ main (int argc, } } + /* Test that dbus-glib correctly exports an object that has two separate + * interfaces in it's introspection XML, such that dbus-glib puts the + * properties for each interface on that interface, and does not forget + * the second interface entirely. + */ + ti_proxy = dbus_g_proxy_new_for_name (connection, TEST_NAMESPACE, + TEST_TWO_INTERFACES_OBJECT_PATH, + "org.freedesktop.DBus.Properties"); + + /* If legacy property access is enabled, we expect to be able to read + * all properties of the object no matter what interface. + */ + test_ti_property_get (ti_proxy, "1", TEST_TWO_INTERFACES_IFACE_1, "AnotherProperty", "another property value"); + test_ti_property_get (ti_proxy, "2", TEST_TWO_INTERFACES_IFACE_2, "SomeProperty", "some property value"); + + /* If legacy property access is disabled, we expect that attempting to read + * a property we know is not part of an interface will fail. + */ + tmp_proxy = dbus_g_proxy_new_for_name (connection, TEST_NAMESPACE, + TEST_TWO_INTERFACES_OBJECT_PATH, + "org.freedesktop.DBus.GLib.Test.Interfaces.Number1"); + + if (!org_freedesktop_DBus_GLib_Test_Interfaces_Number1_disable_legacy_prop_access (tmp_proxy, &error)) + { + g_print ("Failed to disable legacy property access\n"); + g_error_free (error); + exit(1); + } + + test_ti_property_get (ti_proxy, "1", TEST_TWO_INTERFACES_IFACE_1, "AnotherProperty", NULL); + test_ti_property_get (ti_proxy, "2", TEST_TWO_INTERFACES_IFACE_2, "SomeProperty", NULL); + + /* Test that a GetAll of the interface doesn't return a property from the + * other interface. + */ + test_ti_property_all (ti_proxy, "1", TEST_TWO_INTERFACES_IFACE_1, + "SomeProperty", "some property value", "AnotherProperty"); + test_ti_property_all (ti_proxy, "2", TEST_TWO_INTERFACES_IFACE_2, + "AnotherProperty", "another property value", "SomeProperty"); + exit(0); } diff --git a/test/interfaces/test-server.c b/test/interfaces/test-server.c index fcf1b09..9427d08 100644 --- a/test/interfaces/test-server.c +++ b/test/interfaces/test-server.c @@ -3,10 +3,12 @@ #include "tools/dbus-glib-bindings.h" #include "test-objects.h" #include "test-dup-prop.h" +#include "test-two-interfaces.h" #define TEST_NAMESPACE "org.freedesktop.DBus.GLib.Test.Interfaces" #define TEST_OBJECT_PATH "/org/freedesktop/DBus/GLib/Test/Interfaces" #define TEST_DP_OBJECT_PATH "/org/freedesktop/DBus/GLib/Test/DupPropInterfaces" +#define TEST_TWO_INTERFACES_OBJECT_PATH "/org/freedesktop/DBus/GLib/Test/TwoInterfaces" static GMainLoop *loop = NULL; @@ -20,6 +22,7 @@ main (int argc, guint32 ret; TestBeatlesSong *song; TestDpObj *dp_obj; + TestTwoInterfaces *ti_obj; g_type_init (); @@ -59,6 +62,11 @@ main (int argc, TEST_DP_OBJECT_PATH, G_OBJECT (dp_obj)); + ti_obj = test_two_interfaces_new (); + dbus_g_connection_register_g_object (connection, + TEST_TWO_INTERFACES_OBJECT_PATH, + G_OBJECT (ti_obj)); + loop = g_main_loop_new (NULL, FALSE); g_main_loop_run (loop); diff --git a/test/interfaces/test-two-interfaces.c b/test/interfaces/test-two-interfaces.c new file mode 100644 index 0000000..03353c9 --- /dev/null +++ b/test/interfaces/test-two-interfaces.c @@ -0,0 +1,81 @@ +#ifdef HAVE_CONFIG_H +# include +#endif +#include +#include "test-two-interfaces.h" + +G_DEFINE_TYPE (TestTwoInterfaces, test_two_interfaces, G_TYPE_OBJECT) + +enum { + PROP_0, + PROP_SOME_PROPERTY, + PROP_ANOTHER_PROPERTY, + LAST_PROP +}; + +static gboolean +test_two_interfaces_disable_legacy_prop_access (TestTwoInterfaces *obj, + GError **error) +{ + dbus_glib_global_set_disable_legacy_property_access (); + return TRUE; +} + +#include "test-two-interfaces-glue.h" + +TestTwoInterfaces * +test_two_interfaces_new (void) +{ + return (TestTwoInterfaces *) g_object_new (TEST_TYPE_TWO_INTERFACES, NULL); +} + +static void +test_two_interfaces_init (TestTwoInterfaces *obj) +{ +} + +static void +get_property (GObject *object, guint prop_id, GValue *value, GParamSpec *pspec) +{ + switch (prop_id) { + case PROP_SOME_PROPERTY: + g_value_set_string (value, "some property value"); + break; + case PROP_ANOTHER_PROPERTY: + g_value_set_string (value, "another property value"); + break; + default: + G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec); + break; + } +} + +static void +test_two_interfaces_class_init (TestTwoInterfacesClass *tclass) +{ + GObjectClass *object_class = G_OBJECT_CLASS (tclass); + + object_class->get_property = get_property; + + /* This property is a member of the + * org.freedesktop.DBus.GLib.Test.Interfaces.Number1 interface + */ + g_object_class_install_property (object_class, PROP_SOME_PROPERTY, + g_param_spec_string (TEST_TWO_INTERFACES_SOME_PROPERTY, + "Some Property", + "Some Property", + NULL, G_PARAM_READABLE)); + + /* This property is a member of the + * org.freedesktop.DBus.GLib.Test.Interfaces.Number2 interface + */ + g_object_class_install_property (object_class, PROP_ANOTHER_PROPERTY, + g_param_spec_string (TEST_TWO_INTERFACES_ANOTHER_PROPERTY, + "Another Property", + "Another Property", + NULL, G_PARAM_READABLE)); + + dbus_g_object_type_install_info (G_TYPE_FROM_CLASS (object_class), + &dbus_glib_test_two_interfaces_object_info); +} + diff --git a/test/interfaces/test-two-interfaces.h b/test/interfaces/test-two-interfaces.h new file mode 100644 index 0000000..efd14dd --- /dev/null +++ b/test/interfaces/test-two-interfaces.h @@ -0,0 +1,28 @@ +#ifndef __TEST_TWO_INTERFACES_H__ +#define __TEST_TWO_INTERFACES_H__ + +#include + +#define TEST_TYPE_TWO_INTERFACES (test_two_interfaces_get_type ()) +#define TEST_TWO_INTERFACES(obj) (G_TYPE_CHECK_INSTANCE_CAST ((obj), TEST_TYPE_TWO_INTERFACES, TestTwoInterfaces)) +#define TEST_TWO_INTERFACES_CLASS(klass) (G_TYPE_CHECK_CLASS_CAST ((klass), TEST_TYPE_TWO_INTERFACES, TestTwoInterfacesClass)) +#define TEST_IS_TWO_INTERFACES(obj) (G_TYPE_CHECK_INSTANCE_TYPE ((obj), TEST_TYPE_TWO_INTERFACES)) +#define TEST_IS_TWO_INTERFACES_CLASS(klass) (G_TYPE_CHECK_CLASS_TYPE ((obj), TEST_TYPE_TWO_INTERFACES)) +#define TEST_TWO_INTERFACES_GET_CLASS(obj) (G_TYPE_INSTANCE_GET_CLASS ((obj), TEST_TYPE_TWO_INTERFACES, TestTwoInterfacesClass)) + +#define TEST_TWO_INTERFACES_SOME_PROPERTY "some-property" +#define TEST_TWO_INTERFACES_ANOTHER_PROPERTY "another-property" + +typedef struct { + GObject parent; +} TestTwoInterfaces; + +typedef struct { + GObjectClass parent; +} TestTwoInterfacesClass; + +GType test_two_interfaces_get_type (void); + +TestTwoInterfaces *test_two_interfaces_new (void); + +#endif /* __TEST_TWO_INTERFACES_H__ */ diff --git a/test/interfaces/test-two-interfaces.xml b/test/interfaces/test-two-interfaces.xml new file mode 100644 index 0000000..6df6460 --- /dev/null +++ b/test/interfaces/test-two-interfaces.xml @@ -0,0 +1,14 @@ + + + + + + + + + + + + + + -- 1.7.3.5