GNOME Bugzilla – Bug 642724
GDBusProxy: DBus properties returned from service are not validated
Last modified: 2011-03-28 16:13:23 UTC
Created attachment 181296 [details] [review] Validate DBus properties from services if possible If the proxy has a GDBusInterfaceInfo, then it knows what kind of properties to expect and what type they should have. So it should validate any incoming property from the dbus service against that. Otherwise, every application has to do it. Also, in the patch, there is one bit that may be a bit controversial, it ignores any property it doesn't know about. But I figure that if the application sets a GDBusInterfaceInfo with some properties, then its code will not know about properties that are not in the GDBusII. This should protect applications from crashing because of badly written services.
Since the g_hash_table_insert adopts the string, it'll leak on the early return paths. (Also, the param shouldn't be "const" if it's adopted.)
(In reply to comment #0) > Created an attachment (id=181296) [details] [review] > Validate DBus properties from services if possible Please avoid using return in the middle of a function - it's much less error-prone if functions have a single place to exit (just like they have a single place they start). > If the proxy has a GDBusInterfaceInfo, then it knows what kind of properties to > expect and what type they should have. So it should validate any incoming > property from the dbus service against that. Otherwise, every application has > to do it. > > Also, in the patch, there is one bit that may be a bit controversial, it > ignores any property it doesn't know about. But I figure that if the > application sets a GDBusInterfaceInfo with some properties, then its code will > not know about properties that are not in the GDBusII. > > This should protect applications from crashing because of badly written > services. I'm ok with doing this (in principle).
>+ iif (!info) >+ return; Also don't treat pointers as if they were booleans (all the other surrounding code does the right thing and uses != NULL and == NULL) - doing so only makes things harder to read.
> info = g_dbus_interface_info_lookup_property (proxy->priv->expected_interface, property_name); Also, a more real problem is that the cost of this lookup is O(n) in number of properties (as clearly documented [1]). Since we do this for every received property, it means that the cost of receiving all properties (as we do on construction) is O(n^2). That's probably not acceptable.... [1] : http://library.gnome.org/devel/gio/2.26/gio-D-Bus-Introspection-Data.html#g-dbus-interface-info-lookup-property
Created attachment 181336 [details] [review] Validate DBus properties from services if possible Thanks for the comments, they're fixed.. Except for the O(n) problem.. I guess there are two solutions here, either we put the DBusPropertyInfo in a hash table once they've been used once. Or we assume that n will always be sufficiently small that it doesn't matter. I should also note that you do a O(n) processing on every method call already. Also, the doc for "g-interface-info" says that it will typecheck incoming signals against the declared interface, but I don't see the code that does that?
For the O(n) thing, I just committed this http://git.gnome.org/browse/glib/commit/?id=5bcf54b29cfe65f07d362b48a7fce7ac7c9a6dc3 http://git.gnome.org/browse/glib/commit/?id=b845c62c7feb06f3d16921b5c08065fb13a1030b http://git.gnome.org/browse/glib/commit/?id=91f97ebbaad602115b3b26e36592a3b3a22cf9e6 so lookups are always O(1).
(In reply to comment #5) > Created an attachment (id=181336) [details] [review] > Validate DBus properties from services if possible > > Thanks for the comments, they're fixed.. Except for the O(n) problem.. I guess > there are two solutions here, either we put the DBusPropertyInfo in a hash > table once they've been used once. Or we assume that n will always be > sufficiently small that it doesn't matter. I should also note that you do a > O(n) processing on every method call already. OK, committed this with a small typo-fix (s/unkown/unknown/) and prefixed commit message http://git.gnome.org/browse/glib/commit/?id=aa59fb9dd1a20004a5ba5f4d97c271eb5abe01e9 Thanks! > Also, the doc for "g-interface-info" says that it will typecheck incoming > signals against the declared interface, but I don't see the code that does > that? Good point, fixed here: http://git.gnome.org/browse/glib/commit/?id=caf993df6f9dbf2cd01ae16f5d757187c44683e3