Bug 8376 - dbus_shutdown() called but connections were still live
dbus_shutdown() called but connections were still live
Status: RESOLVED FIXED
Product: dbus
Classification: Unclassified
Component: core
unspecified
Other Linux (All)
: high normal
Assigned To: Havoc Pennington
John (J5) Palmieri
: patch
: 8367 8377 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-09-21 03:30 UTC by frederic heem
Modified: 2011-04-07 09:47 UTC (History)
3 users (show)

See Also:


Attachments
proposed patch (3.17 KB, patch)
2007-10-26 07:04 UTC, Kimmo Hämäläinen
Details | Splinter Review
fixed patch (3.13 KB, patch)
2007-10-29 00:11 UTC, Kimmo Hämäläinen
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description frederic heem 2006-09-21 03:30:34 UTC
dbus_shutdown() warns about the following api miusse:
"dbus_shutdown() called but connections were still live"

Invoking dbus_connection_flush() doesn't solve the problem.
Comment 1 Thiago Macieira 2006-09-21 05:05:27 UTC
Make sure you call dbus_connection_close() on your connections (not the shared 
ones).

Make sure you call dbus_connection_unref() on the shared ones.
Comment 2 frederic heem 2006-09-21 05:19:09 UTC
That was already done, here is the destructor
dbus_g_connection_flush(pSniffer->pConnection);
dbus_g_connection_unref(pSniffer->pConnection);
dbus_shutdown();

Note that dbus_g_connection_close and dbus_g_shutdown() don't exist.

Comment 3 Havoc Pennington 2006-09-21 08:14:41 UTC
The simplest possible explanation is a reference count leak somewhere, can you 
look at the refcount in the connection in the debugger (break on 
dbus_shutdown) and see what it is, then track down where any existing count 
came from... ?
Comment 4 frederic heem 2006-09-21 08:26:56 UTC
Before calling dbus_g_connection_unref, refcount is 3.
Setting a breakpoint on dbus_g_connection_ref shows who is the culprit, here 
are the 3 backtraces:
First backtrace
#0  dbus_connection_ref (connection=0x936dfb0) at dbus-connection.c:1743
#1  0x007fe5dc in internal_bus_get (type=DBUS_BUS_SYSTEM, error=0xbfd2403c, 
private=0) at dbus-bus.c:419
#2  0x00bbd539 in dbus_g_bus_get (type=DBUS_BUS_SYSTEM, error=0xbfd24098) at 
dbus-gmain.c:769
#3  0x0805230a in sniffer_dbus_register_service (pSniffer=0x936ac00) at 
dbus/wireshark-dbus-sniffer.c:352
#4  0x080530f5 in sniffer_dbus_init (pCaptureOptions=0x8058c80) at 
dbus/wireshark-dbus-sniffer.c:252
#5  0x080504f1 in main (argc=2, argv=0xbfd241d4) at dumpcap.c:381

Second backtrace:
#0  dbus_connection_ref (connection=0x936dfb0) at dbus-connection.c:1743
#1  0x00bbc2f5 in dbus_g_connection_ref (gconnection=0x936dfb4) at 
dbus-glib.c:64
#2  0x0014d2d2 in boxed_proxy_collect_value (value=0x937127c, 
n_collect_values=1, collect_values=0xbfd23f9c, collect_flags=0) at 
gboxed.c:306
#3  0x00154e5d in IA__g_object_new_valist (object_type=154577104, 
first_property_name=0xbd2076 "name", var_args=0xbfd24044 "") at gobject.c:984
#4  0x00154fe0 in IA__g_object_new (object_type=154577104, 
first_property_name=0xbd2076 "name") at gobject.c:793
#5  0x00bc4308 in dbus_g_proxy_new (connection=0x936dfb4, 
name=0x80569a9 "org.freedesktop.DBus", 
path_name=0x80569be "/org/freedesktop/DBus", 
interface_name=0x80569a9 "org.freedesktop.DBus") at dbus-gproxy.c:1824
#6  0x08052358 in sniffer_dbus_register_service (pSniffer=0x936ac00) at 
dbus/wireshark-dbus-sniffer.c:367
#7  0x080530f5 in sniffer_dbus_init (pCaptureOptions=0x8058c80) at 
dbus/wireshark-dbus-sniffer.c:252
#8  0x080504f1 in main (argc=2, argv=0xbfd241d4) at dumpcap.c:381

Third backtrace:
#0  dbus_connection_ref (connection=0x936dfb0) at dbus-connection.c:1743
#1  0x00bbc2f5 in dbus_g_connection_ref (gconnection=0x936dfb4) at 
dbus-glib.c:64
#2  0x0014d390 in boxed_proxy_value_copy (src_value=0x937127c, 
dest_value=0xbfd23dd8) at gboxed.c:273
#3  0x001723fa in IA__g_value_copy (src_value=0x937127c, 
dest_value=0xbfd23dd8) at gvalue.c:119
#4  0x001724ff in IA__g_value_transform (src_value=0x937127c, 
dest_value=0xbfd23dd8) at gvalue.c:341
#5  0x0015683d in g_object_constructor (type=154577104, 
n_construct_properties=4, construct_params=0x9371438) at gobject.c:677
#6  0x00bc5c4e in dbus_g_proxy_constructor (type=154577104, 
n_construct_properties=4, construct_properties=0x9371418) at 
dbus-gproxy.c:1311
#7  0x0015432a in IA__g_object_newv (object_type=154577104, n_parameters=4, 
parameters=0x9371230) at gobject.c:912
#8  0x00154ed9 in IA__g_object_new_valist (object_type=154577104, 
first_property_name=0xbd2076 "name", var_args=0xbfd24044 "") at gobject.c:996
#9  0x00154fe0 in IA__g_object_new (object_type=154577104, 
first_property_name=0xbd2076 "name") at gobject.c:793
#10 0x00bc4308 in dbus_g_proxy_new (connection=0x936dfb4, 
name=0x80569a9 "org.freedesktop.DBus", 
path_name=0x80569be "/org/freedesktop/DBus", 
interface_name=0x80569a9 "org.freedesktop.DBus") at dbus-gproxy.c:1824
#11 0x08052358 in sniffer_dbus_register_service (pSniffer=0x936ac00) at 
dbus/wireshark-dbus-sniffer.c:367
#12 0x080530f5 in sniffer_dbus_init (pCaptureOptions=0x8058c80) at 
dbus/wireshark-dbus-sniffer.c:252
#13 0x080504f1 in main (argc=2, argv=0xbfd241d4) at dumpcap.c:381
Comment 5 John (J5) Palmieri 2006-09-24 08:33:17 UTC
The actuall explination is we hold a ref on the shared connection.  Assert
should be changed to connection->refcount <= 1 or connection->refcount == 1
since it actually be an error if it is == 0 and still in the cache.
Comment 6 John (J5) Palmieri 2006-09-24 08:42:50 UTC
Actually is is a little more complicated than that.  Since we are talking about
shared known connections (i.e. SESSION, SYSTEM and STARTER bus) we might have
two refs, one for the shared connection cache and one for the known bus array. 
Not sure what the best course of action is but perhaps have the shutdown
functions unref and then have a check afterwards.
Comment 7 John (J5) Palmieri 2006-09-24 09:20:06 UTC
*** Bug 8377 has been marked as a duplicate of this bug. ***
Comment 8 John (J5) Palmieri 2006-09-24 09:38:28 UTC
*** Bug 8367 has been marked as a duplicate of this bug. ***
Comment 9 Havoc Pennington 2006-09-24 14:53:59 UTC
shutdown will need to disconnect the shared connections and drop refs to 
them...

Comment 10 John (J5) Palmieri 2006-10-07 09:11:31 UTC
havoc, did one of your commits fix this?
Comment 11 Havoc Pennington 2006-10-10 06:12:13 UTC
Yes, this is probably fixed now. I changed the dbus-bus.c ref to be a weak ref.
Comment 12 John (J5) Palmieri 2006-10-11 08:12:28 UTC
Havoc says fixed.  Please reopen if it is not
Comment 13 frederic heem 2006-10-16 05:53:15 UTC
The bug still exists in the RC2.
Comment 14 Havoc Pennington 2006-10-16 08:45:57 UTC
the only real way to debug this is to write a simple compilable test case 
program, anyone want to do that ;-)

It could even go in the test suite, it would be useful to just run a little 
app that opens a bus connection, calls some methods and sends some signals, 
then dbus_shutdown(), just to verify that the shutdown works and nothing 
crashes.
Comment 15 John (J5) Palmieri 2006-10-16 09:17:41 UTC
It is actually written but we clean up the connections ourselves with
test_connection_shutdown so we never hit the case.  Taking
test_connection_shutdown out of test/test-service.c should give us the testcase
we need.
Comment 16 John (J5) Palmieri 2006-10-16 16:32:16 UTC
Actually I can't reproduce this with RC2 (in fact I can ref a connection
numerous time and it won't complain when dbus_shutdown is called).  Please file
a test case with expected and actuall results placed in the comments.
Comment 17 frederic heem 2006-10-17 03:01:13 UTC
The problem seems to be in glib binding. dbus_g_proxy_new is calling 2 times 
dbus_g_connection_ref
Here is the 2 backtrace when setting a breakpoint in dbus_g_connection_ref:
bt 1:
#0  dbus_g_connection_ref (gconnection=0x97d5bf4) at dbus-glib.c:66
#1  0x0014d2d2 in boxed_proxy_collect_value (value=0x97d7dac, 
n_collect_values=1, collect_values=0xbf9638cc, collect_flags=0) at 
gboxed.c:306
#2  0x00154e5d in IA__g_object_new_valist (object_type=159204112, 
first_property_name=0x5ae10e "name", var_args=0xbf963974 "") at gobject.c:984
#3  0x00154fe0 in IA__g_object_new (object_type=159204112, 
first_property_name=0x5ae10e "name") at gobject.c:793
#4  0x005a0358 in dbus_g_proxy_new (connection=0x97d5bf4, 
name=0x649f3f "org.freedesktop.DBus", 
path_name=0x649f54 "/org/freedesktop/DBus", 
interface_name=0x649f3f "org.freedesktop.DBus") at dbus-gproxy.c:1824
#5  0x00648247 in sniffer_dbus_register_service (pSniffer=0x97d42c8) 
at /home/heefre/software/dbus/wireshark/dbus/wireshark-dbus-sniffer.c:373
#6  0x00647ded in sniffer_dbus_init (pCaptureOptions=0x80553e0) 
at /home/heefre/software/dbus/wireshark/dbus/wireshark-dbus-sniffer.c:255
#7  0x0804fe29 in main (argc=2, argv=0xbf963b14) 
at /home/heefre/software/dbus/wireshark/dumpcap.c:381

bt2:
#0  dbus_g_connection_ref (gconnection=0x97d5bf4) at dbus-glib.c:66
#1  0x0014d390 in boxed_proxy_value_copy (src_value=0x97d7dac, 
dest_value=0xbf963708) at gboxed.c:273
#2  0x001723fa in IA__g_value_copy (src_value=0x97d7dac, 
dest_value=0xbf963708) at gvalue.c:119
#3  0x001724ff in IA__g_value_transform (src_value=0x97d7dac, 
dest_value=0xbf963708) at gvalue.c:341
#4  0x0015683d in g_object_constructor (type=159204112, 
n_construct_properties=4, construct_params=0x97d5f80) at gobject.c:677
#5  0x005a1c9e in dbus_g_proxy_constructor (type=159204112, 
n_construct_properties=4, construct_properties=0x97d5f60) at 
dbus-gproxy.c:1311
#6  0x0015432a in IA__g_object_newv (object_type=159204112, n_parameters=4, 
parameters=0x97d7d60) at gobject.c:912
#7  0x00154ed9 in IA__g_object_new_valist (object_type=159204112, 
first_property_name=0x5ae10e "name", var_args=0xbf963974 "") at gobject.c:996
#8  0x00154fe0 in IA__g_object_new (object_type=159204112, 
first_property_name=0x5ae10e "name") at gobject.c:793
#9  0x005a0358 in dbus_g_proxy_new (connection=0x97d5bf4, 
name=0x649f3f "org.freedeskt
op.DBus", path_name=0x649f54 "/org/freedesktop/DBus", 
interface_name=0x649f3f "org.freedesktop.DBus") at dbus-gproxy.c:1824
#10 0x00648247 in sniffer_dbus_register_service (pSniffer=0x97d42c8) 
at /home/heefre/software/dbus/wireshark/dbus/wireshark-dbus-sniffer.c:373
#11 0x00647ded in sniffer_dbus_init (pCaptureOptions=0x80553e0) 
at /home/heefre/software/dbus/wireshark/dbus/wireshark-dbus-sniffer.c:255
#12 0x0804fe29 in main (argc=2, argv=0xbf963b14) 
at /home/heefre/software/dbus/wireshark/dumpcap.c:381


Comment 18 frederic heem 2006-10-17 03:29:09 UTC
If dbus_g_connection_unref is called 2 times, the second 
dbus_g_connection_unref complains that the transport is still connected:
_dbus_warn ("The last reference on a connection was dropped without closing 
the connection. This is a bug. See dbus_connection_unref() documentation for 
details.\n");


Comment 19 Havoc Pennington 2006-11-24 02:12:57 UTC
From the list:

It appears the problem is that in dbus-connection.c if a bus address has 
no "guid" property (as the default system bus address does not), we don't 
record the connection in the hash table of shared connections. Thus on 
shutdown in dbus-connection.c:shared_connections_shutdown we don't disconnect 
the connection, resulting in a leak.

The fix is pretty simple; keep a list of shared connections that lack a guid 
and be sure each shared connection is either in the list or in the hash.
Comment 20 Kimmo Hämäläinen 2007-10-26 07:04:43 UTC
Created attachment 12215 [details] [review]
proposed patch

Something like this? Seems to work.
Comment 21 Havoc Pennington 2007-10-26 09:41:15 UTC
Thanks, some nitpicks on this one:

- no_guid_connections I would rename to shared_connections_no_guid or something, to show the relationship to the shared_connections variable

- the unlock/lock should not be inside connection_shutdown_helper because it keeps the locks from being visibly paired (i.e. all lock/unlock pairs should be visible in the same function). Instead just keep the unlock and lock where they were.

- let's call connection_shutdown_helper something like close_connection_on_shutdown
 
Comment 22 Kimmo Hämäläinen 2007-10-29 00:11:17 UTC
Created attachment 12242 [details] [review]
fixed patch

ok, here
Comment 23 Kimmo Hämäläinen 2007-10-30 03:12:12 UTC
Should a guidless connection also be reusable? For example, if an application uses lot of libraries that all get their own system bus connections.
Comment 24 Havoc Pennington 2007-10-30 07:58:05 UTC
without the guid, we would have no way to know the connections are the same.
Comment 25 Kimmo Hämäläinen 2007-10-30 08:01:35 UTC
It seems that at least system bus connections are reused. I tested with multiple dbus_bus_get(DBUS_BUS_SYSTEM, &err) calls in a program.
Comment 26 Havoc Pennington 2007-10-30 08:09:26 UTC
That's a different thing (the dbus_bus_get mechanism juse uses a global variable in dbus-bus.c, vs. shared connections in dbus-connection.c)
Comment 27 Simon McVittie 2011-04-07 09:47:25 UTC
This appears to have been fixed since dbus-1.1.20, back in 2008, by applying Kimmo's patch (commit 381c8548b2d).