Bug 23616

Summary: object paths etc. should be checked for validity
Product: dbus Reporter: Andres Salomon <dilinger>
Component: GLibAssignee: Simon McVittie <smcv>
Status: RESOLVED FIXED QA Contact: John (J5) Palmieri <johnp>
Severity: normal    
Priority: medium CC: cosimo.alfarano, rob.taylor, smcv, will
Version: unspecifiedKeywords: patch
Hardware: Other   
OS: All   
URL: http://git.collabora.co.uk/?p=user/smcv/dbus-glib-smcv.git;a=shortlog;h=refs/heads/check-more-args-23616
Whiteboard:
i915 platform: i915 features:
Bug Depends on: 30171, 33145    
Bug Blocks: 7909    
Attachments: [PATCH 1/5] dbus_g_proxy_new_from_proxy: check that the old proxy is in fact a proxy
[PATCH 2/5] Document that most DBusGProxy methods stop working on ::destroy
[PATCH 3/5] dbus_g_proxy_set_interface: check that it's a proxy and not destroyed
[PATCH 4/5] dbus_g_proxy_end_call: check that it's a proxy
[PATCH 5/5] DBusGProxy: link against GIO and use GDBus to check names' syntax
[PATCH 5/5 v2] DBusGProxy: link against GIO and use GDBus to check names' syntax

Description Andres Salomon 2009-08-31 19:15:00 UTC
I just saw the following:

process 4716: arguments to dbus_message_new_method_call() were
incorrect, assertion "_dbus_check_is_valid_path (path)" failed in file
dbus-message.c line 1078. This is normally a bug in some application
using the D-Bus library.

** ERROR **: Out of memory
aborting...
Aborted

The problem is in the following code:

  dbus = dbus_g_bus_get (DBUS_BUS_SYSTEM, &e);
  ofono = dbus_g_proxy_new_for_name (dbus, "org.ofono", priv->device, intf);

The problem is that the server expects a string that's a dbus path, but
the string was incorrect.  Ie, priv->device should've been "/G1", but
instead an old value was there ("dev:/dev/smd0").  This string was then
passed to dbus_g_proxy_new_for_name as the path.

It's not unreasonable to assume that the object path may come from something
that's not hardcoded (a config file, or from a user).  This type of error (invalid path)
should really be an error, not an assertion; it could easily be handled w/ an error
message.  

Alternatively, if it is to be handled with an assertion, there should be an associated
function that a programmer can use to sanity check the path.  Ie:

  if (!dbus_g_proxy_path_is_sane (priv->device))
    {
      g_set_error (error, "%s is an invalid path!", priv->device);
      return FALSE;
    }
  ofono = dbus_g_proxy_new_for_name (dbus, "org.ofono", priv->device, intf);
Comment 1 Simon McVittie 2011-03-29 09:07:36 UTC
Fixed in a branch, along with some more checks.
Comment 2 Simon McVittie 2011-03-29 09:08:27 UTC
(In reply to comment #0)
> ** ERROR **: Out of memory
> aborting...
> Aborted

This is another manifestation of Bug #30171, incidentally.
Comment 3 Simon McVittie 2011-03-29 09:08:58 UTC
Created attachment 45003 [details] [review]
[PATCH 1/5] dbus_g_proxy_new_from_proxy: check that the old proxy is in fact a proxy
Comment 4 Simon McVittie 2011-03-29 09:09:15 UTC
Created attachment 45004 [details] [review]
[PATCH 2/5] Document that most DBusGProxy methods stop working on ::destroy
Comment 5 Simon McVittie 2011-03-29 09:09:32 UTC
Created attachment 45005 [details] [review]
[PATCH 3/5] dbus_g_proxy_set_interface: check that it's a proxy and not destroyed

If it has emitted destroy, our use of priv->manager will be a NULL
pointer dereference.
Comment 6 Simon McVittie 2011-03-29 09:09:47 UTC
Created attachment 45006 [details] [review]
[PATCH 4/5] dbus_g_proxy_end_call: check that it's a proxy
Comment 7 Simon McVittie 2011-03-29 09:10:07 UTC
Created attachment 45007 [details] [review]
[PATCH 5/5] DBusGProxy: link against GIO and use GDBus to check names' syntax
Comment 8 Simon McVittie 2011-03-29 09:11:27 UTC
(In reply to comment #0)
> Alternatively, if it is to be handled with an assertion, there should be an
> associated
> function that a programmer can use to sanity check the path.

I used GDBus (GIO) for this; life's too short to implement these functions yet again.

11:39 < wjt> smcv: top marks for proposing that dbus-glib use methods from gdbus
Comment 9 Simon McVittie 2011-03-29 10:26:47 UTC
Created attachment 45010 [details] [review]
[PATCH 5/5 v2] DBusGProxy: link against GIO and use GDBus to check names' syntax

Oops, one of the NULL returns on a programming error should have been FALSE.
Comment 10 Will Thompson 2011-04-19 02:42:47 UTC
Review of attachment 45003 [details] [review]:

looks good.
Comment 11 Will Thompson 2011-04-19 02:46:57 UTC
Review of attachment 45004 [details] [review]:

I'm assuming that you grepped for g_return_if_fail (!DBUS_G_PROXY_DESTROYED (proxy)); to decide where to sprinkle these notes?

Looks good in any case.
Comment 12 Will Thompson 2011-04-19 02:47:32 UTC
Review of attachment 45005 [details] [review]:

Looks fine.
Comment 13 Will Thompson 2011-04-19 02:48:07 UTC
Review of attachment 45006 [details] [review]:

Looks good.
Comment 14 Will Thompson 2011-04-19 02:49:38 UTC
Review of attachment 45010 [details] [review]:

Looks fine.
Comment 15 Simon McVittie 2011-04-19 03:08:12 UTC
All applied to master for 0.94, thanks!

Bug #36216 has a similar documentation patch, adjusted to apply after this one.

Bug #7909, Bug #35767 should now be mergeable.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.