Bug 23616 - object paths etc. should be checked for validity
Summary: object paths etc. should be checked for validity
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: GLib (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Simon McVittie
QA Contact: John (J5) Palmieri
URL: http://git.collabora.co.uk/?p=user/sm...
Whiteboard:
Keywords: patch
Depends on: 30171 33145
Blocks: 7909
  Show dependency treegraph
 
Reported: 2009-08-31 19:15 UTC by Andres Salomon
Modified: 2011-04-19 03:08 UTC (History)
4 users (show)

See Also:
i915 platform:
i915 features:


Attachments
[PATCH 1/5] dbus_g_proxy_new_from_proxy: check that the old proxy is in fact a proxy (752 bytes, patch)
2011-03-29 09:08 UTC, Simon McVittie
Details | Splinter Review
[PATCH 2/5] Document that most DBusGProxy methods stop working on ::destroy (5.86 KB, patch)
2011-03-29 09:09 UTC, Simon McVittie
Details | Splinter Review
[PATCH 3/5] dbus_g_proxy_set_interface: check that it's a proxy and not destroyed (1.23 KB, patch)
2011-03-29 09:09 UTC, Simon McVittie
Details | Splinter Review
[PATCH 4/5] dbus_g_proxy_end_call: check that it's a proxy (773 bytes, patch)
2011-03-29 09:09 UTC, Simon McVittie
Details | Splinter Review
[PATCH 5/5] DBusGProxy: link against GIO and use GDBus to check names' syntax (6.16 KB, patch)
2011-03-29 09:10 UTC, Simon McVittie
Details | Splinter Review
[PATCH 5/5 v2] DBusGProxy: link against GIO and use GDBus to check names' syntax (6.16 KB, patch)
2011-03-29 10:26 UTC, Simon McVittie
Details | Splinter Review

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.