diff --git a/gio/gdbusproxy.c b/gio/gdbusproxy.c index 992280f43..57ec011dd 100644 --- a/gio/gdbusproxy.c +++ b/gio/gdbusproxy.c @@ -360,10 +360,33 @@ g_dbus_proxy_class_init (GDBusProxyClass *klass) * GDBusProxy:g-interface-info: * * Ensure that interactions with this proxy conform to the given - * interface. For example, when completing a method call, if the - * type signature of the message isn't what's expected, the given - * #GError is set. Signals that have a type signature mismatch are - * simply dropped. + * interface. This is mainly to ensure that malformed data received + * from the other peer is ignored. The given #GDBusInterfaceInfo is + * said to be the expected interface. + * + * The checks performed are: + * + * + * When completing a method call, if the type signature of + * the reply message isn't what's expected, the reply is + * discarded and the #GError is set to %G_IO_ERROR_INVALID_ARGUMENT. + * + * + * Received signals that have a type signature mismatch are dropped and + * a warning is logged via g_warning(). + * + * + * Properties received via the initial GetAll() call + * or via the ::PropertiesChanged signal (on the + * org.freedesktop.DBus.Properties interface) or + * set using g_dbus_proxy_set_cached_property() with a type signature + * mismatch are ignored and a warning is logged via g_warning(). + * + * + * Note that these checks are never done on methods, signals and + * properties that are not referenced in the given + * #GDBusInterfaceInfo, since extending a D-Bus interface on the + * service-side is not considered an ABI break. * * Since: 2.26 */ @@ -678,22 +701,17 @@ g_dbus_proxy_get_cached_property_names (GDBusProxy *proxy) * returned value */ static const GDBusPropertyInfo * -lookup_property_info_or_warn (GDBusProxy *proxy, - const gchar *property_name) +lookup_property_info (GDBusProxy *proxy, + const gchar *property_name) { - const GDBusPropertyInfo *info; + const GDBusPropertyInfo *info = NULL; if (proxy->priv->expected_interface == NULL) - return NULL; + goto out; info = g_dbus_interface_info_lookup_property (proxy->priv->expected_interface, property_name); - if (info == NULL) - { - g_warning ("Trying to lookup property %s which isn't in expected interface %s", - property_name, - proxy->priv->expected_interface->name); - } + out: return info; } @@ -706,8 +724,8 @@ lookup_property_info_or_warn (GDBusProxy *proxy, * blocking IO. * * If @proxy has an expected interface (see - * #GDBusProxy:g-interface-info), then @property_name (for existence) - * is checked against it. + * #GDBusProxy:g-interface-info) and @property_name is referenced by + * it, then @value is checked against the type of the property. * * Returns: A reference to the #GVariant instance that holds the value * for @property_name or %NULL if the value is not in the cache. The @@ -719,6 +737,7 @@ GVariant * g_dbus_proxy_get_cached_property (GDBusProxy *proxy, const gchar *property_name) { + const GDBusPropertyInfo *info; GVariant *value; g_return_val_if_fail (G_IS_DBUS_PROXY (proxy), NULL); @@ -728,10 +747,22 @@ g_dbus_proxy_get_cached_property (GDBusProxy *proxy, value = g_hash_table_lookup (proxy->priv->properties, property_name); if (value == NULL) + goto out; + + info = lookup_property_info (proxy, property_name); + if (info != NULL) { - lookup_property_info_or_warn (proxy, property_name); - /* no difference */ - goto out; + const gchar *type_string = g_variant_get_type_string (value); + if (g_strcmp0 (type_string, info->signature) != 0) + { + g_warning ("Trying to get property %s with type %s but according to the expected " + "interface the type is %s", + property_name, + type_string, + info->signature); + value = NULL; + goto out; + } } g_variant_ref (value); @@ -754,8 +785,8 @@ g_dbus_proxy_get_cached_property (GDBusProxy *proxy, * property cache. * * If @proxy has an expected interface (see - * #GDBusProxy:g-interface-info), then @property_name (for existence) - * and @value (for the type) is checked against it. + * #GDBusProxy:g-interface-info) and @property_name is referenced by + * it, then @value is checked against the type of the property. * * If the @value #GVariant is floating, it is consumed. This allows * convenient 'inline' use of g_variant_new(), e.g. @@ -798,7 +829,7 @@ g_dbus_proxy_set_cached_property (GDBusProxy *proxy, if (value != NULL) { - info = lookup_property_info_or_warn (proxy, property_name); + info = lookup_property_info (proxy, property_name); if (info != NULL) { if (g_strcmp0 (info->signature, g_variant_get_type_string (value)) != 0) @@ -865,22 +896,25 @@ on_signal_received (GDBusConnection *connection, if (proxy->priv->expected_interface != NULL) { const GDBusSignalInfo *info; - GVariantType *expected_type; info = g_dbus_interface_info_lookup_signal (proxy->priv->expected_interface, signal_name); - if (info == NULL) - { - G_UNLOCK (properties_lock); - goto out; - } - - expected_type = _g_dbus_compute_complete_signature (info->args); - if (!g_variant_type_equal (expected_type, g_variant_get_type (parameters))) + if (info != NULL) { + GVariantType *expected_type; + expected_type = _g_dbus_compute_complete_signature (info->args); + if (!g_variant_type_equal (expected_type, g_variant_get_type (parameters))) + { + gchar *expected_type_string = g_variant_type_dup_string (expected_type); + g_warning ("Dropping signal %s of type %s since the type from the expected interface is %s", + info->name, + g_variant_get_type_string (parameters), + expected_type_string); + g_free (expected_type_string); + g_variant_type_free (expected_type); + G_UNLOCK (properties_lock); + goto out; + } g_variant_type_free (expected_type); - G_UNLOCK (properties_lock); - goto out; } - g_variant_type_free (expected_type); } G_UNLOCK (properties_lock); @@ -908,15 +942,21 @@ insert_property_checked (GDBusProxy *proxy, if (proxy->priv->expected_interface != NULL) { const GDBusPropertyInfo *info; - info = g_dbus_interface_info_lookup_property (proxy->priv->expected_interface, property_name); - /* Ignore unknown properties */ - if (info == NULL) - goto invalid; - - /* Ignore properties with the wrong type */ - if (g_strcmp0 (info->signature, g_variant_get_type_string (value)) != 0) - goto invalid; + /* Only check known properties */ + if (info != NULL) + { + /* Warn about properties with the wrong type */ + if (g_strcmp0 (info->signature, g_variant_get_type_string (value)) != 0) + { + g_warning ("Received property %s with type %s does not match expected type " + "%s in the expected interface", + property_name, + g_variant_get_type_string (value), + info->signature); + goto invalid; + } + } } g_hash_table_insert (proxy->priv->properties, @@ -2328,10 +2368,9 @@ g_dbus_proxy_set_default_timeout (GDBusProxy *proxy, * g_dbus_proxy_get_interface_info: * @proxy: A #GDBusProxy * - * Returns the #GDBusInterfaceInfo, if any, specifying the minimal - * interface that @proxy conforms to. - * - * See the #GDBusProxy:g-interface-info property for more details. + * Returns the #GDBusInterfaceInfo, if any, specifying the interface + * that @proxy conforms to. See the #GDBusProxy:g-interface-info + * property for more details. * * Returns: A #GDBusInterfaceInfo or %NULL. Do not unref the returned * object, it is owned by @proxy. @@ -2360,12 +2399,8 @@ g_dbus_proxy_get_interface_info (GDBusProxy *proxy) * @info: (allow-none): Minimum interface this proxy conforms to or %NULL to unset. * * Ensure that interactions with @proxy conform to the given - * interface. For example, when completing a method call, if the type - * signature of the message isn't what's expected, the given #GError - * is set. Signals that have a type signature mismatch are simply - * dropped. - * - * See the #GDBusProxy:g-interface-info property for more details. + * interface. See the #GDBusProxy:g-interface-info property for more + * details. * * Since: 2.26 */ @@ -2487,21 +2522,17 @@ reply_cb (GDBusConnection *connection, * returned value */ static const GDBusMethodInfo * -lookup_method_info_or_warn (GDBusProxy *proxy, - const gchar *method_name) +lookup_method_info (GDBusProxy *proxy, + const gchar *method_name) { - const GDBusMethodInfo *info; + const GDBusMethodInfo *info = NULL; if (proxy->priv->expected_interface == NULL) - return NULL; + goto out; info = g_dbus_interface_info_lookup_method (proxy->priv->expected_interface, method_name); - if (info == NULL) - { - g_warning ("Trying to invoke method %s which isn't in expected interface %s", - method_name, proxy->priv->expected_interface->name); - } +out: return info; } @@ -2583,7 +2614,7 @@ g_dbus_proxy_call_internal (GDBusProxy *proxy, if (!was_split) { const GDBusMethodInfo *expected_method_info; - expected_method_info = lookup_method_info_or_warn (proxy, target_method_name); + expected_method_info = lookup_method_info (proxy, target_method_name); if (expected_method_info != NULL) reply_type = _g_dbus_compute_complete_signature (expected_method_info->out_args); } @@ -2717,7 +2748,7 @@ g_dbus_proxy_call_sync_internal (GDBusProxy *proxy, if (!was_split) { const GDBusMethodInfo *expected_method_info; - expected_method_info = lookup_method_info_or_warn (proxy, target_method_name); + expected_method_info = lookup_method_info (proxy, target_method_name); if (expected_method_info != NULL) reply_type = _g_dbus_compute_complete_signature (expected_method_info->out_args); } @@ -2821,6 +2852,10 @@ g_dbus_proxy_call_sync_internal (GDBusProxy *proxy, * &data); * ]| * + * If @proxy has an expected interface (see + * #GDBusProxy:g-interface-info) and @method_name is referenced by it, + * then the return value is checked against the return type. + * * This is an asynchronous method. When the operation is finished, * @callback will be invoked in the * thread-default main loop @@ -2908,6 +2943,10 @@ g_dbus_proxy_call_finish (GDBusProxy *proxy, * g_dbus_proxy_call() for the asynchronous version of this * method. * + * If @proxy has an expected interface (see + * #GDBusProxy:g-interface-info) and @method_name is referenced by it, + * then the return value is checked against the return type. + * * Returns: %NULL if @error is set. Otherwise a #GVariant tuple with * return values. Free with g_variant_unref(). * diff --git a/gio/tests/gdbus-proxy.c b/gio/tests/gdbus-proxy.c index 142476f99..975a9ac03 100644 --- a/gio/tests/gdbus-proxy.c +++ b/gio/tests/gdbus-proxy.c @@ -421,6 +421,8 @@ test_signals (GDBusProxy *proxy) g_string_free (s, TRUE); } +/* ---------------------------------------------------------------------------------------------------- */ + static void test_bogus_method_return (GDBusProxy *proxy) { @@ -439,12 +441,88 @@ test_bogus_method_return (GDBusProxy *proxy) g_assert (result == NULL); } +static void +test_bogus_signal (GDBusProxy *proxy) +{ + GError *error = NULL; + GVariant *result; + GDBusInterfaceInfo *old_iface_info; + + result = g_dbus_proxy_call_sync (proxy, + "EmitSignal2", + NULL, + G_DBUS_CALL_FLAGS_NONE, + -1, + NULL, + &error); + g_assert_no_error (error); + g_assert (result != NULL); + g_assert_cmpstr (g_variant_get_type_string (result), ==, "()"); + g_variant_unref (result); + + if (g_test_trap_fork (0, G_TEST_TRAP_SILENCE_STDOUT | G_TEST_TRAP_SILENCE_STDERR)) + { + /* and now wait for the signal that will never arrive */ + _g_assert_signal_received (proxy, "g-signal"); + } + g_test_trap_assert_stderr ("*Dropping signal TestSignal2 of type (i) since the type from the expected interface is (u)*"); + g_test_trap_assert_failed(); + + /* Our main branch will also do g_warning() when running the mainloop so + * temporarily remove the expected interface + */ + old_iface_info = g_dbus_proxy_get_interface_info (proxy); + g_dbus_proxy_set_interface_info (proxy, NULL); + _g_assert_signal_received (proxy, "g-signal"); + g_dbus_proxy_set_interface_info (proxy, old_iface_info); +} + +static void +test_bogus_property (GDBusProxy *proxy) +{ + GError *error = NULL; + GVariant *result; + GDBusInterfaceInfo *old_iface_info; + + /* Make the service emit a PropertiesChanged signal for property 'i' of type 'i' - since + * our introspection data has this as type 'u' we should get a warning on stderr. + */ + result = g_dbus_proxy_call_sync (proxy, + "FrobSetProperty", + g_variant_new ("(sv)", + "i", g_variant_new_int32 (42)), + G_DBUS_CALL_FLAGS_NONE, + -1, + NULL, + &error); + g_assert_no_error (error); + g_assert (result != NULL); + g_assert_cmpstr (g_variant_get_type_string (result), ==, "()"); + g_variant_unref (result); + + if (g_test_trap_fork (0, G_TEST_TRAP_SILENCE_STDOUT | G_TEST_TRAP_SILENCE_STDERR)) + { + /* and now wait for the signal that will never arrive */ + _g_assert_signal_received (proxy, "g-properties-changed"); + } + g_test_trap_assert_stderr ("*Received property i with type i does not match expected type u in the expected interface*"); + g_test_trap_assert_failed(); + + /* Our main branch will also do g_warning() when running the mainloop so + * temporarily remove the expected interface + */ + old_iface_info = g_dbus_proxy_get_interface_info (proxy); + g_dbus_proxy_set_interface_info (proxy, NULL); + _g_assert_signal_received (proxy, "g-properties-changed"); + g_dbus_proxy_set_interface_info (proxy, old_iface_info); +} + /* ---------------------------------------------------------------------------------------------------- */ static const gchar *frob_dbus_interface_xml = "" " " - /* Deliberately different from gdbus-testserver.py's definition */ + /* PairReturn() is deliberately different from gdbus-testserver.py's definition */ " " " " " " @@ -456,7 +534,14 @@ static const gchar *frob_dbus_interface_xml = " " " " " " + /* We deliberately only mention a single property here */ " " + /* The 'i' property is deliberately different from gdbus-testserver.py's definition */ + " " + /* ::TestSignal2 is deliberately different from gdbus-testserver.py's definition */ + " " + " " + " " " " ""; static GDBusInterfaceInfo *frob_dbus_interface_info; @@ -464,6 +549,9 @@ static GDBusInterfaceInfo *frob_dbus_interface_info; static void test_expected_interface (GDBusProxy *proxy) { + GVariant *value; + GError *error; + /* This is obviously wrong but expected interface is not set so we don't fail... */ g_dbus_proxy_set_cached_property (proxy, "y", g_variant_new_string ("error_me_out!")); g_dbus_proxy_set_cached_property (proxy, "y", g_variant_new_byte (42)); @@ -473,11 +561,12 @@ test_expected_interface (GDBusProxy *proxy) /* Now repeat the method tests, with an expected interface set */ g_dbus_proxy_set_interface_info (proxy, frob_dbus_interface_info); test_methods (proxy); + test_signals (proxy); - /* And now one more test where we deliberately set the expected - * interface definition incorrectly - */ + /* And also where we deliberately set the expected interface definition incorrectly */ test_bogus_method_return (proxy); + test_bogus_signal (proxy); + test_bogus_property (proxy); /* Also check that we complain if setting a cached property of the wrong type */ if (g_test_trap_fork (0, G_TEST_TRAP_SILENCE_STDOUT | G_TEST_TRAP_SILENCE_STDERR)) @@ -487,15 +576,57 @@ test_expected_interface (GDBusProxy *proxy) g_test_trap_assert_stderr ("*Trying to set property y of type s but according to the expected interface the type is y*"); g_test_trap_assert_failed(); - if (g_test_trap_fork (0, G_TEST_TRAP_SILENCE_STDOUT | G_TEST_TRAP_SILENCE_STDERR)) - { - g_dbus_proxy_set_cached_property (proxy, "does-not-exist", g_variant_new_string ("something")); - } - g_test_trap_assert_stderr ("*Trying to lookup property does-not-exist which isn't in expected interface com.example.Frob*"); - g_test_trap_assert_failed(); - /* this should work, however (since the type is correct) */ g_dbus_proxy_set_cached_property (proxy, "y", g_variant_new_byte (42)); + + /* Try to get the value of a property where the type we expect is different from + * what we have in our cache (e.g. what the service returned) + */ + if (g_test_trap_fork (0, G_TEST_TRAP_SILENCE_STDOUT | G_TEST_TRAP_SILENCE_STDERR)) + { + value = g_dbus_proxy_get_cached_property (proxy, "i"); + } + g_test_trap_assert_stderr ("*Trying to get property i with type i but according to the expected interface the type is u*"); + g_test_trap_assert_failed(); + + /* Even if a property does not exist in expected_interface, looking it + * up, or setting it, should never fail. Because it could be that the + * property has been added to the service but the GDBusInterfaceInfo* + * passed to g_dbus_proxy_set_interface_info() just haven't been updated. + * + * See https://bugzilla.gnome.org/show_bug.cgi?id=660886 + */ + value = g_dbus_proxy_get_cached_property (proxy, "d"); + g_assert (value != NULL); + g_assert (g_variant_is_of_type (value, G_VARIANT_TYPE_DOUBLE)); + g_assert_cmpfloat (g_variant_get_double (value), ==, 7.5); + g_variant_unref (value); + /* update it via the cached property... */ + g_dbus_proxy_set_cached_property (proxy, "d", g_variant_new_double (75.0)); + /* ... and finally check that it has changed */ + value = g_dbus_proxy_get_cached_property (proxy, "d"); + g_assert (value != NULL); + g_assert (g_variant_is_of_type (value, G_VARIANT_TYPE_DOUBLE)); + g_assert_cmpfloat (g_variant_get_double (value), ==, 75.0); + g_variant_unref (value); + /* now update it via the D-Bus interface... */ + error = NULL; + value = g_dbus_proxy_call_sync (proxy, "FrobSetProperty", + g_variant_new ("(sv)", "d", g_variant_new_double (85.0)), + G_DBUS_CALL_FLAGS_NONE, + -1, NULL, &error); + g_assert_no_error (error); + g_assert (value != NULL); + g_assert_cmpstr (g_variant_get_type_string (value), ==, "()"); + g_variant_unref (value); + /* ...ensure we receive the ::PropertiesChanged signal... */ + _g_assert_signal_received (proxy, "g-properties-changed"); + /* ... and finally check that it has changed */ + value = g_dbus_proxy_get_cached_property (proxy, "d"); + g_assert (value != NULL); + g_assert (g_variant_is_of_type (value, G_VARIANT_TYPE_DOUBLE)); + g_assert_cmpfloat (g_variant_get_double (value), ==, 85.0); + g_variant_unref (value); } static void diff --git a/gio/tests/gdbus-testserver.py b/gio/tests/gdbus-testserver.py index f7be13e30..6b2b64bdc 100755 --- a/gio/tests/gdbus-testserver.py +++ b/gio/tests/gdbus-testserver.py @@ -218,6 +218,18 @@ class TestService(dbus.service.Object): # ---------------------------------------------------------------------------------------------------- + @dbus.service.signal("com.example.Frob", + signature="i") + def TestSignal2(self, int1): + pass + + @dbus.service.method("com.example.Frob", + in_signature='', out_signature='') + def EmitSignal2(self): + self.TestSignal2 (42) + + # ---------------------------------------------------------------------------------------------------- + @dbus.service.method("com.example.Frob", in_signature='i', out_signature='', async_callbacks=('return_cb', 'raise_cb')) def Sleep(self, msec, return_cb, raise_cb):