diff options
| -rw-r--r-- | ChangeLog | 33 | ||||
| -rw-r--r-- | configure.in | 3 | ||||
| -rw-r--r-- | dbus/dbus-connection-internal.h | 2 | ||||
| -rw-r--r-- | dbus/dbus-connection.c | 461 | ||||
| -rw-r--r-- | dbus/dbus-object-tree.c | 45 | ||||
| -rw-r--r-- | dbus/dbus-server-protected.h | 39 | ||||
| -rw-r--r-- | dbus/dbus-server-unix.c | 55 | ||||
| -rw-r--r-- | dbus/dbus-server.c | 197 | ||||
| -rw-r--r-- | dbus/dbus-transport-unix.c | 18 | ||||
| -rw-r--r-- | dbus/dbus-transport.c | 59 | ||||
| -rw-r--r-- | doc/TODO | 2 | ||||
| -rw-r--r-- | tools/Makefile.am | 4 | ||||
| -rw-r--r-- | tools/dbus-tree-view.c | 2 | ||||
| -rw-r--r-- | tools/dbus-viewer.c | 189 |
14 files changed, 886 insertions, 223 deletions
@@ -1,3 +1,36 @@ +2005-02-13 Havoc Pennington <hp@redhat.com> + + * dbus/dbus-object-tree.c (handle_default_introspect_and_unlock): + fix a double-unlock + + * dbus/dbus-connection.c + (_dbus_connection_detach_pending_call_unlocked): add this + + Initial semi-correct pass through to fix thread locking; there are + still some issues with the condition variable paths I'm pretty + sure + + * dbus/dbus-server.c: add a mutex on DBusServer and appropriate + lock/unlock calls + + * dbus/dbus-connection.c (_dbus_connection_do_iteration_unlocked): + rename to add _unlocked + (struct DBusConnection): move "dispatch_acquired" and + "io_path_acquired" to use only one bit each. + (CONNECTION_LOCK, CONNECTION_UNLOCK): add checks with !DBUS_DISABLE_CHECKS + (dbus_connection_set_watch_functions): hacky fix to reentrancy + (_dbus_connection_add_watch, _dbus_connection_remove_watch) + (_dbus_connection_toggle_watch, _dbus_connection_add_timeout) + (_dbus_connection_remove_timeout) + (_dbus_connection_toggle_timeout): drop lock when calling out to + user functions; done in a hacky/bad way. + (_dbus_connection_send_and_unlock): add a missing unlock + (_dbus_connection_block_for_reply): add a missing unlock + + * dbus/dbus-transport.c (_dbus_transport_get_is_authenticated): + drop lock in a hacky probably unsafe way to call out to user + function + 2005-02-12 Havoc Pennington <hp@redhat.com> * tools/dbus-tree-view.c (info_set_func_text): display more diff --git a/configure.in b/configure.in index 87f214b1..0e76921b 100644 --- a/configure.in +++ b/configure.in @@ -842,6 +842,7 @@ if test x$have_glib = xno ; then have_gtk=no else PKG_CHECK_MODULES(DBUS_GTK, gtk+-2.0, have_gtk=yes, have_gtk=no) + PKG_CHECK_MODULES(DBUS_GTK_THREADS, gtk+-2.0 gthread-2.0, have_gtk_threads=yes, have_gtk_threads=no) fi if test x$have_gtk = xno ; then @@ -863,6 +864,8 @@ AM_CONDITIONAL(HAVE_GTK, test x$have_gtk = xyes) dnl Gtk flags AC_SUBST(DBUS_GTK_CFLAGS) AC_SUBST(DBUS_GTK_LIBS) +AC_SUBST(DBUS_GTK_THREADS_CFLAGS) +AC_SUBST(DBUS_GTK_THREADS_LIBS) # Qt detection AC_PATH_PROG(QT_MOC, moc, no) diff --git a/dbus/dbus-connection-internal.h b/dbus/dbus-connection-internal.h index 21dc415f..57e6fb0d 100644 --- a/dbus/dbus-connection-internal.h +++ b/dbus/dbus-connection-internal.h @@ -74,7 +74,7 @@ void _dbus_connection_toggle_timeout (DBusConnection DBusTimeout *timeout, dbus_bool_t enabled); DBusConnection* _dbus_connection_new_for_transport (DBusTransport *transport); -void _dbus_connection_do_iteration (DBusConnection *connection, +void _dbus_connection_do_iteration_unlocked (DBusConnection *connection, unsigned int flags, int timeout_milliseconds); diff --git a/dbus/dbus-connection.c b/dbus/dbus-connection.c index dfc5d44b..cac4fc8c 100644 --- a/dbus/dbus-connection.c +++ b/dbus/dbus-connection.c @@ -1,7 +1,7 @@ /* -*- mode: C; c-file-style: "gnu" -*- */ /* dbus-connection.c DBusConnection object * - * Copyright (C) 2002, 2003, 2004 Red Hat Inc. + * Copyright (C) 2002, 2003, 2004, 2005 Red Hat Inc. * * Licensed under the Academic Free License version 2.1 * @@ -39,20 +39,37 @@ #include "dbus-pending-call.h" #include "dbus-object-tree.h" -#if 0 -#define CONNECTION_LOCK(connection) do { \ - _dbus_verbose (" LOCK: %s\n", _DBUS_FUNCTION_NAME); \ - dbus_mutex_lock ((connection)->mutex); \ +#ifdef DBUS_DISABLE_CHECKS +#define TOOK_LOCK_CHECK(connection) +#define RELEASING_LOCK_CHECK(connection) +#define HAVE_LOCK_CHECK(connection) +#else +#define TOOK_LOCK_CHECK(connection) do { \ + _dbus_assert (!(connection)->have_connection_lock); \ + (connection)->have_connection_lock = TRUE; \ } while (0) -#define CONNECTION_UNLOCK(connection) do { \ - _dbus_verbose (" UNLOCK: %s\n", _DBUS_FUNCTION_NAME); \ - dbus_mutex_unlock ((connection)->mutex); \ +#define RELEASING_LOCK_CHECK(connection) do { \ + _dbus_assert ((connection)->have_connection_lock); \ + (connection)->have_connection_lock = FALSE; \ } while (0) -#else -#define CONNECTION_LOCK(connection) dbus_mutex_lock ((connection)->mutex) -#define CONNECTION_UNLOCK(connection) dbus_mutex_unlock ((connection)->mutex) +#define HAVE_LOCK_CHECK(connection) _dbus_assert ((connection)->have_connection_lock) +/* A "DO_NOT_HAVE_LOCK_CHECK" is impossible since we need the lock to check the flag */ #endif +#define TRACE_LOCKS 1 + +#define CONNECTION_LOCK(connection) do { \ + if (TRACE_LOCKS) { _dbus_verbose (" LOCK: %s\n", _DBUS_FUNCTION_NAME); } \ + dbus_mutex_lock ((connection)->mutex); \ + TOOK_LOCK_CHECK (connection); \ + } while (0) + +#define CONNECTION_UNLOCK(connection) do { \ + if (TRACE_LOCKS) { _dbus_verbose (" UNLOCK: %s\n", _DBUS_FUNCTION_NAME); } \ + RELEASING_LOCK_CHECK (connection); \ + dbus_mutex_unlock ((connection)->mutex); \ + } while (0) + #define DISPATCH_STATUS_NAME(s) \ ((s) == DBUS_DISPATCH_COMPLETE ? "complete" : \ (s) == DBUS_DISPATCH_DATA_REMAINS ? "data remains" : \ @@ -171,10 +188,7 @@ struct DBusConnection DBusMutex *mutex; /**< Lock on the entire DBusConnection */ - dbus_bool_t dispatch_acquired; /**< Protects dispatch() */ DBusCondVar *dispatch_cond; /**< Protects dispatch() */ - - dbus_bool_t io_path_acquired; /**< Protects transport io path */ DBusCondVar *io_path_cond; /**< Protects transport io path */ DBusList *outgoing_messages; /**< Queue of messages we need to send, send the end of the list first. */ @@ -215,9 +229,16 @@ struct DBusConnection * for the global linked list mempool lock */ DBusObjectTree *objects; /**< Object path handlers registered with this connection */ - + + unsigned int dispatch_acquired : 1; /**< Someone has dispatch path */ + unsigned int io_path_acquired : 1; /**< Someone has transport io path */ + unsigned int exit_on_disconnect : 1; /**< If #TRUE, exit after handling disconnect signal */ - + +#ifndef DBUS_DISABLE_CHECKS + unsigned int have_connection_lock : 1; /**< Used to check locking */ +#endif + #ifndef DBUS_DISABLE_CHECKS int generation; /**< _dbus_current_generation that should correspond to this connection */ #endif @@ -387,6 +408,8 @@ static void _dbus_connection_queue_synthesized_message_link (DBusConnection *connection, DBusList *link) { + HAVE_LOCK_CHECK (connection); + _dbus_list_append_link (&connection->incoming_messages, link); connection->n_incoming += 1; @@ -408,6 +431,7 @@ _dbus_connection_queue_synthesized_message_link (DBusConnection *connection, dbus_bool_t _dbus_connection_has_messages_to_send_unlocked (DBusConnection *connection) { + HAVE_LOCK_CHECK (connection); return connection->outgoing_messages != NULL; } @@ -441,6 +465,8 @@ dbus_connection_has_messages_to_send (DBusConnection *connection) DBusMessage* _dbus_connection_get_message_to_send (DBusConnection *connection) { + HAVE_LOCK_CHECK (connection); + return _dbus_list_get_last (&connection->outgoing_messages); } @@ -458,6 +484,8 @@ _dbus_connection_message_sent (DBusConnection *connection, { DBusList *link; + HAVE_LOCK_CHECK (connection); + /* This can be called before we even complete authentication, since * it's called on disconnect to clean up the outgoing queue. * It's also called as we successfully send each message. @@ -495,6 +523,62 @@ _dbus_connection_message_sent (DBusConnection *connection, dbus_message_unref (message); } +typedef dbus_bool_t (* DBusWatchAddFunction) (DBusWatchList *list, + DBusWatch *watch); +typedef void (* DBusWatchRemoveFunction) (DBusWatchList *list, + DBusWatch *watch); +typedef void (* DBusWatchToggleFunction) (DBusWatchList *list, + DBusWatch *watch, + dbus_bool_t enabled); + +static dbus_bool_t +protected_change_watch (DBusConnection *connection, + DBusWatch *watch, + DBusWatchAddFunction add_function, + DBusWatchRemoveFunction remove_function, + DBusWatchToggleFunction toggle_function, + dbus_bool_t enabled) +{ + DBusWatchList *watches; + dbus_bool_t retval; + + HAVE_LOCK_CHECK (connection); + + /* This isn't really safe or reasonable; a better pattern is the "do everything, then + * drop lock and call out" one; but it has to be propagated up through all callers + */ + + watches = connection->watches; + if (watches) + { + connection->watches = NULL; + _dbus_connection_ref_unlocked (connection); + CONNECTION_UNLOCK (connection); + + if (add_function) + retval = (* add_function) (watches, watch); + else if (remove_function) + { + retval = TRUE; + (* remove_function) (watches, watch); + } + else + { + retval = TRUE; + (* toggle_function) (watches, watch, enabled); + } + + CONNECTION_LOCK (connection); + connection->watches = watches; + _dbus_connection_unref_unlocked (connection); + + return retval; + } + else + return FALSE; +} + + /** * Adds a watch using the connection's DBusAddWatchFunction if * available. Otherwise records the watch to be added when said @@ -509,11 +593,9 @@ dbus_bool_t _dbus_connection_add_watch (DBusConnection *connection, DBusWatch *watch) { - if (connection->watches) /* null during finalize */ - return _dbus_watch_list_add_watch (connection->watches, - watch); - else - return FALSE; + return protected_change_watch (connection, watch, + _dbus_watch_list_add_watch, + NULL, NULL, FALSE); } /** @@ -528,9 +610,10 @@ void _dbus_connection_remove_watch (DBusConnection *connection, DBusWatch *watch) { - if (connection->watches) /* null during finalize */ - _dbus_watch_list_remove_watch (connection->watches, - watch); + protected_change_watch (connection, watch, + NULL, + _dbus_watch_list_remove_watch, + NULL, FALSE); } /** @@ -549,10 +632,66 @@ _dbus_connection_toggle_watch (DBusConnection *connection, dbus_bool_t enabled) { _dbus_assert (watch != NULL); + + protected_change_watch (connection, watch, + NULL, NULL, + _dbus_watch_list_toggle_watch, + enabled); +} + +typedef dbus_bool_t (* DBusTimeoutAddFunction) (DBusTimeoutList *list, + DBusTimeout *timeout); +typedef void (* DBusTimeoutRemoveFunction) (DBusTimeoutList *list, + DBusTimeout *timeout); +typedef void (* DBusTimeoutToggleFunction) (DBusTimeoutList *list, + DBusTimeout *timeout, + dbus_bool_t enabled); + +static dbus_bool_t +protected_change_timeout (DBusConnection *connection, + DBusTimeout *timeout, + DBusTimeoutAddFunction add_function, + DBusTimeoutRemoveFunction remove_function, + DBusTimeoutToggleFunction toggle_function, + dbus_bool_t enabled) +{ + DBusTimeoutList *timeouts; + dbus_bool_t retval; + + HAVE_LOCK_CHECK (connection); + + /* This isn't really safe or reasonable; a better pattern is the "do everything, then + * drop lock and call out" one; but it has to be propagated up through all callers + */ - if (connection->watches) /* null during finalize */ - _dbus_watch_list_toggle_watch (connection->watches, - watch, enabled); + timeouts = connection->timeouts; + if (timeouts) + { + connection->timeouts = NULL; + _dbus_connection_ref_unlocked (connection); + CONNECTION_UNLOCK (connection); + + if (add_function) + retval = (* add_function) (timeouts, timeout); + else if (remove_function) + { + retval = TRUE; + (* remove_function) (timeouts, timeout); + } + else + { + retval = TRUE; + (* toggle_function) (timeouts, timeout, enabled); + } + + CONNECTION_LOCK (connection); + connection->timeouts = timeouts; + _dbus_connection_unref_unlocked (connection); + + return retval; + } + else + return FALSE; } /** @@ -570,11 +709,9 @@ dbus_bool_t _dbus_connection_add_timeout (DBusConnection *connection, DBusTimeout *timeout) { - if (connection->timeouts) /* null during finalize */ - return _dbus_timeout_list_add_timeout (connection->timeouts, - timeout); - else - return FALSE; + return protected_change_timeout (connection, timeout, + _dbus_timeout_list_add_timeout, + NULL, NULL, FALSE); } /** @@ -589,9 +726,10 @@ void _dbus_connection_remove_timeout (DBusConnection *connection, DBusTimeout *timeout) { - if (connection->timeouts) /* null during finalize */ - _dbus_timeout_list_remove_timeout (connection->timeouts, - timeout); + protected_change_timeout (connection, timeout, + NULL, + _dbus_timeout_list_remove_timeout, + NULL, FALSE); } /** @@ -604,19 +742,22 @@ _dbus_connection_remove_timeout (DBusConnection *connection, * @param enabled whether to enable or disable */ void -_dbus_connection_toggle_timeout (DBusConnection *connection, +_dbus_connection_toggle_timeout (DBusConnection *connection, DBusTimeout *timeout, - dbus_bool_t enabled) + dbus_bool_t enabled) { - if (connection->timeouts) /* null during finalize */ - _dbus_timeout_list_toggle_timeout (connection->timeouts, - timeout, enabled); + protected_change_timeout (connection, timeout, + NULL, NULL, + _dbus_timeout_list_toggle_timeout, + enabled); } static dbus_bool_t _dbus_connection_attach_pending_call_unlocked (DBusConnection *connection, DBusPendingCall *pending) { + HAVE_LOCK_CHECK (connection); + _dbus_assert (pending->reply_serial != 0); if (!_dbus_connection_add_timeout (connection, pending->timeout)) @@ -627,6 +768,8 @@ _dbus_connection_attach_pending_call_unlocked (DBusConnection *connection, pending)) { _dbus_connection_remove_timeout (connection, pending->timeout); + + HAVE_LOCK_CHECK (connection); return FALSE; } @@ -634,6 +777,8 @@ _dbus_connection_attach_pending_call_unlocked (DBusConnection *connection, pending->connection = connection; dbus_pending_call_ref (pending); + + HAVE_LOCK_CHECK (connection); return TRUE; } @@ -664,6 +809,19 @@ free_pending_call_on_hash_removal (void *data) } static void +_dbus_connection_detach_pending_call_unlocked (DBusConnection *connection, + DBusPendingCall *pending) +{ + /* Can't have a destroy notifier on the pending call if we're going to do this */ + + dbus_pending_call_ref (pending); + _dbus_hash_table_remove_int (connection->pending_replies, + pending->reply_serial); + _dbus_assert (pending->connection == NULL); + dbus_pending_call_unref (pending); +} + +static void _dbus_connection_detach_pending_call_and_unlock (DBusConnection *connection, DBusPendingCall *pending) { @@ -674,6 +832,7 @@ _dbus_connection_detach_pending_call_and_unlock (DBusConnection *connection, dbus_pending_call_ref (pending); _dbus_hash_table_remove_int (connection->pending_replies, pending->reply_serial); + _dbus_assert (pending->connection == NULL); CONNECTION_UNLOCK (connection); dbus_pending_call_unref (pending); } @@ -749,14 +908,25 @@ _dbus_connection_acquire_io_path (DBusConnection *connection, { dbus_bool_t res = TRUE; + _dbus_verbose ("%s start connection->io_path_acquired = %d timeout = %d\n", + _DBUS_FUNCTION_NAME, connection->io_path_acquired, timeout_milliseconds); + if (connection->io_path_acquired) { +#ifndef DBUS_DISABLE_CHECKS + connection->have_connection_lock = FALSE; +#endif + if (timeout_milliseconds != -1) res = dbus_condvar_wait_timeout (connection->io_path_cond, connection->mutex, timeout_milliseconds); else dbus_condvar_wait (connection->io_path_cond, connection->mutex); + +#ifndef DBUS_DISABLE_CHECKS + connection->have_connection_lock = TRUE; +#endif } if (res) @@ -765,6 +935,9 @@ _dbus_connection_acquire_io_path (DBusConnection *connection, connection->io_path_acquired = TRUE; } + + _dbus_verbose ("%s end connection->io_path_acquired = %d res = %d\n", + _DBUS_FUNCTION_NAME, connection->io_path_acquired, res); return res; } @@ -781,6 +954,9 @@ _dbus_connection_release_io_path (DBusConnection *connection) { _dbus_assert (connection->io_path_acquired); + _dbus_verbose ("%s start connection->io_path_acquired = %d\n", + _DBUS_FUNCTION_NAME, connection->io_path_acquired); + connection->io_path_acquired = FALSE; dbus_condvar_wake_one (connection->io_path_cond); } @@ -789,7 +965,7 @@ _dbus_connection_release_io_path (DBusConnection *connection) /** * Queues incoming messages and sends outgoing messages for this * connection, optionally blocking in the process. Each call to - * _dbus_connection_do_iteration() will call select() or poll() one + * _dbus_connection_do_iteration_unlocked() will call select() or poll() one * time and then read or write data if possible. * * The purpose of this function is to be able to flush outgoing @@ -815,10 +991,14 @@ _dbus_connection_release_io_path (DBusConnection *connection) * @param timeout_milliseconds maximum blocking time, or -1 for no limit. */ void -_dbus_connection_do_iteration (DBusConnection *connection, - unsigned int flags, - int timeout_milliseconds) +_dbus_connection_do_iteration_unlocked (DBusConnection *connection, + unsigned int flags, + int timeout_milliseconds) { + _dbus_verbose ("%s start\n", _DBUS_FUNCTION_NAME); + + HAVE_LOCK_CHECK (connection); + if (connection->n_outgoing == 0) flags &= ~DBUS_ITERATION_DO_WRITING; @@ -829,6 +1009,8 @@ _dbus_connection_do_iteration (DBusConnection *connection, flags, timeout_milliseconds); _dbus_connection_release_io_path (connection); } + + _dbus_verbose ("%s end\n", _DBUS_FUNCTION_NAME); } /** @@ -949,11 +1131,15 @@ _dbus_connection_new_for_transport (DBusTransport *transport) connection->client_serial = 1; connection->disconnect_message_link = disconnect_link; + + CONNECTION_LOCK (connection); if (!_dbus_transport_set_connection (transport, connection)) goto error; - _dbus_transport_ref (transport); + _dbus_transport_ref (transport); + + CONNECTION_UNLOCK (connection); return connection; @@ -1006,9 +1192,11 @@ _dbus_connection_new_for_transport (DBusTransport *transport) */ DBusConnection * _dbus_connection_ref_unlocked (DBusConnection *connection) -{ +{ _dbus_assert (connection != NULL); _dbus_assert (connection->generation == _dbus_current_generation); + + HAVE_LOCK_CHECK (connection); #ifdef DBUS_HAVE_ATOMIC_INT _dbus_atomic_inc (&connection->refcount); @@ -1031,7 +1219,9 @@ _dbus_connection_unref_unlocked (DBusConnection *connection) { dbus_bool_t last_unref; - _dbus_return_if_fail (connection != NULL); + HAVE_LOCK_CHECK (connection); + + _dbus_assert (connection != NULL); /* The connection lock is better than the global * lock in the atomic increment fallback @@ -1089,6 +1279,8 @@ _dbus_connection_handle_watch (DBusWatch *watch, DBusDispatchStatus status; connection = data; + + _dbus_verbose ("%s start\n", _DBUS_FUNCTION_NAME); CONNECTION_LOCK (connection); _dbus_connection_acquire_io_path (connection, -1); @@ -1096,10 +1288,16 @@ _dbus_connection_handle_watch (DBusWatch *watch, watch, condition); _dbus_connection_release_io_path (connection); + HAVE_LOCK_CHECK (connection); + + _dbus_verbose ("%s middle\n", _DBUS_FUNCTION_NAME); + status = _dbus_connection_get_dispatch_status_unlocked (connection); /* this calls out to user code */ _dbus_connection_update_dispatch_status_and_unlock (connection, status); + + _dbus_verbose ("%s end\n", _DBUS_FUNCTION_NAME); return retval; } @@ -1154,7 +1352,10 @@ dbus_connection_open (const char *address, dbus_set_error (error, DBUS_ERROR_NO_MEMORY, NULL); return NULL; } - + +#ifndef DBUS_DISABLE_CHECKS + _dbus_assert (!connection->have_connection_lock); +#endif return connection; } @@ -1357,7 +1558,8 @@ dbus_connection_disconnect (DBusConnection *connection) CONNECTION_LOCK (connection); _dbus_transport_disconnect (connection->transport); - + + _dbus_verbose ("%s middle\n", _DBUS_FUNCTION_NAME); status = _dbus_connection_get_dispatch_status_unlocked (connection); /* this calls out to user code */ @@ -1367,6 +1569,7 @@ dbus_connection_disconnect (DBusConnection *connection) static dbus_bool_t _dbus_connection_get_is_connected_unlocked (DBusConnection *connection) { + HAVE_LOCK_CHECK (connection); return _dbus_transport_get_is_connected (connection->transport); } @@ -1445,6 +1648,8 @@ _dbus_connection_preallocate_send_unlocked (DBusConnection *connection) { DBusPreallocatedSend *preallocated; + HAVE_LOCK_CHECK (connection); + _dbus_assert (connection != NULL); preallocated = dbus_new (DBusPreallocatedSend, 1); @@ -1604,9 +1809,9 @@ _dbus_connection_send_preallocated_unlocked_no_update (DBusConnection *con /* Now we need to run an iteration to hopefully just write the messages * out immediately, and otherwise get them queued up */ - _dbus_connection_do_iteration (connection, - DBUS_ITERATION_DO_WRITING, - -1); + _dbus_connection_do_iteration_unlocked (connection, + DBUS_ITERATION_DO_WRITING, + -1); /* If stuff is still queued up, be sure we wake up the main loop */ if (connection->n_outgoing > 0) @@ -1621,10 +1826,13 @@ _dbus_connection_send_preallocated_and_unlock (DBusConnection *connection, { DBusDispatchStatus status; + HAVE_LOCK_CHECK (connection); + _dbus_connection_send_preallocated_unlocked_no_update (connection, preallocated, message, client_serial); + _dbus_verbose ("%s middle\n", _DBUS_FUNCTION_NAME); status = _dbus_connection_get_dispatch_status_unlocked (connection); /* this calls out to user code */ @@ -1699,7 +1907,10 @@ _dbus_connection_send_and_unlock (DBusConnection *connection, preallocated = _dbus_connection_preallocate_send_unlocked (connection); if (preallocated == NULL) - return FALSE; + { + CONNECTION_UNLOCK (connection); + return FALSE; + } _dbus_connection_send_preallocated_and_unlock (connection, preallocated, @@ -1762,6 +1973,7 @@ reply_handler_timeout (void *data) pending->timeout); pending->timeout_added = FALSE; + _dbus_verbose ("%s middle\n", _DBUS_FUNCTION_NAME); status = _dbus_connection_get_dispatch_status_unlocked (connection); /* Unlocks, and calls out to user code */ @@ -1877,8 +2089,12 @@ dbus_connection_send_with_reply (DBusConnection *connection, if (pending_return) *pending_return = pending; else - dbus_pending_call_unref (pending); + { + _dbus_connection_detach_pending_call_unlocked (connection, pending); + dbus_pending_call_unref (pending); + } + _dbus_verbose ("%s middle\n", _DBUS_FUNCTION_NAME); status = _dbus_connection_get_dispatch_status_unlocked (connection); /* this calls out to user code */ @@ -1898,6 +2114,8 @@ check_for_reply_unlocked (DBusConnection *connection, dbus_uint32_t client_serial) { DBusList *link; + + HAVE_LOCK_CHECK (connection); link = _dbus_list_get_first_link (&connection->incoming_messages); @@ -1980,14 +2198,19 @@ _dbus_connection_block_for_reply (DBusConnection *connection, * gets the message before we do? */ /* always block at least once as we know we don't have the reply yet */ - _dbus_connection_do_iteration (connection, - DBUS_ITERATION_DO_READING | - DBUS_ITERATION_BLOCK, - timeout_milliseconds); + _dbus_connection_do_iteration_unlocked (connection, + DBUS_ITERATION_DO_READING | + DBUS_ITERATION_BLOCK, + timeout_milliseconds); recheck_status: + _dbus_verbose ("%s top of recheck\n", _DBUS_FUNCTION_NAME); + + HAVE_LOCK_CHECK (connection); + /* queue messages and get status */ + status = _dbus_connection_get_dispatch_status_unlocked (connection); if (status == DBUS_DISPATCH_DATA_REMAINS) @@ -1996,7 +2219,8 @@ _dbus_connection_block_for_reply (DBusConnection *connection, reply = check_for_reply_unlocked (connection, client_serial); if (reply != NULL) - { + { + _dbus_verbose ("%s checked for reply\n", _DBUS_FUNCTION_NAME); status = _dbus_connection_get_dispatch_status_unlocked (connection); _dbus_verbose ("dbus_connection_send_with_reply_and_block(): got reply\n"); @@ -2011,7 +2235,10 @@ _dbus_connection_block_for_reply (DBusConnection *connection, _dbus_get_current_time (&tv_sec, &tv_usec); if (!_dbus_connection_get_is_connected_unlocked (connection)) - return NULL; + { + CONNECTION_UNLOCK (connection); + return NULL; + } else if (tv_sec < start_tv_sec) _dbus_verbose ("dbus_connection_send_with_reply_and_block(): clock set backward\n"); else if (connection->disconnect_message_link == NULL) @@ -2042,10 +2269,10 @@ _dbus_connection_block_for_reply (DBusConnection *connection, else { /* block again, we don't have the reply buffered yet. */ - _dbus_connection_do_iteration (connection, - DBUS_ITERATION_DO_READING | - DBUS_ITERATION_BLOCK, - timeout_milliseconds); + _dbus_connection_do_iteration_unlocked (connection, + DBUS_ITERATION_DO_READING | + DBUS_ITERATION_BLOCK, + timeout_milliseconds); } goto recheck_status; @@ -2144,16 +2371,25 @@ dbus_connection_flush (DBusConnection *connection) CONNECTION_LOCK (connection); while (connection->n_outgoing > 0 && _dbus_connection_get_is_connected_unlocked (connection)) - _dbus_connection_do_iteration (connection, - DBUS_ITERATION_DO_READING | - DBUS_ITERATION_DO_WRITING | - DBUS_ITERATION_BLOCK, - -1); + { + _dbus_verbose ("doing iteration in %s\n", _DBUS_FUNCTION_NAME); + HAVE_LOCK_CHECK (connection); + _dbus_connection_do_iteration_unlocked (connection, + DBUS_ITERATION_DO_READING | + DBUS_ITERATION_DO_WRITING | + DBUS_ITERATION_BLOCK, + -1); + } + HAVE_LOCK_CHECK (connection); + _dbus_verbose ("%s middle\n", _DBUS_FUNCTION_NAME); status = _dbus_connection_get_dispatch_status_unlocked (connection); + HAVE_LOCK_CHECK (connection); /* Unlocks and calls out to user code */ _dbus_connection_update_dispatch_status_and_unlock (connection, status); + + _dbus_verbose ("%s end\n", _DBUS_FUNCTION_NAME); } /* Call with mutex held. Will drop it while waiting and re-acquire @@ -2165,7 +2401,15 @@ _dbus_connection_wait_for_borrowed (DBusConnection *connection) _dbus_assert (connection->message_borrowed != NULL); while (connection->message_borrowed != NULL) - dbus_condvar_wait (connection->message_returned_cond, connection->mutex); + { +#ifndef DBUS_DISABLE_CHECKS + connection->have_connection_lock = FALSE; +#endif + dbus_condvar_wait (connection->message_returned_cond, connection->mutex); +#ifndef DBUS_DISABLE_CHECKS + connection->have_connection_lock = TRUE; +#endif + } } /** @@ -2191,6 +2435,8 @@ dbus_connection_borrow_message (DBusConnection *connection) _dbus_return_val_if_fail (connection != NULL, NULL); /* can't borrow during dispatch */ _dbus_return_val_if_fail (!connection->dispatch_acquired, NULL); + + _dbus_verbose ("%s start\n", _DBUS_FUNCTION_NAME); /* this is called for the side effect that it queues * up any messages from the transport @@ -2283,6 +2529,8 @@ dbus_connection_steal_borrowed_message (DBusConnection *connection, static DBusList* _dbus_connection_pop_message_link_unlocked (DBusConnection *connection) { + HAVE_LOCK_CHECK (connection); + if (connection->message_borrowed != NULL) _dbus_connection_wait_for_borrowed (connection); @@ -2319,6 +2567,8 @@ static DBusMessage* _dbus_connection_pop_message_unlocked (DBusConnection *connection) { DBusList *link; + + HAVE_LOCK_CHECK (connection); link = _dbus_connection_pop_message_link_unlocked (connection); @@ -2340,6 +2590,8 @@ static void _dbus_connection_putback_message_link_unlocked (DBusConnection *connection, DBusList *message_link) { + HAVE_LOCK_CHECK (connection); + _dbus_assert (message_link != NULL); /* You can't borrow a message while a link is outstanding */ _dbus_assert (connection->message_borrowed == NULL); @@ -2381,6 +2633,8 @@ dbus_connection_pop_message (DBusConnection *connection) DBusMessage *message; DBusDispatchStatus status; + _dbus_verbose ("%s start\n", _DBUS_FUNCTION_NAME); + /* this is called for the side effect that it queues * up any messages from the transport */ @@ -2411,7 +2665,15 @@ static void _dbus_connection_acquire_dispatch (DBusConnection *connection) { if (connection->dispatch_acquired) - dbus_condvar_wait (connection->dispatch_cond, connection->mutex); + { +#ifndef DBUS_DISABLE_CHECKS + connection->have_connection_lock = FALSE; +#endif + dbus_condvar_wait (connection->dispatch_cond, connection->mutex); +#ifndef DBUS_DISABLE_CHECKS + connection->have_connection_lock = TRUE; +#endif + } _dbus_assert (!connection->dispatch_acquired); connection->dispatch_acquired = TRUE; @@ -2445,6 +2707,8 @@ _dbus_connection_failed_pop (DBusConnection *connection, static DBusDispatchStatus _dbus_connection_get_dispatch_status_unlocked (DBusConnection *connection) { + HAVE_LOCK_CHECK (connection); + if (connection->n_incoming > 0) return DBUS_DISPATCH_DATA_REMAINS; else if (!_dbus_transport_queue_messages (connection->transport)) @@ -2511,7 +2775,7 @@ _dbus_connection_update_dispatch_status_and_unlock (DBusConnection *connectio DBusDispatchStatusFunction function; void *data; - /* We have the lock */ + HAVE_LOCK_CHECK (connection); _dbus_connection_ref_unlocked (connection); @@ -2550,6 +2814,8 @@ dbus_connection_get_dispatch_status (DBusConnection *connection) DBusDispatchStatus status; _dbus_return_val_if_fail (connection != NULL, DBUS_DISPATCH_COMPLETE); + + _dbus_verbose ("%s start\n", _DBUS_FUNCTION_NAME); CONNECTION_LOCK (connection); @@ -2759,7 +3025,8 @@ dbus_connection_dispatch (DBusConnection *connection) dbus_message_get_member (message) : "no member", dbus_message_get_signature (message)); - + + HAVE_LOCK_CHECK (connection); result = _dbus_object_tree_dispatch_and_unlock (connection->objects, message); @@ -2874,7 +3141,8 @@ dbus_connection_dispatch (DBusConnection *connection) } _dbus_connection_release_dispatch (connection); - + + _dbus_verbose ("%s before final status update\n", _DBUS_FUNCTION_NAME); status = _dbus_connection_get_dispatch_status_unlocked (connection); /* unlocks and calls user code */ @@ -2952,25 +3220,43 @@ dbus_connection_set_watch_functions (DBusConnection *connection, DBusFreeFunction free_data_function) { dbus_bool_t retval; + DBusWatchList *watches; _dbus_return_val_if_fail (connection != NULL, FALSE); CONNECTION_LOCK (connection); + +#ifndef DBUS_DISABLE_CHECKS + if (connection->watches == NULL) + { + _dbus_warn ("Re-entrant call to %s is not allowed\n", + _DBUS_FUNCTION_NAME); + return FALSE; + } +#endif + /* ref connection for slightly better reentrancy */ _dbus_connection_ref_unlocked (connection); - /* FIXME this can call back into user code, and we need to drop the - * connection lock when it does. + /* This can call back into user code, and we need to drop the + * connection lock when it does. This is kind of a lame + * way to do it. */ - retval = _dbus_watch_list_set_functions (connection->watches, + watches = connection->watches; + connection->watches = NULL; + CONNECTION_UNLOCK (connection); + + retval = _dbus_watch_list_set_functions (watches, add_function, remove_function, toggled_function, data, free_data_function); + CONNECTION_LOCK (connection); + connection->watches = watches; CONNECTION_UNLOCK (connection); /* drop our paranoid refcount */ dbus_connection_unref (connection); - + return retval; } @@ -3016,17 +3302,34 @@ dbus_connection_set_timeout_functions (DBusConnection *connection, DBusFreeFunction free_data_function) { dbus_bool_t retval; + DBusTimeoutList *timeouts; _dbus_return_val_if_fail (connection != NULL, FALSE); CONNECTION_LOCK (connection); + +#ifndef DBUS_DISABLE_CHECKS + if (connection->timeouts == NULL) + { + _dbus_warn ("Re-entrant call to %s is not allowed\n", + _DBUS_FUNCTION_NAME); + return FALSE; + } +#endif + /* ref connection for slightly better reentrancy */ _dbus_connection_ref_unlocked (connection); + + timeouts = connection->timeouts; + connection->timeouts = NULL; + CONNECTION_UNLOCK (connection); - retval = _dbus_timeout_list_set_functions (connection->timeouts, + retval = _dbus_timeout_list_set_functions (timeouts, add_function, remove_function, toggled_function, data, free_data_function); + CONNECTION_LOCK (connection); + connection->timeouts = timeouts; CONNECTION_UNLOCK (connection); /* drop our paranoid refcount */ diff --git a/dbus/dbus-object-tree.c b/dbus/dbus-object-tree.c index ae28cc1c..b693b919 100644 --- a/dbus/dbus-object-tree.c +++ b/dbus/dbus-object-tree.c @@ -1,7 +1,7 @@ /* -*- mode: C; c-file-style: "gnu" -*- */ /* dbus-object-tree.c DBusObjectTree (internals of DBusConnection) * - * Copyright (C) 2003 Red Hat Inc. + * Copyright (C) 2003, 2005 Red Hat Inc. * * Licensed under the Academic Free License version 2.1 * @@ -503,6 +503,7 @@ _dbus_object_tree_unregister_and_unlock (DBusObjectTree *tree, #endif { _dbus_connection_ref_unlocked (connection); + _dbus_verbose ("unlock %s\n", _DBUS_FUNCTION_NAME); _dbus_connection_unlock (connection); } @@ -635,7 +636,10 @@ handle_default_introspect_and_unlock (DBusObjectTree *tree, #ifdef DBUS_BUILD_TESTS if (tree->connection) #endif - _dbus_connection_unlock (tree->connection); + { + _dbus_verbose ("unlock %s %d\n", _DBUS_FUNCTION_NAME, __LINE__); + _dbus_connection_unlock (tree->connection); + } return DBUS_HANDLER_RESULT_NOT_YET_HANDLED; } @@ -647,7 +651,10 @@ handle_default_introspect_and_unlock (DBusObjectTree *tree, #ifdef DBUS_BUILD_TESTS if (tree->connection) #endif - _dbus_connection_unlock (tree->connection); + { + _dbus_verbose ("unlock %s %d\n", _DBUS_FUNCTION_NAME, __LINE__); + _dbus_connection_unlock (tree->connection); + } return DBUS_HANDLER_RESULT_NEED_MEMORY; } @@ -690,6 +697,8 @@ handle_default_introspect_and_unlock (DBusObjectTree *tree, if (tree->connection) #endif { + already_unlocked = TRUE; + if (!_dbus_connection_send_and_unlock (tree->connection, reply, NULL)) goto out; } @@ -702,7 +711,10 @@ handle_default_introspect_and_unlock (DBusObjectTree *tree, #endif { if (!already_unlocked) - _dbus_connection_unlock (tree->connection); + { + _dbus_verbose ("unlock %s %d\n", _DBUS_FUNCTION_NAME, __LINE__); + _dbus_connection_unlock (tree->connection); + } } _dbus_string_free (&xml); @@ -747,7 +759,10 @@ _dbus_object_tree_dispatch_and_unlock (DBusObjectTree *tree, #ifdef DBUS_BUILD_TESTS if (tree->connection) #endif - _dbus_connection_unlock (tree->connection); + { + _dbus_verbose ("unlock %s\n", _DBUS_FUNCTION_NAME); + _dbus_connection_unlock (tree->connection); + } _dbus_verbose ("No memory to get decomposed path\n"); @@ -759,7 +774,10 @@ _dbus_object_tree_dispatch_and_unlock (DBusObjectTree *tree, #ifdef DBUS_BUILD_TESTS if (tree->connection) #endif - _dbus_connection_unlock (tree->connection); + { + _dbus_verbose ("unlock %s\n", _DBUS_FUNCTION_NAME); + _dbus_connection_unlock (tree->connection); + } _dbus_verbose ("No path field in message\n"); return DBUS_HANDLER_RESULT_NOT_YET_HANDLED; @@ -822,7 +840,10 @@ _dbus_object_tree_dispatch_and_unlock (DBusObjectTree *tree, #ifdef DBUS_BUILD_TESTS if (tree->connection) #endif - _dbus_connection_unlock (tree->connection); + { + _dbus_verbose ("unlock %s\n", _DBUS_FUNCTION_NAME); + _dbus_connection_unlock (tree->connection); + } /* FIXME you could unregister the subtree in another thread * before we invoke the callback, and I can't figure out a @@ -859,7 +880,10 @@ _dbus_object_tree_dispatch_and_unlock (DBusObjectTree *tree, #ifdef DBUS_BUILD_TESTS if (tree->connection) #endif - _dbus_connection_unlock (tree->connection); + { + _dbus_verbose ("unlock %s\n", _DBUS_FUNCTION_NAME); + _dbus_connection_unlock (tree->connection); + } } while (list != NULL) @@ -993,7 +1017,10 @@ _dbus_object_tree_list_registered_and_unlock (DBusObjectTree *tree, #ifdef DBUS_BUILD_TESTS if (tree->connection) #endif - _dbus_connection_unlock (tree->connection); + { + _dbus_verbose ("unlock %s\n", _DBUS_FUNCTION_NAME); + _dbus_connection_unlock (tree->connection); + } return result; } diff --git a/dbus/dbus-server-protected.h b/dbus/dbus-server-protected.h index bce29ca8..c8aa8601 100644 --- a/dbus/dbus-server-protected.h +++ b/dbus/dbus-server-protected.h @@ -23,6 +23,7 @@ #ifndef DBUS_SERVER_PROTECTED_H #define DBUS_SERVER_PROTECTED_H +#include <config.h> #include <dbus/dbus-internals.h> #include <dbus/dbus-server.h> #include <dbus/dbus-timeout.h> @@ -51,8 +52,9 @@ struct DBusServerVTable */ struct DBusServer { - int refcount; /**< Reference count. */ + DBusAtomic refcount; /**< Reference count. */ const DBusServerVTable *vtable; /**< Virtual methods for this instance. */ + DBusMutex *mutex; /**< Lock on the server object */ DBusWatchList *watches; /**< Our watches */ DBusTimeoutList *timeouts; /**< Our timeouts */ @@ -74,6 +76,10 @@ struct DBusServer char **auth_mechanisms; /**< Array of allowed authentication mechanisms */ unsigned int disconnected : 1; /**< TRUE if we are disconnected. */ + +#ifndef DBUS_DISABLE_CHECKS + unsigned int have_server_lock : 1; /**< Does someone have the server mutex locked */ +#endif }; dbus_bool_t _dbus_server_init_base (DBusServer *server, @@ -95,7 +101,38 @@ void _dbus_server_toggle_timeout (DBusServer *server, DBusTimeout *timeout, dbus_bool_t enabled); +void _dbus_server_ref_unlocked (DBusServer *server); + +#ifdef DBUS_DISABLE_CHECKS +#define TOOK_LOCK_CHECK(server) +#define RELEASING_LOCK_CHECK(server) +#define HAVE_LOCK_CHECK(server) +#else +#define TOOK_LOCK_CHECK(server) do { \ + _dbus_assert (!(server)->have_server_lock); \ + (server)->have_server_lock = TRUE; \ + } while (0) +#define RELEASING_LOCK_CHECK(server) do { \ + _dbus_assert ((server)->have_server_lock); \ + (server)->have_server_lock = FALSE; \ + } while (0) +#define HAVE_LOCK_CHECK(server) _dbus_assert ((server)->have_server_lock) +/* A "DO_NOT_HAVE_LOCK_CHECK" is impossible since we need the lock to check the flag */ +#endif + +#define TRACE_LOCKS 0 + +#define SERVER_LOCK(server) do { \ + if (TRACE_LOCKS) { _dbus_verbose (" LOCK: %s\n", _DBUS_FUNCTION_NAME); } \ + dbus_mutex_lock ((server)->mutex); \ + TOOK_LOCK_CHECK (server); \ + } while (0) +#define SERVER_UNLOCK(server) do { \ + if (TRACE_LOCKS) { _dbus_verbose (" UNLOCK: %s\n", _DBUS_FUNCTION_NAME); } \ + RELEASING_LOCK_CHECK (server); \ + dbus_mutex_unlock ((server)->mutex); \ + } while (0) DBUS_END_DECLS diff --git a/dbus/dbus-server-unix.c b/dbus/dbus-server-unix.c index f576b427..4a07f149 100644 --- a/dbus/dbus-server-unix.c +++ b/dbus/dbus-server-unix.c @@ -72,21 +72,29 @@ unix_finalize (DBusServer *server) */ /* Return value is just for memory, not other failures. */ static dbus_bool_t -handle_new_client_fd (DBusServer *server, - int client_fd) +handle_new_client_fd_and_unlock (DBusServer *server, + int client_fd) { DBusConnection *connection; DBusTransport *transport; + DBusNewConnectionFunction new_connection_function; + void *new_connection_data; _dbus_verbose ("Creating new client connection with fd %d\n", client_fd); - + + HAVE_LOCK_CHECK (server); + if (!_dbus_set_fd_nonblocking (client_fd, NULL)) - return TRUE; + { + SERVER_UNLOCK (server); + return TRUE; + } transport = _dbus_transport_new_for_fd (client_fd, TRUE, NULL); if (transport == NULL) { close (client_fd); + SERVER_UNLOCK (server); return FALSE; } @@ -94,6 +102,7 @@ handle_new_client_fd (DBusServer *server, (const char **) server->auth_mechanisms)) { _dbus_transport_unref (transport); + SERVER_UNLOCK (server); return FALSE; } @@ -103,19 +112,27 @@ handle_new_client_fd (DBusServer *server, connection = _dbus_connection_new_for_transport (transport); _dbus_transport_unref (transport); + transport = NULL; /* now under the connection lock */ if (connection == NULL) - return FALSE; + { + SERVER_UNLOCK (server); + return FALSE; + } - /* See if someone wants to handle this new connection, - * self-referencing for paranoia + /* See if someone wants to handle this new connection, self-referencing + * for paranoia. */ - if (server->new_connection_function) + new_connection_function = server->new_connection_function; + new_connection_data = server->new_connection_data; + + _dbus_server_ref_unlocked (server); + SERVER_UNLOCK (server); + + if (new_connection_function) { - dbus_server_ref (server); - - (* server->new_connection_function) (server, connection, - server->new_connection_data); + (* new_connection_function) (server, connection, + new_connection_data); dbus_server_unref (server); } @@ -133,6 +150,8 @@ unix_handle_watch (DBusWatch *watch, DBusServer *server = data; DBusServerUnix *unix_server = data; + SERVER_LOCK (server); + _dbus_assert (watch == unix_server->watch); _dbus_verbose ("Handling client connection, flags 0x%x\n", flags); @@ -155,12 +174,14 @@ unix_handle_watch (DBusWatch *watch, else _dbus_verbose ("Failed to accept a client connection: %s\n", _dbus_strerror (errno)); + + SERVER_UNLOCK (server); } else { _dbus_fd_set_close_on_exec (client_fd); - if (!handle_new_client_fd (server, client_fd)) + if (!handle_new_client_fd_and_unlock (server, client_fd)) _dbus_verbose ("Rejected client connection due to lack of memory\n"); } } @@ -246,6 +267,10 @@ _dbus_server_new_for_fd (int fd, return NULL; } +#ifndef DBUS_DISABLE_CHECKS + unix_server->base.have_server_lock = TRUE; +#endif + if (!_dbus_server_add_watch (&unix_server->base, watch)) { @@ -254,6 +279,10 @@ _dbus_server_new_for_fd (int fd, dbus_free (unix_server); return NULL; } + +#ifndef DBUS_DISABLE_CHECKS + unix_server->base.have_server_lock = FALSE; +#endif unix_server->fd = fd; unix_server->watch = watch; diff --git a/dbus/dbus-server.c b/dbus/dbus-server.c index 4c706191..788aaad7 100644 --- a/dbus/dbus-server.c +++ b/dbus/dbus-server.c @@ -67,7 +67,7 @@ _dbus_server_init_base (DBusServer *server, const DBusString *address) { server->vtable = vtable; - server->refcount = 1; + server->refcount.value = 1; server->address = NULL; server->watches = NULL; @@ -75,6 +75,10 @@ _dbus_server_init_base (DBusServer *server, if (!_dbus_string_copy_data (address, &server->address)) goto failed; + + server->mutex = dbus_mutex_new (); + if (server->mutex == NULL) + goto failed; server->watches = _dbus_watch_list_new (); if (server->watches == NULL) @@ -91,6 +95,11 @@ _dbus_server_init_base (DBusServer *server, return TRUE; failed: + if (server->mutex) + { + dbus_mutex_free (server->mutex); + server->mutex = NULL; + } if (server->watches) { _dbus_watch_list_free (server->watches); @@ -118,7 +127,7 @@ _dbus_server_init_base (DBusServer *server, */ void _dbus_server_finalize_base (DBusServer *server) -{ +{ /* calls out to application code... */ _dbus_data_slot_list_free (&server->slot_list); @@ -130,6 +139,8 @@ _dbus_server_finalize_base (DBusServer *server) _dbus_watch_list_free (server->watches); _dbus_timeout_list_free (server->timeouts); + dbus_mutex_free (server->mutex); + dbus_free (server->address); dbus_free_string_array (server->auth_mechanisms); @@ -146,6 +157,7 @@ dbus_bool_t _dbus_server_add_watch (DBusServer *server, DBusWatch *watch) { + HAVE_LOCK_CHECK (server); return _dbus_watch_list_add_watch (server->watches, watch); } @@ -159,6 +171,7 @@ void _dbus_server_remove_watch (DBusServer *server, DBusWatch *watch) { + HAVE_LOCK_CHECK (server); _dbus_watch_list_remove_watch (server->watches, watch); } @@ -176,6 +189,8 @@ _dbus_server_toggle_watch (DBusServer *server, DBusWatch *watch, dbus_bool_t enabled) { + HAVE_LOCK_CHECK (server); + if (server->watches) /* null during finalize */ _dbus_watch_list_toggle_watch (server->watches, watch, enabled); @@ -194,6 +209,8 @@ dbus_bool_t _dbus_server_add_timeout (DBusServer *server, DBusTimeout *timeout) { + HAVE_LOCK_CHECK (server); + return _dbus_timeout_list_add_timeout (server->timeouts, timeout); } @@ -207,6 +224,8 @@ void _dbus_server_remove_timeout (DBusServer *server, DBusTimeout *timeout) { + HAVE_LOCK_CHECK (server); + _dbus_timeout_list_remove_timeout (server->timeouts, timeout); } @@ -224,6 +243,8 @@ _dbus_server_toggle_timeout (DBusServer *server, DBusTimeout *timeout, dbus_bool_t enabled) { + HAVE_LOCK_CHECK (server); + if (server->timeouts) /* null during finalize */ _dbus_timeout_list_toggle_timeout (server->timeouts, timeout, enabled); @@ -457,8 +478,16 @@ DBusServer * dbus_server_ref (DBusServer *server) { _dbus_return_val_if_fail (server != NULL, NULL); - - server->refcount += 1; + +#ifdef DBUS_HAVE_ATOMIC_INT + _dbus_atomic_inc (&server->refcount); +#else + SERVER_LOCK (server); + _dbus_assert (server->refcount.value > 0); + + server->refcount.value += 1; + SERVER_UNLOCK (server); +#endif return server; } @@ -474,12 +503,24 @@ dbus_server_ref (DBusServer *server) void dbus_server_unref (DBusServer *server) { + dbus_bool_t last_unref; + _dbus_return_if_fail (server != NULL); - _dbus_assert (server->refcount > 0); +#ifdef DBUS_HAVE_ATOMIC_INT + last_unref = (_dbus_atomic_dec (&server->refcount) == 1); +#else + SERVER_LOCK (server); + + _dbus_assert (server->refcount.value > 0); - server->refcount -= 1; - if (server->refcount == 0) + server->refcount.value -= 1; + last_unref = (server->refcount.value == 0); + + SERVER_UNLOCK (server); +#endif + + if (last_unref) { _dbus_assert (server->vtable->finalize != NULL); @@ -488,6 +529,25 @@ dbus_server_unref (DBusServer *server) } /** + * Like dbus_server_ref() but does not acquire the lock (must already be held) + * + * @param server the server. + */ +void +_dbus_server_ref_unlocked (DBusServer *server) +{ + HAVE_LOCK_CHECK (server); + +#ifdef DBUS_HAVE_ATOMIC_INT + _dbus_atomic_inc (&server->refcount); +#else + _dbus_assert (server->refcount.value > 0); + + server->refcount.value += 1; +#endif +} + +/** * Releases the server's address and stops listening for * new clients. If called more than once, only the first * call has an effect. Does not modify the server's @@ -499,6 +559,8 @@ void dbus_server_disconnect (DBusServer *server) { _dbus_return_if_fail (server != NULL); + + SERVER_LOCK (server); _dbus_assert (server->vtable->disconnect != NULL); @@ -507,6 +569,8 @@ dbus_server_disconnect (DBusServer *server) (* server->vtable->disconnect) (server); server->disconnected = TRUE; + + SERVER_UNLOCK (server); } /** @@ -517,9 +581,15 @@ dbus_server_disconnect (DBusServer *server) dbus_bool_t dbus_server_get_is_connected (DBusServer *server) { - _dbus_return_val_if_fail (server != NULL, FALSE); + dbus_bool_t retval; - return !server->disconnected; + _dbus_return_val_if_fail (server != NULL, FALSE); + + SERVER_LOCK (server); + retval = !server->disconnected; + SERVER_UNLOCK (server); + + return retval; } /** @@ -532,9 +602,15 @@ dbus_server_get_is_connected (DBusServer *server) char* dbus_server_get_address (DBusServer *server) { - _dbus_return_val_if_fail (server != NULL, NULL); + char *retval; - return _dbus_strdup (server->address); + _dbus_return_val_if_fail (server != NULL, NULL); + + SERVER_LOCK (server); + retval = _dbus_strdup (server->address); + SERVER_UNLOCK (server); + + return retval; } /** @@ -555,14 +631,22 @@ dbus_server_set_new_connection_function (DBusServer *server, void *data, DBusFreeFunction free_data_function) { - _dbus_return_if_fail (server != NULL); + DBusFreeFunction old_free_function; + void *old_data; - if (server->new_connection_free_data_function != NULL) - (* server->new_connection_free_data_function) (server->new_connection_data); + _dbus_return_if_fail (server != NULL); + + SERVER_LOCK (server); + old_free_function = server->new_connection_free_data_function; + old_data = server->new_connection_data; server->new_connection_function = function; server->new_connection_data = data; server->new_connection_free_data_function = free_data_function; + SERVER_UNLOCK (server); + + if (old_free_function != NULL) + (* old_free_function) (old_data); } /** @@ -589,14 +673,34 @@ dbus_server_set_watch_functions (DBusServer *server, void *data, DBusFreeFunction free_data_function) { + dbus_bool_t result; + DBusWatchList *watches; + _dbus_return_val_if_fail (server != NULL, FALSE); + + SERVER_LOCK (server); + watches = server->watches; + server->watches = NULL; + if (watches) + { + SERVER_UNLOCK (server); + result = _dbus_watch_list_set_functions (watches, + add_function, + remove_function, + toggled_function, + data, + free_data_function); + SERVER_LOCK (server); + } + else + { + _dbus_warn ("Re-entrant call to %s\n", _DBUS_FUNCTION_NAME); + result = FALSE; + } + server->watches = watches; + SERVER_UNLOCK (server); - return _dbus_watch_list_set_functions (server->watches, - add_function, - remove_function, - toggled_function, - data, - free_data_function); + return result; } /** @@ -622,12 +726,34 @@ dbus_server_set_timeout_functions (DBusServer *server, void *data, DBusFreeFunction free_data_function) { + dbus_bool_t result; + DBusTimeoutList *timeouts; + _dbus_return_val_if_fail (server != NULL, FALSE); + + SERVER_LOCK (server); + timeouts = server->timeouts; + server->timeouts = NULL; + if (timeouts) + { + SERVER_UNLOCK (server); + result = _dbus_timeout_list_set_functions (timeouts, + add_function, + remove_function, + toggled_function, + data, + free_data_function); + SERVER_LOCK (server); + } + else + { + _dbus_warn ("Re-entrant call to %s\n", _DBUS_FUNCTION_NAME); + result = FALSE; + } + server->timeouts = timeouts; + SERVER_UNLOCK (server); - return _dbus_timeout_list_set_functions (server->timeouts, - add_function, remove_function, - toggled_function, - data, free_data_function); + return result; } /** @@ -647,6 +773,8 @@ dbus_server_set_auth_mechanisms (DBusServer *server, char **copy; _dbus_return_val_if_fail (server != NULL, FALSE); + + SERVER_LOCK (server); if (mechanisms != NULL) { @@ -660,6 +788,8 @@ dbus_server_set_auth_mechanisms (DBusServer *server, dbus_free_string_array (server->auth_mechanisms); server->auth_mechanisms = copy; + SERVER_UNLOCK (server); + return TRUE; } @@ -732,19 +862,16 @@ dbus_server_set_data (DBusServer *server, dbus_bool_t retval; _dbus_return_val_if_fail (server != NULL, FALSE); - -#if 0 - dbus_mutex_lock (server->mutex); -#endif + + SERVER_LOCK (server); retval = _dbus_data_slot_list_set (&slot_allocator, &server->slot_list, slot, data, free_data_func, &old_free_func, &old_data); -#if 0 - dbus_mutex_unlock (server->mutex); -#endif + + SERVER_UNLOCK (server); if (retval) { @@ -772,17 +899,13 @@ dbus_server_get_data (DBusServer *server, _dbus_return_val_if_fail (server != NULL, NULL); -#if 0 - dbus_mutex_lock (server->mutex); -#endif + SERVER_LOCK (server); res = _dbus_data_slot_list_get (&slot_allocator, &server->slot_list, slot); -#if 0 - dbus_mutex_unlock (server->mutex); -#endif + SERVER_UNLOCK (server); return res; } diff --git a/dbus/dbus-transport-unix.c b/dbus/dbus-transport-unix.c index 4190da46..2959886a 100644 --- a/dbus/dbus-transport-unix.c +++ b/dbus/dbus-transport-unix.c @@ -70,6 +70,8 @@ static void free_watches (DBusTransport *transport) { DBusTransportUnix *unix_transport = (DBusTransportUnix*) transport; + + _dbus_verbose ("%s start\n", _DBUS_FUNCTION_NAME); if (unix_transport->read_watch) { @@ -90,12 +92,16 @@ free_watches (DBusTransport *transport) _dbus_watch_unref (unix_transport->write_watch); unix_transport->write_watch = NULL; } + + _dbus_verbose ("%s end\n", _DBUS_FUNCTION_NAME); } static void unix_finalize (DBusTransport *transport) { DBusTransportUnix *unix_transport = (DBusTransportUnix*) transport; + + _dbus_verbose ("%s\n", _DBUS_FUNCTION_NAME); free_watches (transport); @@ -871,6 +877,8 @@ static void unix_disconnect (DBusTransport *transport) { DBusTransportUnix *unix_transport = (DBusTransportUnix*) transport; + + _dbus_verbose ("%s\n", _DBUS_FUNCTION_NAME); free_watches (transport); @@ -1004,7 +1012,10 @@ unix_do_iteration (DBusTransport *transport, * by the io_path_cond condvar, so we won't reenter this. */ if (flags & DBUS_ITERATION_BLOCK) - _dbus_connection_unlock (transport->connection); + { + _dbus_verbose ("unlock %s pre poll\n", _DBUS_FUNCTION_NAME); + _dbus_connection_unlock (transport->connection); + } again: poll_res = _dbus_poll (&poll_fd, 1, poll_timeout); @@ -1013,7 +1024,10 @@ unix_do_iteration (DBusTransport *transport, goto again; if (flags & DBUS_ITERATION_BLOCK) - _dbus_connection_lock (transport->connection); + { + _dbus_verbose ("lock %s post poll\n", _DBUS_FUNCTION_NAME); + _dbus_connection_lock (transport->connection); + } if (poll_res >= 0) { diff --git a/dbus/dbus-transport.c b/dbus/dbus-transport.c index 94d4a8a8..c08573d8 100644 --- a/dbus/dbus-transport.c +++ b/dbus/dbus-transport.c @@ -388,10 +388,12 @@ _dbus_transport_unref (DBusTransport *transport) { _dbus_assert (transport != NULL); _dbus_assert (transport->refcount > 0); - + transport->refcount -= 1; if (transport->refcount == 0) { + _dbus_verbose ("%s: finalizing\n", _DBUS_FUNCTION_NAME); + _dbus_assert (transport->vtable->finalize != NULL); (* transport->vtable->finalize) (transport); @@ -409,14 +411,18 @@ _dbus_transport_unref (DBusTransport *transport) void _dbus_transport_disconnect (DBusTransport *transport) { + _dbus_verbose ("%s start\n", _DBUS_FUNCTION_NAME); + _dbus_assert (transport->vtable->disconnect != NULL); - + if (transport->disconnected) return; (* transport->vtable->disconnect) (transport); transport->disconnected = TRUE; + + _dbus_verbose ("%s end\n", _DBUS_FUNCTION_NAME); } /** @@ -437,7 +443,8 @@ _dbus_transport_get_is_connected (DBusTransport *transport) * Returns #TRUE if we have been authenticated. Will return #TRUE * even if the transport is disconnected. * - * @todo needs to drop connection->mutex when calling the unix_user_function + * @todo we drop connection->mutex when calling the unix_user_function, + * which may not be safe really. * * @param transport the transport * @returns whether we're authenticated @@ -453,6 +460,9 @@ _dbus_transport_get_is_authenticated (DBusTransport *transport) if (transport->disconnected) return FALSE; + + /* paranoia ref since we call user callbacks sometimes */ + _dbus_connection_ref_unlocked (transport->connection); maybe_authenticated = (!(transport->send_credentials_pending || @@ -486,21 +496,40 @@ _dbus_transport_get_is_authenticated (DBusTransport *transport) if (transport->unix_user_function != NULL) { - /* FIXME we hold the connection lock here and should drop it */ - if (!(* transport->unix_user_function) (transport->connection, - auth_identity.uid, - transport->unix_user_data)) + dbus_bool_t allow; + DBusConnection *connection; + DBusAllowUnixUserFunction unix_user_function; + void *unix_user_data; + + /* Dropping the lock here probably isn't that safe. */ + + connection = transport->connection; + unix_user_function = transport->unix_user_function; + unix_user_data = transport->unix_user_data; + + _dbus_verbose ("unlock %s\n", _DBUS_FUNCTION_NAME); + _dbus_connection_unlock (connection); + + allow = (* unix_user_function) (connection, + auth_identity.uid, + unix_user_data); + + _dbus_verbose ("lock %s post unix user function\n", _DBUS_FUNCTION_NAME); + _dbus_connection_lock (connection); + + if (allow) + { + _dbus_verbose ("Client UID "DBUS_UID_FORMAT" authorized\n", auth_identity.uid); + } + else { _dbus_verbose ("Client UID "DBUS_UID_FORMAT " was rejected, disconnecting\n", auth_identity.uid); _dbus_transport_disconnect (transport); + _dbus_connection_unref_unlocked (connection); return FALSE; } - else - { - _dbus_verbose ("Client UID "DBUS_UID_FORMAT" authorized\n", auth_identity.uid); - } } else { @@ -515,6 +544,7 @@ _dbus_transport_get_is_authenticated (DBusTransport *transport) " but our UID is "DBUS_UID_FORMAT", disconnecting\n", auth_identity.uid, our_identity.uid); _dbus_transport_disconnect (transport); + _dbus_connection_unref_unlocked (transport->connection); return FALSE; } else @@ -527,8 +557,9 @@ _dbus_transport_get_is_authenticated (DBusTransport *transport) } transport->authenticated = maybe_authenticated; - - return transport->authenticated; + + _dbus_connection_unref_unlocked (transport->connection); + return maybe_authenticated; } } @@ -670,6 +701,8 @@ _dbus_transport_do_iteration (DBusTransport *transport, (* transport->vtable->do_iteration) (transport, flags, timeout_milliseconds); _dbus_transport_unref (transport); + + _dbus_verbose ("%s end\n", _DBUS_FUNCTION_NAME); } static dbus_bool_t @@ -31,6 +31,8 @@ Important for 1.0 - fix introspection format to handle all signatures + - make the mutex/condvar functions private + Important for 1.0 GLib Bindings === diff --git a/tools/Makefile.am b/tools/Makefile.am index b6747b0f..7cc6cadf 100644 --- a/tools/Makefile.am +++ b/tools/Makefile.am @@ -1,4 +1,4 @@ -INCLUDES=-I$(top_srcdir) $(DBUS_CLIENT_CFLAGS) $(DBUS_GLIB_CFLAGS) $(DBUS_X_CFLAGS) $(DBUS_GTK_CFLAGS) -DDBUS_LOCALEDIR=\"$(prefix)/@DATADIRNAME@/locale\" +INCLUDES=-I$(top_srcdir) $(DBUS_CLIENT_CFLAGS) $(DBUS_GLIB_CFLAGS) $(DBUS_X_CFLAGS) $(DBUS_GTK_THREADS_CFLAGS) -DDBUS_LOCALEDIR=\"$(prefix)/@DATADIRNAME@/locale\" if HAVE_GLIB GLIB_TOOLS=dbus-monitor @@ -40,7 +40,7 @@ dbus_viewer_SOURCES= \ dbus_send_LDADD= $(top_builddir)/dbus/libdbus-1.la dbus_monitor_LDADD= $(top_builddir)/glib/libdbus-glib-1.la dbus_launch_LDADD= $(DBUS_X_LIBS) -dbus_viewer_LDADD= $(DBUS_GLIB_TOOL_LIBS) $(top_builddir)/glib/libdbus-gtool.la $(DBUS_GTK_LIBS) +dbus_viewer_LDADD= $(DBUS_GLIB_TOOL_LIBS) $(top_builddir)/glib/libdbus-gtool.la $(DBUS_GTK_THREADS_LIBS) man_MANS = dbus-send.1 dbus-monitor.1 dbus-launch.1 dbus-cleanup-sockets.1 EXTRA_DIST = $(man_MANS) diff --git a/tools/dbus-tree-view.c b/tools/dbus-tree-view.c index f9342ee9..7f143ab9 100644 --- a/tools/dbus-tree-view.c +++ b/tools/dbus-tree-view.c @@ -333,6 +333,8 @@ info_set_func_text (GtkTreeViewColumn *tree_column, break; case INFO_TYPE_PROPERTY: g_string_append (str, "<i>property</i>"); + g_string_append_printf (str, " <b>%s</b>", + type_to_string (property_info_get_type ((PropertyInfo*)info))); break; case INFO_TYPE_ARG: g_string_append_printf (str, "<i>arg</i> %s", diff --git a/tools/dbus-viewer.c b/tools/dbus-viewer.c index c3903097..338b8e05 100644 --- a/tools/dbus-viewer.c +++ b/tools/dbus-viewer.c @@ -82,6 +82,54 @@ show_error_dialog (GtkWindow *transient_parent, } } +typedef struct +{ + DBusGConnection *connection; + + GtkWidget *window; + GtkWidget *treeview; + GtkWidget *name_menu; + + GtkTreeModel *names_model; + + GtkWidget *error_dialog; + +} TreeWindow; + + +static void +tree_window_set_node (TreeWindow *w, + NodeInfo *node) +{ + char **path; + const char *name; + + name = node_info_get_name (node); + if (name == NULL || + name[0] != '/') + { + g_printerr (_("Assuming root node is at path /, since no absolute path is specified")); + name = "/"; + } + + path = _dbus_gutils_split_path (name); + + dbus_tree_view_update (GTK_TREE_VIEW (w->treeview), + (const char**) path, + node); + + g_strfreev (path); +} + +typedef struct +{ + DBusGConnection *connection; + char *service_name; + GError *error; + NodeInfo *node; + TreeWindow *window; /* Not touched from child thread */ +} LoadFromServiceData; + static gboolean load_child_nodes (const char *service_name, NodeInfo *parent, @@ -179,16 +227,52 @@ load_child_nodes (const char *service_name, return TRUE; } -static NodeInfo* -load_from_service (DBusGConnection *connection, - const char *service_name, - GError **error) +static gboolean +load_from_service_complete_idle (void *data) { - DBusGProxy *root_proxy; + /* Called in main thread */ + GThread *thread = data; + LoadFromServiceData *d; + NodeInfo *node; + + d = g_thread_join (thread); + + node = d->node; + + if (d->error) + { + g_assert (d->node == NULL); + show_error_dialog (GTK_WINDOW (d->window->window), &d->window->error_dialog, + _("Unable to load \"%s\": %s\n"), + d->service_name, d->error->message); + g_error_free (d->error); + } + else + { + g_assert (d->error == NULL); + + tree_window_set_node (d->window, node); + node_info_unref (node); + } + + g_free (d->service_name); + dbus_g_connection_unref (d->connection); + g_free (d); + + return FALSE; +} + +static void* +load_from_service_thread_func (void *thread_data) +{ + DBusGProxy *root_proxy; DBusGPendingCall *call; const char *data; NodeInfo *node; GString *path; + LoadFromServiceData *lfsd; + + lfsd = thread_data; node = NULL; call = NULL; @@ -196,22 +280,22 @@ load_from_service (DBusGConnection *connection, #if 1 /* this will end up autolaunching the service when we introspect it */ - root_proxy = dbus_g_proxy_new_for_name (connection, - service_name, + root_proxy = dbus_g_proxy_new_for_name (lfsd->connection, + lfsd->service_name, "/", DBUS_INTERFACE_ORG_FREEDESKTOP_INTROSPECTABLE); g_assert (root_proxy != NULL); #else /* this will be an error if the service doesn't exist */ - root_proxy = dbus_g_proxy_new_for_name_owner (connection, - service_name, + root_proxy = dbus_g_proxy_new_for_name_owner (lfsd->connection, + lfsd->service_name, "/", DBUS_INTERFACE_ORG_FREEDESKTOP_INTROSPECTABLE, - error); + &lfsd->error); if (root_proxy == NULL) { - g_printerr ("Failed to get owner of '%s'\n", service_name); - return NULL; + g_printerr ("Failed to get owner of '%s'\n", lfsd->service_name); + return lfsd->data; } #endif @@ -219,7 +303,7 @@ load_from_service (DBusGConnection *connection, DBUS_TYPE_INVALID); data = NULL; - if (!dbus_g_proxy_end_call (root_proxy, call, error, DBUS_TYPE_STRING, &data, + if (!dbus_g_proxy_end_call (root_proxy, call, &lfsd->error, DBUS_TYPE_STRING, &data, DBUS_TYPE_INVALID)) { g_printerr ("Failed to Introspect() %s\n", @@ -227,7 +311,7 @@ load_from_service (DBusGConnection *connection, goto out; } - node = description_load_from_string (data, -1, error); + node = description_load_from_string (data, -1, &lfsd->error); /* g_print ("%s\n", data); */ @@ -239,7 +323,7 @@ load_from_service (DBusGConnection *connection, path = g_string_new ("/"); if (!load_child_nodes (dbus_g_proxy_get_bus_name (root_proxy), - node, path, error)) + node, path, &lfsd->error)) { node_info_unref (node); node = NULL; @@ -254,70 +338,40 @@ load_from_service (DBusGConnection *connection, if (path) g_string_free (path, TRUE); - - return node; -} -typedef struct -{ - DBusGConnection *connection; - - GtkWidget *window; - GtkWidget *treeview; - GtkWidget *name_menu; - - GtkTreeModel *names_model; + lfsd->node = node; + g_assert (lfsd->node || lfsd->error); + g_assert (lfsd->node == NULL || lfsd->error == NULL); - GtkWidget *error_dialog; + /* Add idle to main thread that will join us back */ + g_idle_add (load_from_service_complete_idle, g_thread_self ()); -} TreeWindow; + return lfsd; +} static void -tree_window_set_node (TreeWindow *w, - NodeInfo *node) +start_load_from_service (TreeWindow *w, + DBusGConnection *connection, + const char *service_name) { - char **path; - const char *name; + LoadFromServiceData *d; - name = node_info_get_name (node); - if (name == NULL || - name[0] != '/') - { - g_printerr (_("Assuming root node is at path /, since no absolute path is specified")); - name = "/"; - } - - path = _dbus_gutils_split_path (name); + d = g_new0 (LoadFromServiceData, 1); - dbus_tree_view_update (GTK_TREE_VIEW (w->treeview), - (const char**) path, - node); - - g_strfreev (path); + d->connection = dbus_g_connection_ref (connection); + d->service_name = g_strdup (service_name); + d->error = NULL; + d->node = NULL; + d->window = w; + + g_thread_create (load_from_service_thread_func, d, TRUE, NULL); } static void tree_window_set_service (TreeWindow *w, const char *service_name) { - GError *error; - NodeInfo *node; - - error = NULL; - node = load_from_service (w->connection, service_name, &error); - if (node == NULL) - { - g_assert (error != NULL); - show_error_dialog (GTK_WINDOW (w->window), &w->error_dialog, - _("Unable to load \"%s\": %s\n"), - service_name, error->message); - g_error_free (error); - return; - } - - tree_window_set_node (w, node); - - node_info_unref (node); + start_load_from_service (w, w->connection, service_name); } static void @@ -444,10 +498,13 @@ main (int argc, char **argv) DBusGConnection *connection; GError *error; GtkTreeModel *names_model; + + g_thread_init (NULL); + dbus_g_thread_init (); bindtextdomain (GETTEXT_PACKAGE, DBUS_LOCALEDIR); bind_textdomain_codeset (GETTEXT_PACKAGE, "UTF-8"); - textdomain (GETTEXT_PACKAGE); + textdomain (GETTEXT_PACKAGE); gtk_init (&argc, &argv); |