From a2129f7cccaf0265fffe0da79ca0510b6e01131b Mon Sep 17 00:00:00 2001 From: Havoc Pennington Date: Sun, 1 Oct 2006 15:36:19 +0000 Subject: 2006-10-01 Havoc Pennington * dbus/dbus-connection.c (_dbus_connection_close_if_only_one_ref): Add a hack to make DBusNewConnectionFunction work right. * dbus/dbus-server-socket.c (handle_new_client_fd_and_unlock): use the hack here. Also, fix the todo about refcount leak. * dbus/dbus-server-debug-pipe.c (_dbus_transport_debug_pipe_new): and use the hack here * dbus/dbus-connection.c: Kill the "shared" flag vs. the "shareable" flag; this was completely broken, since it meant dbus_connection_open() returned a connection of unknown shared-ness. Now, we always hold a ref on anything opened as shareable. Move the call to notify dbus-bus.c into connection_forget_shared_unlocked, so libdbus consistently forgets all its knowledge of a connection at once. This exposed numerous places where things were totally broken if we dropped a ref inside get_dispatch_status_unlocked where connection_forget_shared_unlocked was previously, so move connection_forget_shared_unlocked into _dbus_connection_update_dispatch_status_and_unlock. Also move the exit_on_disconnect here. (shared_connections_shutdown): this assumed weak refs to the shared connections; since we have strong refs now, the assertion was failing and stuff was left in the hash. Fix it to close still-open shared connections. * bus/dispatch.c: fixup to use dbus_connection_open_private on the debug pipe connections * dbus/dbus-connection.c (dbus_connection_dispatch): only notify dbus-bus.c if the closed connection is in fact shared (_dbus_connection_close_possibly_shared): rename from _dbus_connection_close_internal (dbus_connection_close, dbus_connection_open, dbus_connection_open_private): Improve docs to explain the deal with when you should close or unref or both * dbus/dbus-bus.c (_dbus_bus_notify_shared_connection_disconnected_unlocked): rename from _dbus_bus_check_connection_and_unref_unlocked and modify to loop over all connections * test/test-utils.c (test_connection_shutdown): don't try to close shared connections. * test/name-test/test-threads-init.c (main): fix warnings in here * dbus/dbus-sysdeps.c (_dbus_abort): support DBUS_BLOCK_ON_ABORT env variable to cause blocking waiting for gdb; drop DBUS_PRINT_BACKTRACE and just call _dbus_print_backtrace() unconditionally. * configure.in: add -export-dynamic to libtool flags if assertions enabled so _dbus_print_backtrace works. * dbus/dbus-sysdeps-unix.c (_dbus_print_backtrace): use fprintf instead of _dbus_verbose to print the backtrace, and diagnose lack of -rdynamic/-export-dynamic --- dbus/Makefile.am | 5 +- dbus/dbus-bus.c | 81 +++++--- dbus/dbus-bus.h | 2 +- dbus/dbus-connection-internal.h | 3 +- dbus/dbus-connection.c | 443 +++++++++++++++++++++++++++++----------- dbus/dbus-internals.c | 20 +- dbus/dbus-server-debug-pipe.c | 1 + dbus/dbus-server-socket.c | 11 +- dbus/dbus-server.c | 10 +- dbus/dbus-sysdeps-unix.c | 18 +- dbus/dbus-sysdeps.c | 16 +- 11 files changed, 441 insertions(+), 169 deletions(-) (limited to 'dbus') diff --git a/dbus/Makefile.am b/dbus/Makefile.am index ba326c14..d9d588aa 100644 --- a/dbus/Makefile.am +++ b/dbus/Makefile.am @@ -164,7 +164,9 @@ noinst_LTLIBRARIES=libdbus-convenience.la libdbus_1_la_LIBADD= $(DBUS_CLIENT_LIBS) ## don't export symbols that start with "_" (we use this ## convention for internal symbols) -libdbus_1_la_LDFLAGS= -export-symbols-regex "^[^_].*" -version-info $(LT_CURRENT):$(LT_REVISION):$(LT_AGE) -no-undefined +libdbus_1_la_LDFLAGS= -export-symbols-regex "^[^_].*" -version-info $(LT_CURRENT):$(LT_REVISION):$(LT_AGE) -no-undefined @R_DYNAMIC_LDFLAG@ + +libdbus_convenience_la_LDFLAGS=@R_DYNAMIC_LDFLAG@ ## note that TESTS has special meaning (stuff to use in make check) ## so if adding tests not to be run in make check, don't add them to @@ -184,6 +186,7 @@ dbus_test_SOURCES= \ dbus-test-main.c dbus_test_LDADD=libdbus-convenience.la +dbus_test_LDFLAGS=@R_DYNAMIC_LDFLAG@ ## mop up the gcov files clean-local: diff --git a/dbus/dbus-bus.c b/dbus/dbus-bus.c index a2336786..bc8c107f 100644 --- a/dbus/dbus-bus.c +++ b/dbus/dbus-bus.c @@ -158,6 +158,7 @@ init_connections_unlocked (void) /* Use default system bus address if none set in environment */ bus_connection_addresses[DBUS_BUS_SYSTEM] = _dbus_strdup (DBUS_SYSTEM_BUS_DEFAULT_ADDRESS); + if (bus_connection_addresses[DBUS_BUS_SYSTEM] == NULL) return FALSE; @@ -179,7 +180,8 @@ init_connections_unlocked (void) if (bus_connection_addresses[DBUS_BUS_SESSION] == NULL) bus_connection_addresses[DBUS_BUS_SESSION] = _dbus_strdup (DBUS_SESSION_BUS_DEFAULT_ADDRESS); - if (bus_connection_addresses[DBUS_BUS_SESSION] == NULL) + + if (bus_connection_addresses[DBUS_BUS_SESSION] == NULL) return FALSE; _dbus_verbose (" \"%s\"\n", bus_connection_addresses[DBUS_BUS_SESSION] ? @@ -310,27 +312,32 @@ ensure_bus_data (DBusConnection *connection) return bd; } -/* internal function that checks to see if this - is a shared connection owned by the bus and if it is unref it */ +/** + * Internal function that checks to see if this + * is a shared connection owned by the bus and if it is unref it. + * + * @param connection a connection that has been disconnected. + */ void -_dbus_bus_check_connection_and_unref_unlocked (DBusConnection *connection) +_dbus_bus_notify_shared_connection_disconnected_unlocked (DBusConnection *connection) { + int i; + _DBUS_LOCK (bus); - if (bus_connections[DBUS_BUS_SYSTEM] == connection) - { - bus_connections[DBUS_BUS_SYSTEM] = NULL; - _dbus_connection_unref_unlocked (connection); - } - else if (bus_connections[DBUS_BUS_SESSION] == connection) - { - bus_connections[DBUS_BUS_SESSION] = NULL; - _dbus_connection_unref_unlocked (connection); - } - else if (bus_connections[DBUS_BUS_STARTER] == connection) + /* We are expecting to have the connection saved in only one of these + * slots, but someone could in a pathological case set system and session + * bus to the same bus or something. Or set one of them to the starter + * bus without setting the starter bus type in the env variable. + * So we don't break the loop as soon as we find a match. + */ + for (i = 0; i < N_BUS_TYPES; ++i) { - bus_connections[DBUS_BUS_STARTER] = NULL; - _dbus_connection_unref_unlocked (connection); + if (bus_connections[i] == connection) + { + bus_connections[i] = NULL; + _dbus_connection_unref_unlocked (connection); + } } _DBUS_UNLOCK (bus); @@ -392,7 +399,7 @@ internal_bus_get (DBusBusType type, } if (private) - connection = dbus_connection_open_private(address, error); + connection = dbus_connection_open_private (address, error); else connection = dbus_connection_open (address, error); @@ -412,7 +419,7 @@ internal_bus_get (DBusBusType type, if (!dbus_bus_register (connection, error)) { _DBUS_ASSERT_ERROR_IS_SET (error); - _dbus_connection_close_internal (connection); + _dbus_connection_close_possibly_shared (connection); dbus_connection_unref (connection); _DBUS_UNLOCK (bus); @@ -432,6 +439,8 @@ internal_bus_get (DBusBusType type, bd->is_well_known = TRUE; _DBUS_UNLOCK (bus); + + /* Return a reference to the caller */ return connection; } @@ -446,11 +455,22 @@ internal_bus_get (DBusBusType type, /** * Connects to a bus daemon and registers the client with it. If a * connection to the bus already exists, then that connection is - * returned. Caller owns a reference to the bus. + * returned. The caller of this function owns a reference to the bus. + * + * The caller may NOT call dbus_connection_close() on this connection; + * see dbus_connection_open() and dbus_connection_close() for details + * on that. * + * If this function obtains a new connection object never before + * returned from dbus_bus_get(), it will call + * dbus_connection_set_exit_on_disconnect(), so the application + * will exit if the connection closes. You can undo this + * by calling dbus_connection_set_exit_on_disconnect() yourself + * after you get the connection. + * * @param type bus type * @param error address where an error can be returned. - * @returns a DBusConnection with new ref + * @returns a #DBusConnection with new ref */ DBusConnection * dbus_bus_get (DBusBusType type, @@ -460,10 +480,20 @@ dbus_bus_get (DBusBusType type, } /** - * Connects to a bus daemon and registers the client with it. Unlike - * dbus_bus_get(), always creates a new connection. This connection + * Connects to a bus daemon and registers the client with it as with dbus_bus_register(). + * Unlike dbus_bus_get(), always creates a new connection. This connection * will not be saved or recycled by libdbus. Caller owns a reference - * to the bus. + * to the bus and must either close it or know it to be closed + * prior to releasing this reference. + * + * See dbus_connection_open_private() for more details on when to + * close and unref this connection. + * + * This function calls + * dbus_connection_set_exit_on_disconnect() on the new connection, so the application + * will exit if the connection closes. You can undo this + * by calling dbus_connection_set_exit_on_disconnect() yourself + * after you get the connection. * * @param type bus type * @param error address where an error can be returned. @@ -481,6 +511,9 @@ dbus_bus_get_private (DBusBusType type, * thing an application does when connecting to the message bus. * If registration succeeds, the unique name will be set, * and can be obtained using dbus_bus_get_unique_name(). + * + * If you use dbus_bus_get() or dbus_bus_get_private() this + * function will be called for you. * * @param connection the connection * @param error place to store errors diff --git a/dbus/dbus-bus.h b/dbus/dbus-bus.h index 6cefe353..b4933af3 100644 --- a/dbus/dbus-bus.h +++ b/dbus/dbus-bus.h @@ -68,7 +68,7 @@ void dbus_bus_remove_match (DBusConnection *connection, const char *rule, DBusError *error); -void _dbus_bus_check_connection_and_unref_unlocked (DBusConnection *connection); +void _dbus_bus_notify_shared_connection_disconnected_unlocked (DBusConnection *connection); DBUS_END_DECLS diff --git a/dbus/dbus-connection-internal.h b/dbus/dbus-connection-internal.h index 6f42e168..55775d2e 100644 --- a/dbus/dbus-connection-internal.h +++ b/dbus/dbus-connection-internal.h @@ -77,7 +77,8 @@ DBusConnection* _dbus_connection_new_for_transport (DBusTransport void _dbus_connection_do_iteration_unlocked (DBusConnection *connection, unsigned int flags, int timeout_milliseconds); -void _dbus_connection_close_internal (DBusConnection *connection); +void _dbus_connection_close_possibly_shared (DBusConnection *connection); +void _dbus_connection_close_if_only_one_ref (DBusConnection *connection); DBusPendingCall* _dbus_pending_call_new (DBusConnection *connection, int timeout_milliseconds, diff --git a/dbus/dbus-connection.c b/dbus/dbus-connection.c index 254eb008..252b5e5e 100644 --- a/dbus/dbus-connection.c +++ b/dbus/dbus-connection.c @@ -238,9 +238,7 @@ struct DBusConnection char *server_guid; /**< GUID of server if we are in shared_connections, #NULL if server GUID is unknown or connection is private */ - unsigned int shareable : 1; /**< #TRUE if connection can go in shared_connections once we know the GUID */ - - unsigned int shared : 1; /** < #TRUE if connection is shared and we hold a ref to it */ + unsigned int shareable : 1; /**< #TRUE if libdbus owns a reference to the connection and can return it from dbus_connection_open() more than once */ unsigned int dispatch_acquired : 1; /**< Someone has dispatch path (can drain incoming queue) */ unsigned int io_path_acquired : 1; /**< Someone has transport io path (can use the transport to read/write messages) */ @@ -248,6 +246,14 @@ struct DBusConnection unsigned int exit_on_disconnect : 1; /**< If #TRUE, exit after handling disconnect signal */ unsigned int route_peer_messages : 1; /**< If #TRUE, if org.freedesktop.DBus.Peer messages have a bus name, don't handle them automatically */ + + unsigned int disconnected_message_arrived : 1; /**< We popped or are dispatching the disconnected message. + * if the disconnect_message_link is NULL then we queued it, but + * this flag is whether it got to the head of the queue. + */ + unsigned int disconnected_message_processed : 1; /**< We did our default handling of the disconnected message, + * such as closing the connection. + */ #ifndef DBUS_DISABLE_CHECKS unsigned int have_connection_lock : 1; /**< Used to check locking */ @@ -265,6 +271,8 @@ static void _dbus_connection_last_unref (DB static void _dbus_connection_acquire_dispatch (DBusConnection *connection); static void _dbus_connection_release_dispatch (DBusConnection *connection); static DBusDispatchStatus _dbus_connection_flush_unlocked (DBusConnection *connection); +static void _dbus_connection_close_possibly_shared_and_unlock (DBusConnection *connection); +static dbus_bool_t _dbus_connection_get_is_connected_unlocked (DBusConnection *connection); static DBusMessageFilter * _dbus_message_filter_ref (DBusMessageFilter *filter) @@ -366,11 +374,11 @@ _dbus_connection_queue_received_message (DBusConnection *connection, */ void _dbus_connection_test_get_locks (DBusConnection *conn, - DBusMutex **mutex_loc, - DBusMutex **dispatch_mutex_loc, - DBusMutex **io_path_mutex_loc, - DBusCondVar **dispatch_cond_loc, - DBusCondVar **io_path_cond_loc) + DBusMutex **mutex_loc, + DBusMutex **dispatch_mutex_loc, + DBusMutex **io_path_mutex_loc, + DBusCondVar **dispatch_cond_loc, + DBusCondVar **io_path_cond_loc) { *mutex_loc = conn->mutex; *dispatch_mutex_loc = conn->dispatch_mutex; @@ -1178,6 +1186,8 @@ _dbus_connection_new_for_transport (DBusTransport *transport) connection->exit_on_disconnect = FALSE; connection->shareable = FALSE; connection->route_peer_messages = FALSE; + connection->disconnected_message_arrived = FALSE; + connection->disconnected_message_processed = FALSE; #ifndef DBUS_DISABLE_CHECKS connection->generation = _dbus_current_generation; @@ -1365,10 +1375,42 @@ static DBusHashTable *shared_connections = NULL; static void shared_connections_shutdown (void *data) { + int n_entries; + _DBUS_LOCK (shared_connections); + + /* This is a little bit unpleasant... better ideas? */ + while ((n_entries = _dbus_hash_table_get_n_entries (shared_connections)) > 0) + { + DBusConnection *connection; + DBusMessage *message; + DBusHashIter iter; + + _dbus_hash_iter_init (shared_connections, &iter); + _dbus_hash_iter_next (&iter); + + connection = _dbus_hash_iter_get_value (&iter); - _dbus_assert (_dbus_hash_table_get_n_entries (shared_connections) == 0); + _DBUS_UNLOCK (shared_connections); + + dbus_connection_ref (connection); + _dbus_connection_close_possibly_shared (connection); + + /* Churn through to the Disconnected message */ + while ((message = dbus_connection_pop_message (connection))) + { + dbus_message_unref (message); + } + dbus_connection_unref (connection); + + _DBUS_LOCK (shared_connections); + /* The connection should now be dead and not in our hash ... */ + _dbus_assert (_dbus_hash_table_get_n_entries (shared_connections) < n_entries); + } + + _dbus_assert (_dbus_hash_table_get_n_entries (shared_connections) == 0); + _dbus_hash_table_unref (shared_connections); shared_connections = NULL; @@ -1419,23 +1461,35 @@ connection_lookup_shared (DBusAddressEntry *entry, if (guid != NULL) { - *result = _dbus_hash_table_lookup_string (shared_connections, - guid); + DBusConnection *connection; + + connection = _dbus_hash_table_lookup_string (shared_connections, + guid); - if (*result) + if (connection) { - /* The DBusConnection can't have been disconnected - * between the lookup and this code, because the - * disconnection will take the shared_connections lock to - * remove the connection. It can't have been finalized - * since you have to disconnect prior to finalize. - * - * Thus it's safe to ref the connection. + /* The DBusConnection can't be finalized without taking + * the shared_connections lock to remove it from the + * hash. So it's safe to ref the connection here. + * However, it may be disconnected if the Disconnected + * message hasn't been processed yet, in which case we + * want to pretend it isn't in the hash and avoid + * returning it. */ - dbus_connection_ref (*result); - - _dbus_verbose ("looked up existing connection to server guid %s\n", - guid); + CONNECTION_LOCK (connection); + if (_dbus_connection_get_is_connected_unlocked (connection)) + { + _dbus_connection_ref_unlocked (connection); + *result = connection; + _dbus_verbose ("looked up existing connection to server guid %s\n", + guid); + } + else + { + _dbus_verbose ("looked up existing connection to server guid %s but it was disconnected so ignoring it\n", + guid); + } + CONNECTION_UNLOCK (connection); } } @@ -1451,14 +1505,24 @@ connection_record_shared_unlocked (DBusConnection *connection, char *guid_key; char *guid_in_connection; + HAVE_LOCK_CHECK (connection); + _dbus_assert (connection->server_guid == NULL); + _dbus_assert (connection->shareable); + + /* get a hard ref on this connection, even if + * we won't in fact store it in the hash, we still + * need to hold a ref on it until it's disconnected. + */ + _dbus_connection_ref_unlocked (connection); + + if (guid == NULL) + return TRUE; /* don't store in the hash */ + /* A separate copy of the key is required in the hash table, because * we don't have a lock on the connection when we are doing a hash * lookup. */ - _dbus_assert (connection->server_guid == NULL); - _dbus_assert (connection->shareable); - guid_key = _dbus_strdup (guid); if (guid_key == NULL) return FALSE; @@ -1483,10 +1547,6 @@ connection_record_shared_unlocked (DBusConnection *connection, } connection->server_guid = guid_in_connection; - connection->shared = TRUE; - - /* get a hard ref on this connection */ - dbus_connection_ref (connection); _dbus_verbose ("stored connection to %s to be shared\n", connection->server_guid); @@ -1502,25 +1562,30 @@ static void connection_forget_shared_unlocked (DBusConnection *connection) { HAVE_LOCK_CHECK (connection); - - if (connection->server_guid == NULL) - return; - _dbus_verbose ("dropping connection to %s out of the shared table\n", - connection->server_guid); + if (!connection->shareable) + return; - _DBUS_LOCK (shared_connections); + if (connection->server_guid != NULL) + { + _dbus_verbose ("dropping connection to %s out of the shared table\n", + connection->server_guid); + + _DBUS_LOCK (shared_connections); + + if (!_dbus_hash_table_remove_string (shared_connections, + connection->server_guid)) + _dbus_assert_not_reached ("connection was not in the shared table"); + + dbus_free (connection->server_guid); + connection->server_guid = NULL; + _DBUS_UNLOCK (shared_connections); + } - if (!_dbus_hash_table_remove_string (shared_connections, - connection->server_guid)) - _dbus_assert_not_reached ("connection was not in the shared table"); + _dbus_bus_notify_shared_connection_disconnected_unlocked (connection); - dbus_free (connection->server_guid); - connection->server_guid = NULL; - - /* remove the hash ref */ + /* remove our reference held on all shareable connections */ _dbus_connection_unref_unlocked (connection); - _DBUS_UNLOCK (shared_connections); } static DBusConnection* @@ -1603,36 +1668,42 @@ _dbus_connection_open_internal (const char *address, { connection = connection_try_from_address_entry (entries[i], &tmp_error); - - if (connection != NULL && shared) - { - const char *guid; - - connection->shareable = TRUE; - - guid = dbus_address_entry_get_value (entries[i], "guid"); - /* we don't have a connection lock but we know nobody - * else has a handle to the connection - */ - - if (guid && - !connection_record_shared_unlocked (connection, guid)) + if (connection != NULL) + { + CONNECTION_LOCK (connection); + + if (shared) { - _DBUS_SET_OOM (&tmp_error); - _dbus_connection_close_internal (connection); - dbus_connection_unref (connection); - connection = NULL; + const char *guid; + + connection->shareable = TRUE; + + /* guid may be NULL */ + guid = dbus_address_entry_get_value (entries[i], "guid"); + + if (!connection_record_shared_unlocked (connection, guid)) + { + _DBUS_SET_OOM (&tmp_error); + _dbus_connection_close_possibly_shared_and_unlock (connection); + dbus_connection_unref (connection); + connection = NULL; + } + + /* Note: as of now the connection is possibly shared + * since another thread could have pulled it from the table. + * However, we still have it locked so that thread isn't + * doing anything more than blocking on the lock. + */ } - - /* but as of now the connection is possibly shared - * since another thread could have pulled it from the table - */ } } if (connection) - break; + { + HAVE_LOCK_CHECK (connection); + break; + } _DBUS_ASSERT_ERROR_IS_SET (&tmp_error); @@ -1641,8 +1712,6 @@ _dbus_connection_open_internal (const char *address, else dbus_error_free (&tmp_error); } - - /* NOTE we don't have a lock on a possibly-shared connection object */ _DBUS_ASSERT_ERROR_IS_CLEAR (error); _DBUS_ASSERT_ERROR_IS_CLEAR (&tmp_error); @@ -1655,6 +1724,8 @@ _dbus_connection_open_internal (const char *address, else { dbus_error_free (&first_error); + + CONNECTION_UNLOCK (connection); } dbus_address_entries_free (entries); @@ -1683,6 +1754,10 @@ _dbus_connection_open_internal (const char *address, * reason for the failure in the error parameter. Pass #NULL for the * error parameter if you aren't interested in the reason for * failure. + * + * Because this connection is shared, no user of the connection + * may call dbus_connection_close(). However, when you are done with the + * connection you should call dbus_connection_unref(). * * @param address the address. * @param error address where an error can be returned. @@ -1713,6 +1788,14 @@ dbus_connection_open (const char *address, * reason for the failure in the error parameter. Pass #NULL for the * error parameter if you aren't interested in the reason for * failure. + * + * When you are done with this connection, you must + * dbus_connection_close() to disconnect it, + * and dbus_connection_unref() to free the connection object. + * + * (The dbus_connection_close() can be skipped if the + * connection is already known to be disconnected, for example + * if you are inside a handler for the Disconnected signal.) * * @param address the address. * @param error address where an error can be returned. @@ -1869,12 +1952,19 @@ _dbus_connection_last_unref (DBusConnection *connection) /** * Decrements the reference count of a DBusConnection, and finalizes - * it if the count reaches zero. It is a bug to drop the last reference - * to a connection that has not been disconnected. + * it if the count reaches zero. + * + * Note: it is a bug to drop the last reference to a connection that + * is still connected. * - * @todo in practice it can be quite tricky to never unref a connection - * that's still connected; maybe there's some way we could avoid - * the requirement. + * For shared connections, libdbus will own a reference + * as long as the connection is connected, so you can know that either + * you don't have the last reference, or it's OK to drop the last reference. + * Most connections are shared. + * + * For private connections, the creator of the connection must arrange for + * dbus_connection_close() to be called prior to dropping the last reference. + * Private connections come from dbus_connection_open_private() or dbus_bus_get_private(). * * @param connection the connection. */ @@ -1912,11 +2002,19 @@ dbus_connection_unref (DBusConnection *connection) } static void -_dbus_connection_close_internal_and_unlock (DBusConnection *connection) +_dbus_connection_close_possibly_shared_and_unlock (DBusConnection *connection) { DBusDispatchStatus status; + + HAVE_LOCK_CHECK (connection); _dbus_verbose ("Disconnecting %p\n", connection); + + /* We need to ref because update_dispatch_status_and_unlock will unref + * the connection if it was shared and libdbus was the only remaining + * refcount holder. + */ + _dbus_connection_ref_unlocked (connection); _dbus_transport_disconnect (connection->transport); @@ -1925,34 +2023,62 @@ _dbus_connection_close_internal_and_unlock (DBusConnection *connection) /* this calls out to user code */ _dbus_connection_update_dispatch_status_and_unlock (connection, status); + + /* could also call out to user code */ + dbus_connection_unref (connection); } void -_dbus_connection_close_internal (DBusConnection *connection) +_dbus_connection_close_possibly_shared (DBusConnection *connection) { _dbus_assert (connection != NULL); _dbus_assert (connection->generation == _dbus_current_generation); CONNECTION_LOCK (connection); - _dbus_connection_close_internal_and_unlock (connection); + _dbus_connection_close_possibly_shared_and_unlock (connection); } /** - * Closes the connection, so no further data can be sent or received. - * Any further attempts to send data will result in errors. This - * function does not affect the connection's reference count. It's - * safe to disconnect a connection more than once; all calls after the + * Closes a private connection, so no further data can be sent or received. + * This disconnects the transport (such as a socket) underlying the + * connection. + * + * Attempts to send messages after closing a connection are safe, but will result in + * error replies generated locally in libdbus. + * + * This function does not affect the connection's reference count. It's + * safe to close a connection more than once; all calls after the * first do nothing. It's impossible to "reopen" a connection, a * new connection must be created. This function may result in a call * to the DBusDispatchStatusFunction set with * dbus_connection_set_dispatch_status_function(), as the disconnect * message it generates needs to be dispatched. * - * If the connection is a shared connection we print out a warning that - * you can not close shared connection and we return. Internal calls - * should use _dbus_connection_close_internal() to close shared connections. - * - * @param connection the connection. + * If a connection is dropped by the remote application, it will + * close itself. + * + * You must close a connection prior to releasing the last reference to + * the connection. If you dbus_connection_unref() for the last time + * without closing the connection, the results are undefined; it + * is a bug in your program and libdbus will try to print a warning. + * + * You may not close a shared connection. Connections created with + * dbus_connection_open() or dbus_bus_get() are shared. + * These connections are owned by libdbus, and applications should + * only unref them, never close them. Applications can know it is + * safe to unref these connections because libdbus will be holding a + * reference as long as the connection is open. Thus, either the + * connection is closed and it is OK to drop the last reference, + * or the connection is open and the app knows it does not have the + * last reference. + * + * Connections created with dbus_connection_open_private() or + * dbus_bus_get_private() are not kept track of or referenced by + * libdbus. The creator of these connections is responsible for + * calling dbus_connection_close() prior to releasing the last + * reference, if the connection is not already disconnected. + * + * @param connection the private (unshared) connection to close */ void dbus_connection_close (DBusConnection *connection) @@ -1962,15 +2088,52 @@ dbus_connection_close (DBusConnection *connection) CONNECTION_LOCK (connection); - if (connection->shared) + if (connection->shareable) { CONNECTION_UNLOCK (connection); - _dbus_warn ("Applications can not close shared connections. Please fix this in your app. Ignoring close request and continuing."); + _dbus_warn ("Applications must not close shared connections - see dbus_connection_close() docs. This is a bug in the application.\n"); return; } - _dbus_connection_close_internal_and_unlock (connection); + _dbus_connection_close_possibly_shared_and_unlock (connection); +} + +/** + * Used internally to handle the semantics of dbus_server_set_new_connection_function(). + * If the new connection function does not ref the connection, we want to close it. + * + * A bit of a hack, probably the new connection function should have returned a value + * for whether to close, or should have had to close the connection itself if it + * didn't want it. + * + * But, this works OK as long as the new connection function doesn't do anything + * crazy like keep the connection around without ref'ing it. + * + * We have to lock the connection across refcount check and close in case + * the new connection function spawns a thread that closes and unrefs. + * In that case, if the app thread + * closes and unrefs first, we'll harmlessly close again; if the app thread + * still has the ref, we'll close and then the app will close harmlessly. + * If the app unrefs without closing, the app is broken since if the + * app refs from the new connection function it is supposed to also close. + * + * If we didn't atomically check the refcount and close with the lock held + * though, we could screw this up. + * + * @param connection the connection + */ +void +_dbus_connection_close_if_only_one_ref (DBusConnection *connection) +{ + CONNECTION_LOCK (connection); + + _dbus_assert (connection->refcount.value > 0); + + if (connection->refcount.value == 1) + _dbus_connection_close_possibly_shared_and_unlock (connection); + else + CONNECTION_UNLOCK (connection); } static dbus_bool_t @@ -1981,11 +2144,14 @@ _dbus_connection_get_is_connected_unlocked (DBusConnection *connection) } /** - * Gets whether the connection is currently connected. All - * connections are connected when they are opened. A connection may + * Gets whether the connection is currently open. A connection may * become disconnected when the remote application closes its end, or * exits; a connection may also be disconnected with * dbus_connection_close(). + * + * There are not separate states for "closed" and "disconnected," the two + * terms are synonymous. This function should really be called + * get_is_open() but for historical reasons is not. * * @param connection the connection. * @returns #TRUE if the connection is still alive. @@ -2557,7 +2723,7 @@ connection_timeout_and_complete_all_pending_calls_unlocked (DBusConnection *conn _dbus_hash_iter_init (connection->pending_replies, &iter); _dbus_hash_iter_next (&iter); - pending = (DBusPendingCall *) _dbus_hash_iter_get_value (&iter); + pending = _dbus_hash_iter_get_value (&iter); _dbus_pending_call_ref_unlocked (pending); _dbus_pending_call_queue_timeout_error_unlocked (pending, @@ -3116,6 +3282,27 @@ dbus_connection_read_write (DBusConnection *connection, return _dbus_connection_read_write_dispatch(connection, timeout_milliseconds, FALSE); } +/* We need to call this anytime we pop the head of the queue, and then + * update_dispatch_status_and_unlock needs to be called afterward + * which will "process" the disconnected message and set + * disconnected_message_processed. + */ +static void +check_disconnected_message_arrived_unlocked (DBusConnection *connection, + DBusMessage *head_of_queue) +{ + HAVE_LOCK_CHECK (connection); + + /* checking that the link is NULL is an optimization to avoid the is_signal call */ + if (connection->disconnect_message_link == NULL && + dbus_message_is_signal (head_of_queue, + DBUS_INTERFACE_LOCAL, + "Disconnected")) + { + connection->disconnected_message_arrived = TRUE; + } +} + /** * Returns the first-received message from the incoming message queue, * leaving it in the queue. If the queue is empty, returns #NULL. @@ -3163,11 +3350,15 @@ dbus_connection_borrow_message (DBusConnection *connection) message = connection->message_borrowed; + check_disconnected_message_arrived_unlocked (connection, message); + /* Note that we KEEP the dispatch lock until the message is returned */ if (message == NULL) _dbus_connection_release_dispatch (connection); CONNECTION_UNLOCK (connection); + + /* We don't update dispatch status until it's returned or stolen */ return message; } @@ -3184,6 +3375,8 @@ void dbus_connection_return_message (DBusConnection *connection, DBusMessage *message) { + DBusDispatchStatus status; + _dbus_return_if_fail (connection != NULL); _dbus_return_if_fail (message != NULL); _dbus_return_if_fail (message == connection->message_borrowed); @@ -3195,9 +3388,10 @@ dbus_connection_return_message (DBusConnection *connection, connection->message_borrowed = NULL; - _dbus_connection_release_dispatch (connection); - - CONNECTION_UNLOCK (connection); + _dbus_connection_release_dispatch (connection); + + status = _dbus_connection_get_dispatch_status_unlocked (connection); + _dbus_connection_update_dispatch_status_and_unlock (connection, status); } /** @@ -3214,6 +3408,7 @@ dbus_connection_steal_borrowed_message (DBusConnection *connection, DBusMessage *message) { DBusMessage *pop_message; + DBusDispatchStatus status; _dbus_return_if_fail (connection != NULL); _dbus_return_if_fail (message != NULL); @@ -3235,8 +3430,9 @@ dbus_connection_steal_borrowed_message (DBusConnection *connection, connection->message_borrowed = NULL; _dbus_connection_release_dispatch (connection); - - CONNECTION_UNLOCK (connection); + + status = _dbus_connection_get_dispatch_status_unlocked (connection); + _dbus_connection_update_dispatch_status_and_unlock (connection, status); } /* See dbus_connection_pop_message, but requires the caller to own @@ -3271,6 +3467,8 @@ _dbus_connection_pop_message_link_unlocked (DBusConnection *connection) dbus_message_get_signature (link->data), connection, connection->n_incoming); + check_disconnected_message_arrived_unlocked (connection, link->data); + return link; } else @@ -3375,7 +3573,9 @@ dbus_connection_pop_message (DBusConnection *connection) _dbus_verbose ("Returning popped message %p\n", message); _dbus_connection_release_dispatch (connection); - CONNECTION_UNLOCK (connection); + + status = _dbus_connection_get_dispatch_status_unlocked (connection); + _dbus_connection_update_dispatch_status_and_unlock (connection, status); return message; } @@ -3476,12 +3676,12 @@ _dbus_connection_get_dispatch_status_unlocked (DBusConnection *connection) { _dbus_verbose ("Sending disconnect message from %s\n", _DBUS_FUNCTION_NAME); - - connection_forget_shared_unlocked (connection); - - /* If we have pending calls queued timeouts on disconnect */ + + /* If we have pending calls, queue their timeouts - we want the Disconnected + * to be the last message, after these timeouts. + */ connection_timeout_and_complete_all_pending_calls_unlocked (connection); - + /* We haven't sent the disconnect message already, * and all real messages have been queued up. */ @@ -3538,6 +3738,27 @@ _dbus_connection_update_dispatch_status_and_unlock (DBusConnection *connectio function = connection->dispatch_status_function; data = connection->dispatch_status_data; + if (connection->disconnected_message_arrived && + !connection->disconnected_message_processed) + { + connection->disconnected_message_processed = TRUE; + + /* this does an unref, but we have a ref + * so we should not run the finalizer here + * inside the lock. + */ + connection_forget_shared_unlocked (connection); + + if (connection->exit_on_disconnect) + { + CONNECTION_UNLOCK (connection); + + _dbus_verbose ("Exiting on Disconnected signal\n"); + _dbus_exit (1); + _dbus_assert_not_reached ("Call to exit() returned"); + } + } + /* We drop the lock */ CONNECTION_UNLOCK (connection); @@ -3981,22 +4202,6 @@ dbus_connection_dispatch (DBusConnection *connection) { _dbus_verbose (" ... done dispatching in %s\n", _DBUS_FUNCTION_NAME); - if (dbus_message_is_signal (message, - DBUS_INTERFACE_LOCAL, - "Disconnected")) - { - _dbus_bus_check_connection_and_unref_unlocked (connection); - - if (connection->exit_on_disconnect) - { - CONNECTION_UNLOCK (connection); - - _dbus_verbose ("Exiting on Disconnected signal\n"); - _dbus_exit (1); - _dbus_assert_not_reached ("Call to exit() returned"); - } - } - _dbus_list_free_link (message_link); dbus_message_unref (message); /* don't want the message to count in max message limits * in computing dispatch status below diff --git a/dbus/dbus-internals.c b/dbus/dbus-internals.c index 707b4005..ecddfb6f 100644 --- a/dbus/dbus-internals.c +++ b/dbus/dbus-internals.c @@ -190,6 +190,9 @@ */ const char _dbus_no_memory_message[] = "Not enough memory"; +static dbus_bool_t warn_initted = FALSE; +static dbus_bool_t fatal_warnings = FALSE; + /** * Prints a warning message to stderr. * @@ -199,12 +202,27 @@ void _dbus_warn (const char *format, ...) { - /* FIXME not portable enough? */ va_list args; + if (!warn_initted) + { + const char *s; + s = _dbus_getenv ("DBUS_FATAL_WARNINGS"); + if (s && *s) + fatal_warnings = TRUE; + + warn_initted = TRUE; + } + va_start (args, format); vfprintf (stderr, format, args); va_end (args); + + if (fatal_warnings) + { + fflush (stderr); + _dbus_abort (); + } } #ifdef DBUS_ENABLE_VERBOSE_MODE diff --git a/dbus/dbus-server-debug-pipe.c b/dbus/dbus-server-debug-pipe.c index 925f8971..0fb45c63 100644 --- a/dbus/dbus-server-debug-pipe.c +++ b/dbus/dbus-server-debug-pipe.c @@ -317,6 +317,7 @@ _dbus_transport_debug_pipe_new (const char *server_name, /* If no one grabbed a reference, the connection will die, * and the client transport will get an immediate disconnect */ + _dbus_connection_close_if_only_one_ref (connection); dbus_connection_unref (connection); return client_transport; diff --git a/dbus/dbus-server-socket.c b/dbus/dbus-server-socket.c index f4b5b0fd..5c11e145 100644 --- a/dbus/dbus-server-socket.c +++ b/dbus/dbus-server-socket.c @@ -69,14 +69,6 @@ socket_finalize (DBusServer *server) dbus_free (server); } -/** - * @todo unreffing the connection at the end may cause - * us to drop the last ref to the connection before - * disconnecting it. That is invalid. - * - * @todo doesn't this leak a server refcount if - * new_connection_function is NULL? - */ /* Return value is just for memory, not other failures. */ static dbus_bool_t handle_new_client_fd_and_unlock (DBusServer *server, @@ -140,10 +132,11 @@ handle_new_client_fd_and_unlock (DBusServer *server, { (* new_connection_function) (server, connection, new_connection_data); - dbus_server_unref (server); } + dbus_server_unref (server); /* If no one grabbed a reference, the connection will die. */ + _dbus_connection_close_if_only_one_ref (connection); dbus_connection_unref (connection); return TRUE; diff --git a/dbus/dbus-server.c b/dbus/dbus-server.c index 3b9ee341..e8bb8d11 100644 --- a/dbus/dbus-server.c +++ b/dbus/dbus-server.c @@ -791,7 +791,15 @@ dbus_server_get_address (DBusServer *server) * function is passed each new connection as the connection is * created. If the new connection function increments the connection's * reference count, the connection will stay alive. Otherwise, the - * connection will be unreferenced and closed. + * connection will be unreferenced and closed. The new connection + * function may also close the connection itself, which is considered + * good form if the connection is not wanted. + * + * The connection here is private in the sense of + * dbus_connection_open_private(), so if the new connection function + * keeps a reference it must arrange for the connection to be closed. + * i.e. libdbus does not own this connection once the new connection + * function takes a reference. * * @param server the server. * @param function a function to handle new connections. diff --git a/dbus/dbus-sysdeps-unix.c b/dbus/dbus-sysdeps-unix.c index d0538010..55b61a39 100644 --- a/dbus/dbus-sysdeps-unix.c +++ b/dbus/dbus-sysdeps-unix.c @@ -2129,14 +2129,14 @@ _dbus_set_fd_nonblocking (int fd, #if !defined (DBUS_DISABLE_ASSERT) || defined(DBUS_BUILD_TESTS) /** - * On GNU libc systems, print a crude backtrace to the verbose log. - * On other systems, print "no backtrace support" - * + * On GNU libc systems, print a crude backtrace to stderr. On other + * systems, print "no backtrace support" and block for possible gdb + * attachment if an appropriate environment variable is set. */ void _dbus_print_backtrace (void) -{ -#if defined (HAVE_BACKTRACE) && defined (DBUS_ENABLE_VERBOSE_MODE) +{ +#if defined (HAVE_BACKTRACE) && defined (DBUS_BUILT_R_DYNAMIC) void *bt[500]; int bt_size; int i; @@ -2149,13 +2149,17 @@ _dbus_print_backtrace (void) i = 0; while (i < bt_size) { - _dbus_verbose (" %s\n", syms[i]); + /* don't use dbus_warn since it can _dbus_abort() */ + fprintf (stderr, " %s\n", syms[i]); ++i; } + fflush (stderr); free (syms); +#elif defined (HAVE_BACKTRACE) && ! defined (DBUS_BUILT_R_DYNAMIC) + fprintf (stderr, " D-Bus not built with -rdynamic so unable to print a backtrace\n"); #else - _dbus_verbose (" D-Bus not compiled with backtrace support\n"); + fprintf (stderr, " D-Bus not compiled with backtrace support so unable to print a backtrace\n"); #endif } #endif /* asserts or tests enabled */ diff --git a/dbus/dbus-sysdeps.c b/dbus/dbus-sysdeps.c index 2db45900..e8c03ef3 100644 --- a/dbus/dbus-sysdeps.c +++ b/dbus/dbus-sysdeps.c @@ -60,14 +60,20 @@ _DBUS_DEFINE_GLOBAL_LOCK (sid_atom_cache); void _dbus_abort (void) { -#ifdef DBUS_ENABLE_VERBOSE_MODE const char *s; - s = _dbus_getenv ("DBUS_PRINT_BACKTRACE"); + + _dbus_print_backtrace (); + + s = _dbus_getenv ("DBUS_BLOCK_ON_ABORT"); if (s && *s) - _dbus_print_backtrace (); -#endif + { + /* don't use _dbus_warn here since it can _dbus_abort() */ + fprintf (stderr, " Process %lu sleeping for gdb attach\n", (unsigned long) _dbus_getpid()); + _dbus_sleep_milliseconds (1000 * 60); + } + abort (); - _dbus_exit (1); /* in case someone manages to ignore SIGABRT */ + _dbus_exit (1); /* in case someone manages to ignore SIGABRT ? */ } #endif -- cgit