[glib] GDBusProxy: Correctly handle unknown members when having an expected interface
- From: David Zeuthen <davidz src gnome org>
- To: commits-list gnome org
- Cc:
- Subject: [glib] GDBusProxy: Correctly handle unknown members when having an expected interface
- Date: Wed, 5 Oct 2011 14:45:41 +0000 (UTC)
commit 2b963266b68a3b14afcaa237ed41319c99949e43
Author: David Zeuthen <davidz redhat com>
Date: Tue Oct 4 11:37:16 2011 -0400
GDBusProxy: Correctly handle unknown members when having an expected interface
Since it is valid for a D-Bus interface / service to add new methods,
signals or properties we must NEVER warn about unknown properties or
drop unknown signals or disallow unknown method invocations when we
have an expected interface.
So this means that the expected_interface machinery is only useful for
checking that the service didn't break ABI.
Also update the docs so it is clear exactly what it means to have an
expected interface.
https://bugzilla.gnome.org/show_bug.cgi?id=660886
Signed-off-by: David Zeuthen <davidz redhat com>
gio/gdbusproxy.c | 165 +++++++++++++++++++++++++----------------
gio/tests/gdbus-proxy.c | 147 ++++++++++++++++++++++++++++++++++--
gio/tests/gdbus-testserver.py | 12 +++
3 files changed, 253 insertions(+), 71 deletions(-)
---
diff --git a/gio/gdbusproxy.c b/gio/gdbusproxy.c
index 992280f..57ec011 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 <emphasis>expected interface</emphasis>.
+ *
+ * The checks performed are:
+ * <itemizedlist>
+ * <listitem><para>
+ * 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.
+ * </para></listitem>
+ * <listitem><para>
+ * Received signals that have a type signature mismatch are dropped and
+ * a warning is logged via g_warning().
+ * </para></listitem>
+ * <listitem><para>
+ * Properties received via the initial <literal>GetAll()</literal> call
+ * or via the <literal>::PropertiesChanged</literal> signal (on the
+ * <ulink url="http://dbus.freedesktop.org/doc/dbus-specification.html#standard-interfaces-properties";>org.freedesktop.DBus.Properties</ulink> 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().
+ * </para></listitem>
+ * </itemizedlist>
+ * 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
* <link linkend="g-main-context-push-thread-default">thread-default main loop</link>
@@ -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 142476f..975a9ac 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 =
"<node>"
" <interface name='com.example.Frob'>"
- /* Deliberately different from gdbus-testserver.py's definition */
+ /* PairReturn() is deliberately different from gdbus-testserver.py's definition */
" <method name='PairReturn'>"
" <arg type='u' name='somenumber' direction='in'/>"
" <arg type='s' name='somestring' direction='out'/>"
@@ -456,7 +534,14 @@ static const gchar *frob_dbus_interface_xml =
" <method name='Sleep'>"
" <arg type='i' name='timeout' direction='in'/>"
" </method>"
+ /* We deliberately only mention a single property here */
" <property name='y' type='y' access='readwrite'/>"
+ /* The 'i' property is deliberately different from gdbus-testserver.py's definition */
+ " <property name='i' type='u' access='readwrite'/>"
+ /* ::TestSignal2 is deliberately different from gdbus-testserver.py's definition */
+ " <signal name='TestSignal2'>"
+ " <arg type='u' name='somenumber'/>"
+ " </signal>"
" </interface>"
"</node>";
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();
+ /* 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))
{
- g_dbus_proxy_set_cached_property (proxy, "does-not-exist", g_variant_new_string ("something"));
+ value = g_dbus_proxy_get_cached_property (proxy, "i");
}
- 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_stderr ("*Trying to get property i with type i but according to the expected interface the type is u*");
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));
+ /* 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 f7be13e..6b2b64b 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):
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]
