DBusGProxy is supposed to be allowed to have a NULL name, eg if created by dbus_g_proxy_new_for_peer, but this causes some functions to crash, including its constructor. This bug appears to affect at least 0.36.2 and 0.50.
Created attachment 3431 [details] [review] Hopefully fixes this bug One of the fixes is to provide a wrapper for strcmp which allows one or more input strings to be NULL. I used this wrapper around a couple of other strcmp's in the same file to be on the safe side, but it may not be necessary for these.
I would call the utility function something descriptive like strcmp_allowing_null() ; the dbus_g_proxy namespace isn't required for a static function. Also, start the function name in column 0: static int strcmp_allowing_null( ... It wouldn't hurt to add a unit test for this, since as you can see there were about a half-dozen places it was broken, pretty plausible someone would accidentally add another one...
I agree it should have a more general name, but it shouldn't begin with str because that's reserved by the standard. I think I'll raise a "wishlist" bug for glib; that would be a good place to have a family of such functions. Not all the strcmps I changed were necessarily broken, I was just playing safe. OTOH I didn't touch any other source files, so some of them could contain similar bugs.
I've just thought of a better way to protect strcmp from NULL inputs: strcmp(s1 ? s1 : "", s2 ? s2 : ""); It's concise enough to be used inline instead of creating a wrapper function.
I don't think Owen will take the null-accepting strcmp in glib, he hasn't in the past. I think he has a pretty good argument in the general case probably. I wouldn't worry about starting with str either, I don't see that in the C99 spec, but even if it is we already have a ton of identifiers starting with underscore. If some libc we care about sprouts a function called this, we'll just rename ours in the next release; already-linked binaries won't break since the function is static. But whatever, picking some other short meaningful name is OK too. strcmp(s1 ? s1 : "", s2 ? s2 : "") I don't think is quite as good since it involves cut-and-paste.
Owen's points are fair enough. You can call it what you like I suppose if it's static, but it might be a good idea to make it available to other parts of the library and keep the dbus_ prefix. I think the standard does cover str[a-z] being reserved. 7.1.3 mentions "including the future library directions" and that section (7.26.11) says "Function names that begin with str, mem or or wcs and a lowercase letter may be added...". Identifiers beginning _[a-z] (but not upper case or two leading underscores) are OK for static linkage. We could use _strcmp_allowing_null(). strcmp(s1 ? s1 : "", s2 ? s2 : "") I thought would be OK for the implementaion of a function wrapper if you don't like cut & paste, but it could also be an inline function it's so short. But it does have a drawback I didn't realise until later, and that's the equivalence of "" and NULL. I think that's OK in this case though.
I've just triggered this bug in new functionality of gnome-power-manager, which uses the glib bindings heavily. I'm using 0.50 on FC4, but rawhide 0.60 is also affected. Until this bug is fixed, I'm stalled :-( Thanks, Richard. I get the following backtrace: 0x00463978 in find_name_in_info (a=0x0, b=0x0) at dbus-gproxy.c:497 497 return strcmp (info->name, name); (gdb) bt #0 0x00463978 in find_name_in_info (a=0x0, b=0x0) at dbus-gproxy.c:497 #1 0x03927494 in g_slist_find_custom () from /usr/lib/libglib-2.0.so.0 #2 0x004639ce in name_owner_foreach (key=0x0, val=0x0, data=0xbfd61724) at dbus-gproxy.c:524 #3 0x03904a12 in g_hash_table_foreach () from /usr/lib/libglib-2.0.so.0 #4 0x00463a5a in dbus_g_proxy_manager_lookup_name_owner (manager=Variable "manager" is not available. ) at dbus-gproxy.c:544 #5 0x00467012 in dbus_g_proxy_manager_register (manager=0x9b30ce8, proxy=0x9b3a2b8) at dbus-gproxy.c:939 #6 0x00467284 in dbus_g_proxy_new (connection=Variable "connection" is not available. ) at dbus-gproxy.c:1780 #7 0x0804debb in hal_device_get_bool (udi=0x9b3b630 "/org/freedesktop/Hal/devices/acpi_ADP1", key=0x9b29118 "ac_adapter.present", value=0x80596ec) at glibhal-main.c:68 #8 0x08053263 in hal_device_property_modified (udi=0x9b3b630 "/org/freedesktop/Hal/devices/acpi_ADP1", key=0x9b29118 "ac_adapter.present", is_added=0, is_removed=0) at gpm-main.c:418 #9 0x0804e5f3 in signal_handler_PropertyModified (proxy=0x9b3a788, type=1, properties=0x9afbe38) at glibhal-callback.c:86 #10 0x004641e8 in marshal_dbus_message_to_g_marshaller (closure=0x0, return_value=0x0, n_param_values=3, param_values=0xbfd61b1c, invocation_hint=0x0, marshal_data=0x0) at dbus-gproxy.c:1566 #11 0x003b5285 in g_closure_invoke () from /usr/lib/libgobject-2.0.so.0 #12 0x003c375b in g_signal_stop_emission () from /usr/lib/libgobject-2.0.so.0 #13 0x003c4eb0 in g_signal_emit_valist () from /usr/lib/libgobject-2.0.so.0 #14 0x003c5223 in g_signal_emit () from /usr/lib/libgobject-2.0.so.0 #15 0x00466695 in dbus_g_proxy_manager_filter (connection=0x9b293c0, message=0x9b7b980, user_data=0x9b30ce8) at dbus-gproxy.c:1618 #16 0x07b5a8bf in dbus_connection_dispatch (connection=0x9b293c0) at dbus-connection.c:3549 #17 0x0045dd07 in message_queue_dispatch (source=0x0, callback=0, user_data=0x0) at dbus-gmain.c:113 #18 0x039114ce in g_main_context_dispatch () from /usr/lib/libglib-2.0.so.0 #19 0x039144d6 in g_main_context_check () from /usr/lib/libglib-2.0.so.0 #20 0x039147c3 in g_main_loop_run () from /usr/lib/libglib-2.0.so.0 #21 0x08053eb5 in main (argc=3, argv=0xbfd620d4) at gpm-main.c:823 (gdb)
Also crashes gnome-panel in FC5 pretty simply (if any name owner is replaced it appears). Will see about checking in a fix, no real reason there isn't one, just nobody has done it...
Oops, we got off on that strcmp tangent and it wasn't even the right fix I don't think, though I don't understand this code very well and don't have time to figure it out right this second... Not sure how this relates to git migration, but I put in a patch that might help a little, at least getting an earlier backtrace. If someone has an easy testcase and can debug further I'd appreciate it (gnome-panel is a pain to work with, and I'm swamped right now)
*** Bug 5979 has been marked as a duplicate of this bug. ***
Related bug on GNOME bugzilla: http://bugzilla.gnome.org/show_bug.cgi?id=368487
Has this still not been fixed?
Hi, Can someone point me to which programs are using this so I know how to test the fix?
(In reply to comment #13) > Hi, > > Can someone point me to which programs are using this so I know how to test the > fix? The code at http://shiyee.dk/fileadmin/dbus-tests/ refers to this bug. My code at http://github.com/herzi/p2p-dbus doesn't suffer from this issue, so maybe this is WORKSFORME in the mean time.
This is so old I've forgotten where it was causing me a problem, but I think at the time I found a workaround in the affected application. The only useful info I can add now is that a suitable NULL-safe version of strcmp was added to glib; it's called g_strcmp0. The fix may not have been perfect, but it did fix at least some use cases and was extremely simple (so just by looking at it you could easily deduce at least it didn't cause anything worse than the problem it fixed), so wouldn't it have been better to include it instead of waiting more than 5 years for tests to be written or other related NULL-dereferences to be found?
Review of attachment 3431 [details] [review]: It appears this was fixed in 2007, with a regression test (peer-server and peer-client); the actual bugfix was commit af91f5e0e2. The key thing that's different in the patch that was committed is that peer-to-peer proxies are treated like proxies bound to a unique name (for_owner = TRUE), whereas here they're treated like proxies bound to a well-known name (FALSE). Proxies bound to a well-known name need some fairly elaborate machinery to track what the corresponding unique name is, none of which is meaningful on a peer-to-peer connection. In the well-known-name code path, a NULL name would make no sense, and would indicate a bug (which use of g_strcmp0() would disguise). I've checked each block of the patch and it doesn't seem to do anything that's still relevant, so I think we can close this. ::: /usr/src/dbus-0.50/glib/dbus-gproxy.c.orig @@ +372,3 @@ bp += len + 1; + if (dbus_g_proxy_strcmp (ap, bp) != 0) These can never be NULL, they point into the middle of an existing string @@ +502,3 @@ const char *name = b; + return dbus_g_proxy_strcmp (info->name, name); This function (and everything that calls it) is meaningless for peer-to-peer proxies - it's trying to determine a name owner, and name ownership is only meaningful when talking to a bus daemon. If we still get crashes here, the right fix is to work out why this is ever called for a peer, and correct that. @@ +661,3 @@ manager = proxy->manager; + if (!dbus_g_proxy_strcmp (proxy->name, name)) Likewise @@ +702,3 @@ proxy = tmp->data; + if (!dbus_g_proxy_strcmp (proxy->name, name)) Likewise @@ +1315,3 @@ construct_properties)); + proxy->for_owner = (proxy->name && proxy->name[0] == ':'); Fixed differently in the commit I mentioned
Fixed in 0.74
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.