Bug 22156

This commit:

commit 94d68f00d9d244de3b1d66d3cf78bb5171552311
Author: Tambet Ingo <tambet@gmail.com>
Date:   Thu Dec 18 12:09:43 2008 +0200

    Use the provided interface for org.freedesktop.DBus.Properties.GetAll call.

Did change the behavior - before we just returned all object properties, regardless, which was generally wrong.  My guess what's happening here is some other code was requesting an interface your object didn't actually claim to implement, but dbus-glib just returned the properties anyways.

Does this seem to be the case?

The object implements the interface, but the interface has no properties. In that case, it should really return an empty array, not an error.

As far as I can tell from the code this should work; we do:

  if (getall)
    {
      object_info = lookup_object_info_by_iface (object, wincaps_propiface);
      if (object_info != NULL)
          ret = get_all_object_properties (connection, message, object_info, object);
      else
          return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
    ...

And get_all_object_properties always returns an array.  However we would do  DBUS_HANDLER_RESULT_NOT_YET_HANDLED if we can't find the interface on the object.

Is your code free software or somewhere where I can look at it?

The service is in telepathy-sofiasip:
http://git.collabora.co.uk/?p=telepathy-sofiasip.git;a=blob;f=src/sip-media-stream.c
And it has no dbus properties, but it does implement the org.freedesktop.Telepathy.Media.StreamHandler interface (which now does).

And the client is libtelepathy-farsight
http://git.collabora.co.uk/?p=telepathy-farsight.git;a=blob;f=telepathy-farsight/stream.c#l578

The thing is that the interface is implemented, but it has no exported properties I believe..

lookup_object_info_by_iface is mis-named (it specifically looks for properties, not for methods or signals). It returns NULL if there are no properties.

It turns out that (at least in v1 introspection) if there are no properties, no methods and no signals, then we have no record that an interface exists at all, so we can't possibly distinguish between these cases.

However, arguably GetAll("any.valid.interface.name") should return an empty array without error - the empty set of properties implemented by that interface. (Get() should still raise an error if it doesn't know the property.)

For what it's worth, telepathy-glib's TpDBusPropertiesMixin never raises an error, even if the interface name is syntatically invalid - it'll just not find any matching properties.

Created attachment 45861 [details] [review]
[PATCH 1/2] dbus-gobject: return empty from GetAll if there are no  known properties

Created attachment 45862 [details] [review]
[PATCH 2/2] Add a regression test for GetAll on objects with no  properties

I guess I'd prefer to modify the GetAll checks to still return some kind of error if the interface is not implemented at all, versus if it's implemented but has no properties (in which case, return empty set).

Isn't this as easy as removing the info->exported_properties check from lookup_object_info_by_iface_cb()?  All the code that calls lookup_object_info_by_iface() is smart enough to handle exported_properties being NULL.  A quick audit didn't turn up any cases where we'd be bypassing property access controls, but somebody would need to do a longer one than the 10 minutes I spent on it.  But I think that might get us where we need to be?

Thus lookup_object_info_by_iface() would always return the object info if the object implemented that interface, even if the interface had no properties.  The three places that call it are:

1) lookup_property_name(): if the interface has no properties, then there won't be any shadow properties, which will return the non-shadow property name, exactly the same as if object_info was NULL

2) check_property_access(): the checks for property_info_from_object_info() will fail because exported_properties is NULL and thus the while() loop in property_info_from_object_info() will simply not execute, and it'll return FALSE, leading to DBUS_ERROR_ACCESS_DENIED at the end of check_property_access().

3) object_registration_message() for GetAll: calls get_all_object_properties() which will open the returned properties container, never execute the while() loop due to exported_properties being NULL, close the container, and return.

That approach, which basically is:

--- a/dbus/dbus-gobject.c
+++ b/dbus/dbus-gobject.c
@@ -599,7 +599,7 @@ 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 if (!strcmp (info->exported_properties, lookup_data->iface))
     {
       lookup_data->info = info;
       lookup_data->iface_type = gtype;

passes your 2/2 testcase here too.  Though we'd want another testcase to ensure we get an error if the interface actually doesn't exist.

Comment on attachment 45862 [details] [review]
[PATCH 2/2] Add a regression test for GetAll on objects with no  properties

Review of attachment 45862 [details] [review]:
-----------------------------------------------------------------

You need this hunk too:

--- a/test/core/Makefile.am
+++ b/test/core/Makefile.am
@@ -123,7 +123,7 @@ test_variant_recursion_SOURCES=test-variant-recursion.c
 
 test_variant_recursion_LDADD= $(tool_ldadd)
 
-BUILT_SOURCES = test-service-glib-glue.h test-service-glib-subclass-glue.h test-service-glib-bindings.h my-object-marshal.c my-object-marshal.h 
+BUILT_SOURCES = test-service-glib-glue.h test-service-glib-subclass-glue.h test-service-glib-bindings.h my-object-marshal.c my-object-marshal.h boring-ifa
 
 test_service_glib_SOURCES=                             \
        my-object.c                             \

(In reply to comment #9)
> Comment on attachment 45862 [details] [review] [review]
> [PATCH 2/2] Add a regression test for GetAll on objects with no  properties
> 
> Review of attachment 45862 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> You need this hunk too:
> 
> --- a/test/core/Makefile.am
> +++ b/test/core/Makefile.am
> @@ -123,7 +123,7 @@ test_variant_recursion_SOURCES=test-variant-recursion.c
>  
>  test_variant_recursion_LDADD= $(tool_ldadd)
>  
> -BUILT_SOURCES = test-service-glib-glue.h test-service-glib-subclass-glue.h
> test-service-glib-bindings.h my-object-marshal.c my-object-marshal.h 
> +BUILT_SOURCES = test-service-glib-glue.h test-service-glib-subclass-glue.h
> test-service-glib-bindings.h my-object-marshal.c my-object-marshal.h
> boring-ifa
>  
>  test_service_glib_SOURCES=                             \
>         my-object.c                             \

Ugh, my cut/paste from the terminal failed because it's not wrapping long lines.  Basically, make sure you add boring-iface-glue.h to BUILT_SOURCES.

Comment on attachment 45862 [details] [review]
[PATCH 2/2] Add a regression test for GetAll on objects with no  properties

Review of attachment 45862 [details] [review]:
-----------------------------------------------------------------

Grr, also dont' forget to add the test to test/core/run-test.sh otherwise it won't get run for 'make check'.  Now that the test is actually run, my proposed change does *not* pass this test, because of the changed semantics for the case where an interface has no properties, methods, or signals (and thus no object info).

IMHO if the interface is implemented but has no properties, methods, or signals, then we should still return an error.  What would the point of that interface be?  Really, we just want to catch the case where the interface has no properties, but *does* have signals or methods, which is much much more useful.

No properties, Has signals | Has methods = return empty set

No properties, No signals, No methods = return error

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/dbus/dbus-glib/issues/8.