fdo-mirrors/dbus
1
0
Fork 0
mirror of https://gitlab.freedesktop.org/dbus/dbus.git synced 2026-01-07 07:40:19 +01:00
Code Issues Projects Releases Packages Wiki Activity Actions

Fix the reentrancy issue reported on bug 17754.

Browse source 
Patch based on patch by Havoc Pennington, with the references that
this is temporary removed.

        Patch based on one from Olivier Hochreutiner <olivier.hochreutiner
        gmail.com>

        * dbus/dbus-connection.c (protected_change_timeout): remove the
        elaborate nonworking hack to try to drop locks and just keep the
        locks; this isn't right either, but at least is correct, though
        it puts restrictions on apps.

        * dbus/dbus-connection.c (protected_change_watch): make the same
        change as for timeouts

        * dbus/dbus-connection.c (dbus_connection_set_timeout_functions):
        don't drop the lock here; add documentation of the problem to API
        docs
        (dbus_connection_set_watch_functions): same

        * dbus/dbus-connection.c (dbus_connection_get_data)
        (dbus_connection_set_data): introduce a separate slot_mutex
        protecting connection->slot_list so these two functions can be
        called inside watch and timeout functions. Not sure this
        is going to be a good idea.

        * dbus/dbus-connection.c (dbus_connection_unref)
        (dbus_connection_ref): avoid using connection lock in ref/unref
        so these can also be used in watch and timeout functions
This commit is contained in:
Thiago Macieira 2010-06-22 15:13:23 +02:00
parent 591236a669
commit 6ff1d07931
1 changed files with 110 additions and 96 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

206
dbus/dbus-connection.c
View file

@ -75,6 +75,14 @@
_dbus_mutex_unlock ((connection)->mutex); \
} while (0)
#define SLOTS_LOCK(connection) do { \
_dbus_mutex_lock ((connection)->slot_mutex); \
} while (0)
#define SLOTS_UNLOCK(connection) do { \
_dbus_mutex_unlock ((connection)->slot_mutex); \
} while (0)
#define DISPATCH_STATUS_NAME(s) \
((s) == DBUS_DISPATCH_COMPLETE ? "complete" : \
(s) == DBUS_DISPATCH_DATA_REMAINS ? "data remains" : \
@ -263,6 +271,7 @@ struct DBusConnection
DBusList *filter_list; /**< List of filters. */
DBusMutex *slot_mutex; /**< Lock on slot_list so overall connection lock need not be taken */
DBusDataSlotList slot_list; /**< Data stored by allocated integer ID */
DBusHashTable *pending_replies; /**< Hash of message serials to #DBusPendingCall. */
@ -655,39 +664,42 @@ protected_change_watch (DBusConnection *connection,
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
/* The original purpose of protected_change_watch() was to hold a
* ref on the connection while dropping the connection lock, then
* calling out to the app. This was a broken hack that did not
* work, since the connection was in a hosed state (no WatchList
* field) while calling out.
*
* So for now we'll just keep the lock while calling out. This means
* apps are not allowed to call DBusConnection methods inside a
* watch function or they will deadlock.
*
* The "real fix" is to use the _and_unlock() pattern found
* elsewhere in the code, to defer calling out to the app until
* we're about to drop locks and return flow of control to the app
* anyway.
*
* See http://lists.freedesktop.org/archives/dbus/2007-July/thread.html#8144
*/
watches = connection->watches;
if (watches)
{
connection->watches = NULL;
_dbus_connection_ref_unlocked (connection);
CONNECTION_UNLOCK (connection);
if (connection->watches)
{
if (add_function)
retval = (* add_function) (watches, watch);
retval = (* add_function) (connection->watches, watch);
else if (remove_function)
{
retval = TRUE;
(* remove_function) (watches, watch);
(* remove_function) (connection->watches, watch);
}
else
{
retval = TRUE;
(* toggle_function) (watches, watch, enabled);
(* toggle_function) (connection->watches, watch, enabled);
}
CONNECTION_LOCK (connection);
connection->watches = watches;
_dbus_connection_unref_unlocked (connection);
return retval;
}
else
@ -776,39 +788,42 @@ protected_change_timeout (DBusConnection *connection,
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
/* The original purpose of protected_change_timeout() was to hold a
* ref on the connection while dropping the connection lock, then
* calling out to the app. This was a broken hack that did not
* work, since the connection was in a hosed state (no TimeoutList
* field) while calling out.
*
* So for now we'll just keep the lock while calling out. This means
* apps are not allowed to call DBusConnection methods inside a
* timeout function or they will deadlock.
*
* The "real fix" is to use the _and_unlock() pattern found
* elsewhere in the code, to defer calling out to the app until
* we're about to drop locks and return flow of control to the app
* anyway.
*
* See http://lists.freedesktop.org/archives/dbus/2007-July/thread.html#8144
*/
timeouts = connection->timeouts;
if (timeouts)
{
connection->timeouts = NULL;
_dbus_connection_ref_unlocked (connection);
CONNECTION_UNLOCK (connection);
if (connection->timeouts)
{
if (add_function)
retval = (* add_function) (timeouts, timeout);
retval = (* add_function) (connection->timeouts, timeout);
else if (remove_function)
{
retval = TRUE;
(* remove_function) (timeouts, timeout);
(* remove_function) (connection->timeouts, timeout);
}
else
{
retval = TRUE;
(* toggle_function) (timeouts, timeout, enabled);
(* toggle_function) (connection->timeouts, timeout, enabled);
}
CONNECTION_LOCK (connection);
connection->timeouts = timeouts;
_dbus_connection_unref_unlocked (connection);
return retval;
}
else
@ -1269,6 +1284,10 @@ _dbus_connection_new_for_transport (DBusTransport *transport)
if (connection->io_path_cond == NULL)
goto error;
_dbus_mutex_new_at_location (&connection->slot_mutex);
if (connection->slot_mutex == NULL)
goto error;
disconnect_message = dbus_message_new_signal (DBUS_PATH_LOCAL,
DBUS_INTERFACE_LOCAL,
"Disconnected");
@ -1345,6 +1364,7 @@ _dbus_connection_new_for_transport (DBusTransport *transport)
_dbus_mutex_free_at_location (&connection->mutex);
_dbus_mutex_free_at_location (&connection->io_path_mutex);
_dbus_mutex_free_at_location (&connection->dispatch_mutex);
_dbus_mutex_free_at_location (&connection->slot_mutex);
dbus_free (connection);
}
if (pending_replies)
@ -2618,9 +2638,14 @@ dbus_connection_ref (DBusConnection *connection)
/* The connection lock is better than the global
* lock in the atomic increment fallback
*
* (FIXME but for now we always use the atomic version,
* to avoid taking the connection lock, due to
* the mess with set_timeout_functions()/set_watch_functions()
* calling out to the app without dropping locks)
*/
#ifdef DBUS_HAVE_ATOMIC_INT
#if 1
_dbus_atomic_inc (&connection->refcount);
#else
CONNECTION_LOCK (connection);
@ -2732,6 +2757,8 @@ _dbus_connection_last_unref (DBusConnection *connection)
_dbus_mutex_free_at_location (&connection->io_path_mutex);
_dbus_mutex_free_at_location (&connection->dispatch_mutex);
_dbus_mutex_free_at_location (&connection->slot_mutex);
_dbus_mutex_free_at_location (&connection->mutex);
dbus_free (connection);
@ -2766,9 +2793,14 @@ dbus_connection_unref (DBusConnection *connection)
/* The connection lock is better than the global
* lock in the atomic increment fallback
*
* (FIXME but for now we always use the atomic version,
* to avoid taking the connection lock, due to
* the mess with set_timeout_functions()/set_watch_functions()
* calling out to the app without dropping locks)
*/
#ifdef DBUS_HAVE_ATOMIC_INT
#if 1
last_unref = (_dbus_atomic_dec (&connection->refcount) == 1);
#else
CONNECTION_LOCK (connection);
@ -4819,9 +4851,11 @@ dbus_connection_dispatch (DBusConnection *connection)
* should be that dbus_connection_set_watch_functions() has no effect,
* but the add_function and remove_function may have been called.
*
* @todo We need to drop the lock when we call the
* add/remove/toggled functions which can be a side effect
* of setting the watch functions.
* @note The thread lock on DBusConnection is held while
* watch functions are invoked, so inside these functions you
* may not invoke any methods on DBusConnection or it will deadlock.
* See the comments in the code or http://lists.freedesktop.org/archives/dbus/2007-July/tread.html#8144
* if you encounter this issue and want to attempt writing a patch.
*
* @param connection the connection.
* @param add_function function to begin monitoring a new descriptor.
@ -4840,42 +4874,18 @@ 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_check_failed ("Re-entrant call is not allowed\n");
return FALSE;
}
#endif
/* ref connection for slightly better reentrancy */
_dbus_connection_ref_unlocked (connection);
/* 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.
*/
watches = connection->watches;
connection->watches = NULL;
CONNECTION_UNLOCK (connection);
retval = _dbus_watch_list_set_functions (watches,
retval = _dbus_watch_list_set_functions (connection->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;
}
@ -4904,6 +4914,12 @@ dbus_connection_set_watch_functions (DBusConnection *connection,
* given remove_function. The timer interval may change whenever the
* timeout is added, removed, or toggled.
*
* @note The thread lock on DBusConnection is held while
* timeout functions are invoked, so inside these functions you
* may not invoke any methods on DBusConnection or it will deadlock.
* See the comments in the code or http://lists.freedesktop.org/archives/dbus/2007-July/thread.html#8144
* if you encounter this issue and want to attempt writing a patch.
*
* @param connection the connection.
* @param add_function function to add a timeout.
* @param remove_function function to remove a timeout.
@ -4921,37 +4937,17 @@ 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_check_failed ("Re-entrant call is not allowed\n");
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 (timeouts,
retval = _dbus_timeout_list_set_functions (connection->timeouts,
add_function, remove_function,
toggled_function,
data, free_data_function);
CONNECTION_LOCK (connection);
connection->timeouts = timeouts;
CONNECTION_UNLOCK (connection);
/* drop our paranoid refcount */
dbus_connection_unref (connection);
return retval;
}
@ -5911,6 +5907,15 @@ dbus_connection_free_data_slot (dbus_int32_t *slot_p)
* the connection is finalized. The slot number
* must have been allocated with dbus_connection_allocate_data_slot().
*
* @note This function does not take the
* main thread lock on DBusConnection, which allows it to be
* used from inside watch and timeout functions. (See the
* note in docs for dbus_connection_set_watch_functions().)
* A side effect of this is that you need to know there's
* a reference held on the connection while invoking
* dbus_connection_set_data(), or the connection could be
* finalized during dbus_connection_set_data().
*
* @param connection the connection
* @param slot the slot number
* @param data the data to store
@ -5930,14 +5935,14 @@ dbus_connection_set_data (DBusConnection *connection,
_dbus_return_val_if_fail (connection != NULL, FALSE);
_dbus_return_val_if_fail (slot >= 0, FALSE);
CONNECTION_LOCK (connection);
SLOTS_LOCK (connection);
retval = _dbus_data_slot_list_set (&slot_allocator,
&connection->slot_list,
slot, data, free_data_func,
&old_free_func, &old_data);
CONNECTION_UNLOCK (connection);
SLOTS_UNLOCK (connection);
if (retval)
{
@ -5953,6 +5958,15 @@ dbus_connection_set_data (DBusConnection *connection,
* Retrieves data previously set with dbus_connection_set_data().
* The slot must still be allocated (must not have been freed).
*
* @note This function does not take the
* main thread lock on DBusConnection, which allows it to be
* used from inside watch and timeout functions. (See the
* note in docs for dbus_connection_set_watch_functions().)
* A side effect of this is that you need to know there's
* a reference held on the connection while invoking
* dbus_connection_get_data(), or the connection could be
* finalized during dbus_connection_get_data().
*
* @param connection the connection
* @param slot the slot to get data from
* @returns the data, or #NULL if not found
@ -5965,13 +5979,13 @@ dbus_connection_get_data (DBusConnection *connection,
_dbus_return_val_if_fail (connection != NULL, NULL);
CONNECTION_LOCK (connection);
SLOTS_LOCK (connection);
res = _dbus_data_slot_list_get (&slot_allocator,
&connection->slot_list,
slot);
CONNECTION_UNLOCK (connection);
SLOTS_UNLOCK (connection);
return res;
}