From 79494f75e4109e028a30e29d418ce1c953751208 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Mon, 15 Apr 2013 20:50:02 +0100 Subject: [PATCH 6/8] Refactor global locks to use a simple array initialized on first use This is at least as thread-safe than what we already had: if dbus_threads_init_default() is called first, then the array is populated, and if it isn't, we populate it on first use. The remaining broken case is that two threads both start using libdbus at the same time, with no prior initialization. That's already broken, so there's nothing we can do to fix it. This makes it possible for _dbus_global_lock() (the former _DBUS_LOCK()) and _dbus_user_database_lock_system() to fail with OOM; compensate for that. This also removes the unused locks win_fds and sid_atom_cache, which were presumably once used on Windows, but are no longer mentioned anywhere. It also renames the existing static _dbus_global_lock(), _dbus_global_unlock() in dbus-sysdeps-win to _dbus_systemwide_lock(), _dbus_systemwide_unlock() which describe their semantics better (they deal with named OS-wide locks, rather than being local to a process like pthreads mutexes). --- dbus/dbus-bus.c | 60 +++++++++------ dbus/dbus-connection.c | 83 ++++++++++++--------- dbus/dbus-dataslot.c | 72 +++++++----------- dbus/dbus-dataslot.h | 8 +- dbus/dbus-internals.c | 45 ++---------- dbus/dbus-internals.h | 31 +------- dbus/dbus-list.c | 37 +++++++--- dbus/dbus-memory.c | 21 ++++-- dbus/dbus-message.c | 34 +++++---- dbus/dbus-pending-call.c | 5 +- dbus/dbus-server.c | 6 +- dbus/dbus-sysdeps-pthread.c | 8 ++ dbus/dbus-sysdeps-thread-win.c | 8 ++ dbus/dbus-sysdeps-win.c | 30 ++++---- dbus/dbus-sysdeps.c | 4 - dbus/dbus-threads-internal.h | 30 ++++++++ dbus/dbus-threads.c | 157 +++++++++++++++++++++++----------------- dbus/dbus-userdb-util.c | 23 +++++- dbus/dbus-userdb.c | 45 +++++++++--- dbus/dbus-userdb.h | 2 +- 20 files changed, 404 insertions(+), 305 deletions(-) diff --git a/dbus/dbus-bus.c b/dbus/dbus-bus.c index 6f81c74..1389ea8 100644 --- a/dbus/dbus-bus.c +++ b/dbus/dbus-bus.c @@ -98,7 +98,7 @@ static dbus_bool_t initialized = FALSE; /** * Lock for globals in this file */ -_DBUS_DEFINE_GLOBAL_LOCK (bus); +#define _DBUS_LOCK_BUS _DBUS_LOCK_BUS /** * Global lock covering all BusData on any connection. The bet is @@ -106,7 +106,7 @@ _DBUS_DEFINE_GLOBAL_LOCK (bus); * for a per-connection lock, but it's tough to imagine it mattering * either way. */ -_DBUS_DEFINE_GLOBAL_LOCK (bus_datas); +#define _DBUS_LOCK_BUS_DATAS _DBUS_LOCK_BUS_DATAS static void addresses_shutdown_func (void *data) @@ -330,7 +330,9 @@ bus_data_free (void *data) if (bd->is_well_known) { int i; - _DBUS_LOCK (bus); + /* we initialized global locks before we allocated @data */ + _dbus_global_lock_cannot_fail (_DBUS_LOCK_BUS); + /* We may be stored in more than one slot */ /* This should now be impossible - these slots are supposed to * be cleared on disconnect, so should not need to be cleared on @@ -344,7 +346,7 @@ bus_data_free (void *data) ++i; } - _DBUS_UNLOCK (bus); + _dbus_global_unlock (_DBUS_LOCK_BUS); } dbus_free (bd->unique_name); @@ -401,8 +403,13 @@ void _dbus_bus_notify_shared_connection_disconnected_unlocked (DBusConnection *connection) { int i; - - _DBUS_LOCK (bus); + + if (!_dbus_global_lock (_DBUS_LOCK_BUS)) + { + /* if it was a shared connection then we would have allocated global + * locks before we put it in bus_connections, so do nothing */ + return; + } /* 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 @@ -418,7 +425,7 @@ _dbus_bus_notify_shared_connection_disconnected_unlocked (DBusConnection *connec } } - _DBUS_UNLOCK (bus); + _dbus_global_unlock (_DBUS_LOCK_BUS); } static DBusConnection * @@ -436,7 +443,11 @@ internal_bus_get (DBusBusType type, connection = NULL; - _DBUS_LOCK (bus); + if (!_dbus_global_lock (_DBUS_LOCK_BUS)) + { + _DBUS_SET_OOM (error); + return NULL; + } if (!init_connections_unlocked ()) { @@ -506,20 +517,21 @@ internal_bus_get (DBusBusType type, */ dbus_connection_set_exit_on_disconnect (connection, TRUE); - - _DBUS_LOCK (bus_datas); + + /* we allocated global locks further up */ + _dbus_global_lock_cannot_fail (_DBUS_LOCK_BUS_DATAS); bd = ensure_bus_data (connection); _dbus_assert (bd != NULL); /* it should have been created on register, so OOM not possible */ bd->is_well_known = TRUE; - _DBUS_UNLOCK (bus_datas); + _dbus_global_unlock (_DBUS_LOCK_BUS_DATAS); out: /* Return a reference to the caller, or NULL with error set. */ if (connection == NULL) _DBUS_ASSERT_ERROR_IS_SET (error); - _DBUS_UNLOCK (bus); + _dbus_global_unlock (_DBUS_LOCK_BUS); return connection; } @@ -660,7 +672,11 @@ dbus_bus_register (DBusConnection *connection, message = NULL; reply = NULL; - _DBUS_LOCK (bus_datas); + if (!_dbus_global_lock (_DBUS_LOCK_BUS_DATAS)) + { + _DBUS_SET_OOM (error); + return FALSE; + } bd = ensure_bus_data (connection); if (bd == NULL) @@ -710,7 +726,7 @@ dbus_bus_register (DBusConnection *connection, retval = TRUE; out: - _DBUS_UNLOCK (bus_datas); + _dbus_global_unlock (_DBUS_LOCK_BUS_DATAS); if (message) dbus_message_unref (message); @@ -769,8 +785,9 @@ dbus_bus_set_unique_name (DBusConnection *connection, _dbus_return_val_if_fail (connection != NULL, FALSE); _dbus_return_val_if_fail (unique_name != NULL, FALSE); - _DBUS_LOCK (bus_datas); - + if (!_dbus_global_lock (_DBUS_LOCK_BUS_DATAS)) + return FALSE; + bd = ensure_bus_data (connection); if (bd == NULL) goto out; @@ -781,8 +798,8 @@ dbus_bus_set_unique_name (DBusConnection *connection, success = bd->unique_name != NULL; out: - _DBUS_UNLOCK (bus_datas); - + _dbus_global_unlock (_DBUS_LOCK_BUS_DATAS); + return success; } @@ -812,8 +829,9 @@ dbus_bus_get_unique_name (DBusConnection *connection) _dbus_return_val_if_fail (connection != NULL, NULL); - _DBUS_LOCK (bus_datas); - + if (!_dbus_global_lock (_DBUS_LOCK_BUS_DATAS)) + return NULL; + bd = ensure_bus_data (connection); if (bd == NULL) goto out; @@ -821,7 +839,7 @@ dbus_bus_get_unique_name (DBusConnection *connection) unique_name = bd->unique_name; out: - _DBUS_UNLOCK (bus_datas); + _dbus_global_unlock (_DBUS_LOCK_BUS_DATAS); return unique_name; } diff --git a/dbus/dbus-connection.c b/dbus/dbus-connection.c index 66315b3..0d92208 100644 --- a/dbus/dbus-connection.c +++ b/dbus/dbus-connection.c @@ -1531,7 +1531,7 @@ _dbus_connection_handle_watch (DBusWatch *watch, return retval; } -_DBUS_DEFINE_GLOBAL_LOCK (shared_connections); +/* Protected by _DBUS_LOCK_SHARED_CONNECTIONS */ static DBusHashTable *shared_connections = NULL; static DBusList *shared_connections_no_guid = NULL; @@ -1555,9 +1555,10 @@ static void shared_connections_shutdown (void *data) { int n_entries; - - _DBUS_LOCK (shared_connections); - + + /* We initialized global locks before we set up the shutdown function. */ + _dbus_global_lock_cannot_fail (_DBUS_LOCK_SHARED_CONNECTIONS); + /* This is a little bit unpleasant... better ideas? */ while ((n_entries = _dbus_hash_table_get_n_entries (shared_connections)) > 0) { @@ -1569,9 +1570,10 @@ shared_connections_shutdown (void *data) connection = _dbus_hash_iter_get_value (&iter); - _DBUS_UNLOCK (shared_connections); + _dbus_global_unlock (_DBUS_LOCK_SHARED_CONNECTIONS); close_connection_on_shutdown (connection); - _DBUS_LOCK (shared_connections); + /* we were holding the lock already, so we must have initialized */ + _dbus_global_lock_cannot_fail (_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); @@ -1588,16 +1590,17 @@ shared_connections_shutdown (void *data) connection = _dbus_list_pop_first (&shared_connections_no_guid); while (connection != NULL) { - _DBUS_UNLOCK (shared_connections); + _dbus_global_unlock (_DBUS_LOCK_SHARED_CONNECTIONS); close_connection_on_shutdown (connection); - _DBUS_LOCK (shared_connections); + /* we were holding the lock already, so we must have initialized */ + _dbus_global_lock_cannot_fail (_DBUS_LOCK_SHARED_CONNECTIONS); connection = _dbus_list_pop_first (&shared_connections_no_guid); } } shared_connections_no_guid = NULL; - - _DBUS_UNLOCK (shared_connections); + + _dbus_global_unlock (_DBUS_LOCK_SHARED_CONNECTIONS); } static dbus_bool_t @@ -1607,8 +1610,13 @@ connection_lookup_shared (DBusAddressEntry *entry, _dbus_verbose ("checking for existing connection\n"); *result = NULL; - - _DBUS_LOCK (shared_connections); + + if (!_dbus_global_lock (_DBUS_LOCK_SHARED_CONNECTIONS)) + { + /* If we had this connection, then we would have allocated global locks + * at the time we stored it; so logically, we can't have it. */ + return FALSE; + } if (shared_connections == NULL) { @@ -1619,7 +1627,7 @@ connection_lookup_shared (DBusAddressEntry *entry, NULL); if (shared_connections == NULL) { - _DBUS_UNLOCK (shared_connections); + _dbus_global_unlock (_DBUS_LOCK_SHARED_CONNECTIONS); return FALSE; } @@ -1627,13 +1635,13 @@ connection_lookup_shared (DBusAddressEntry *entry, { _dbus_hash_table_unref (shared_connections); shared_connections = NULL; - _DBUS_UNLOCK (shared_connections); + _dbus_global_unlock (_DBUS_LOCK_SHARED_CONNECTIONS); return FALSE; } _dbus_verbose (" successfully created shared_connections\n"); - - _DBUS_UNLOCK (shared_connections); + + _dbus_global_unlock (_DBUS_LOCK_SHARED_CONNECTIONS); return TRUE; /* no point looking up in the hash we just made */ } else @@ -1681,8 +1689,8 @@ connection_lookup_shared (DBusAddressEntry *entry, CONNECTION_UNLOCK (connection); } } - - _DBUS_UNLOCK (shared_connections); + + _dbus_global_unlock (_DBUS_LOCK_SHARED_CONNECTIONS); return TRUE; } } @@ -1706,15 +1714,16 @@ connection_record_shared_unlocked (DBusConnection *connection, if (guid == NULL) { - _DBUS_LOCK (shared_connections); + if (!_dbus_global_lock (_DBUS_LOCK_SHARED_CONNECTIONS)) + return FALSE; if (!_dbus_list_prepend (&shared_connections_no_guid, connection)) { - _DBUS_UNLOCK (shared_connections); + _dbus_global_unlock (_DBUS_LOCK_SHARED_CONNECTIONS); return FALSE; } - _DBUS_UNLOCK (shared_connections); + _dbus_global_unlock (_DBUS_LOCK_SHARED_CONNECTIONS); return TRUE; /* don't store in the hash */ } @@ -1733,8 +1742,10 @@ connection_record_shared_unlocked (DBusConnection *connection, dbus_free (guid_key); return FALSE; } - - _DBUS_LOCK (shared_connections); + + if (!_dbus_global_lock (_DBUS_LOCK_SHARED_CONNECTIONS)) + return FALSE; + _dbus_assert (shared_connections != NULL); if (!_dbus_hash_table_insert_string (shared_connections, @@ -1742,7 +1753,7 @@ connection_record_shared_unlocked (DBusConnection *connection, { dbus_free (guid_key); dbus_free (guid_in_connection); - _DBUS_UNLOCK (shared_connections); + _dbus_global_unlock (_DBUS_LOCK_SHARED_CONNECTIONS); return FALSE; } @@ -1750,8 +1761,8 @@ connection_record_shared_unlocked (DBusConnection *connection, _dbus_verbose ("stored connection to %s to be shared\n", connection->server_guid); - - _DBUS_UNLOCK (shared_connections); + + _dbus_global_unlock (_DBUS_LOCK_SHARED_CONNECTIONS); _dbus_assert (connection->server_guid != NULL); @@ -1765,9 +1776,14 @@ connection_forget_shared_unlocked (DBusConnection *connection) if (!connection->shareable) return; - - _DBUS_LOCK (shared_connections); - + + if (!_dbus_global_lock (_DBUS_LOCK_SHARED_CONNECTIONS)) + { + /* If it was there, we'd have held the lock while we put it in; + * so if global locks were never initialized, it can't be there. */ + return; + } + if (connection->server_guid != NULL) { _dbus_verbose ("dropping connection to %s out of the shared table\n", @@ -1785,8 +1801,8 @@ connection_forget_shared_unlocked (DBusConnection *connection) _dbus_list_remove (&shared_connections_no_guid, connection); } - _DBUS_UNLOCK (shared_connections); - + _dbus_global_unlock (_DBUS_LOCK_SHARED_CONNECTIONS); + /* remove our reference held on all shareable connections */ _dbus_connection_unref_unlocked (connection); } @@ -5852,8 +5868,8 @@ dbus_connection_list_registered (DBusConnection *connection, return retval; } -static DBusDataSlotAllocator slot_allocator; -_DBUS_DEFINE_GLOBAL_LOCK (connection_slots); +static DBusDataSlotAllocator slot_allocator = + DBUS_DATA_SLOT_ALLOCATOR_INIT (_DBUS_LOCK_CONNECTION_SLOTS); /** * Allocates an integer ID to be used for storing application-specific @@ -5873,7 +5889,6 @@ dbus_bool_t dbus_connection_allocate_data_slot (dbus_int32_t *slot_p) { return _dbus_data_slot_allocator_alloc (&slot_allocator, - &_DBUS_LOCK_NAME (connection_slots), slot_p); } diff --git a/dbus/dbus-dataslot.c b/dbus/dbus-dataslot.c index 0369612..656b13e 100644 --- a/dbus/dbus-dataslot.c +++ b/dbus/dbus-dataslot.c @@ -43,12 +43,13 @@ * @param allocator the allocator to initialize */ dbus_bool_t -_dbus_data_slot_allocator_init (DBusDataSlotAllocator *allocator) +_dbus_data_slot_allocator_init (DBusDataSlotAllocator *allocator, + DBusGlobalMutex mutex) { allocator->allocated_slots = NULL; allocator->n_allocated_slots = 0; allocator->n_used_slots = 0; - allocator->lock_loc = NULL; + allocator->mutex = mutex; return TRUE; } @@ -61,29 +62,17 @@ _dbus_data_slot_allocator_init (DBusDataSlotAllocator *allocator) * is allocated and stored at *slot_id_p. * * @param allocator the allocator - * @param mutex_loc the location lock for this allocator * @param slot_id_p address to fill with the slot ID * @returns #TRUE on success */ dbus_bool_t _dbus_data_slot_allocator_alloc (DBusDataSlotAllocator *allocator, - DBusRMutex **mutex_loc, dbus_int32_t *slot_id_p) { dbus_int32_t slot; - _dbus_rmutex_lock (*mutex_loc); - - if (allocator->n_allocated_slots == 0) - { - _dbus_assert (allocator->lock_loc == NULL); - allocator->lock_loc = mutex_loc; - } - else if (allocator->lock_loc != mutex_loc) - { - _dbus_warn_check_failed ("D-Bus threads were initialized after first using the D-Bus library. If your application does not directly initialize threads or use D-Bus, keep in mind that some library or plugin may have used D-Bus or initialized threads behind your back. You can often fix this problem by calling dbus_init_threads() or dbus_g_threads_init() early in your main() method, before D-Bus is used.\n"); - _dbus_assert_not_reached ("exiting"); - } + if (!_dbus_global_lock (allocator->mutex)) + return FALSE; if (*slot_id_p >= 0) { @@ -146,7 +135,7 @@ _dbus_data_slot_allocator_alloc (DBusDataSlotAllocator *allocator, slot, allocator, allocator->n_allocated_slots, allocator->n_used_slots); out: - _dbus_rmutex_unlock (*(allocator->lock_loc)); + _dbus_global_unlock (allocator->mutex); return slot >= 0; } @@ -165,8 +154,10 @@ void _dbus_data_slot_allocator_free (DBusDataSlotAllocator *allocator, dbus_int32_t *slot_id_p) { - _dbus_rmutex_lock (*(allocator->lock_loc)); - + /* Locking the mutex cannot fail unless no slots were allocated, which is + * also an error. */ + _dbus_global_lock_cannot_fail (allocator->mutex); + _dbus_assert (*slot_id_p < allocator->n_allocated_slots); _dbus_assert (allocator->allocated_slots[*slot_id_p].slot_id == *slot_id_p); _dbus_assert (allocator->allocated_slots[*slot_id_p].refcount > 0); @@ -175,7 +166,7 @@ _dbus_data_slot_allocator_free (DBusDataSlotAllocator *allocator, if (allocator->allocated_slots[*slot_id_p].refcount > 0) { - _dbus_rmutex_unlock (*(allocator->lock_loc)); + _dbus_global_unlock (allocator->mutex); return; } @@ -190,19 +181,12 @@ _dbus_data_slot_allocator_free (DBusDataSlotAllocator *allocator, if (allocator->n_used_slots == 0) { - DBusRMutex **mutex_loc = allocator->lock_loc; - dbus_free (allocator->allocated_slots); allocator->allocated_slots = NULL; allocator->n_allocated_slots = 0; - allocator->lock_loc = NULL; - - _dbus_rmutex_unlock (*mutex_loc); - } - else - { - _dbus_rmutex_unlock (*(allocator->lock_loc)); } + + _dbus_global_unlock (allocator->mutex); } /** @@ -246,11 +230,14 @@ _dbus_data_slot_list_set (DBusDataSlotAllocator *allocator, /* We need to take the allocator lock here, because the allocator could * be e.g. realloc()ing allocated_slots. We avoid doing this if asserts * are disabled, since then the asserts are empty. + * + * Locking the mutex cannot fail unless no slots were allocated, which is + * also an error. */ - _dbus_rmutex_lock (*(allocator->lock_loc)); + _dbus_global_lock_cannot_fail (allocator->mutex); _dbus_assert (slot < allocator->n_allocated_slots); _dbus_assert (allocator->allocated_slots[slot].slot_id == slot); - _dbus_rmutex_unlock (*(allocator->lock_loc)); + _dbus_global_unlock (allocator->mutex); #endif if (slot >= list->n_slots) @@ -303,12 +290,15 @@ _dbus_data_slot_list_get (DBusDataSlotAllocator *allocator, /* We need to take the allocator lock here, because the allocator could * be e.g. realloc()ing allocated_slots. We avoid doing this if asserts * are disabled, since then the asserts are empty. + * + * Locking the mutex cannot fail unless no slots were allocated, which is + * also an error. */ - _dbus_rmutex_lock (*(allocator->lock_loc)); + _dbus_global_lock_cannot_fail (allocator->mutex); _dbus_assert (slot >= 0); _dbus_assert (slot < allocator->n_allocated_slots); _dbus_assert (allocator->allocated_slots[slot].slot_id == slot); - _dbus_rmutex_unlock (*(allocator->lock_loc)); + _dbus_global_unlock (allocator->mutex); #endif if (slot >= list->n_slots) @@ -384,17 +374,12 @@ _dbus_data_slot_test (void) int i; DBusFreeFunction old_free_func; void *old_data; - DBusRMutex *mutex; - - if (!_dbus_data_slot_allocator_init (&allocator)) + + if (!_dbus_data_slot_allocator_init (&allocator, _DBUS_LOCK_SERVER_SLOTS)) _dbus_assert_not_reached ("no memory for allocator"); _dbus_data_slot_list_init (&list); - _dbus_rmutex_new_at_location (&mutex); - if (mutex == NULL) - _dbus_assert_not_reached ("failed to alloc mutex"); - #define N_SLOTS 100 i = 0; @@ -405,8 +390,9 @@ _dbus_data_slot_test (void) * here. */ dbus_int32_t tmp = -1; - - _dbus_data_slot_allocator_alloc (&allocator, &mutex, &tmp); + + if (!_dbus_data_slot_allocator_alloc (&allocator, &tmp)) + _dbus_assert_not_reached ("no memory to allocate slot"); if (tmp != i) _dbus_assert_not_reached ("did not allocate slots in numeric order\n"); @@ -471,8 +457,6 @@ _dbus_data_slot_test (void) ++i; } - _dbus_rmutex_free_at_location (&mutex); - return TRUE; } diff --git a/dbus/dbus-dataslot.h b/dbus/dbus-dataslot.h index 3d9d5ed..80132de 100644 --- a/dbus/dbus-dataslot.h +++ b/dbus/dbus-dataslot.h @@ -57,9 +57,11 @@ struct DBusDataSlotAllocator DBusAllocatedSlot *allocated_slots; /**< Allocated slots */ int n_allocated_slots; /**< number of slots malloc'd */ int n_used_slots; /**< number of slots used */ - DBusRMutex **lock_loc; /**< location of thread lock */ + DBusGlobalMutex mutex; /**< index of thread lock */ }; +#define DBUS_DATA_SLOT_ALLOCATOR_INIT(x) { NULL, 0, 0, x } + /** * Data structure that stores the actual user data set at a given * slot. @@ -70,9 +72,9 @@ struct DBusDataSlotList int n_slots; /**< Slots we have storage for in data_slots */ }; -dbus_bool_t _dbus_data_slot_allocator_init (DBusDataSlotAllocator *allocator); +dbus_bool_t _dbus_data_slot_allocator_init (DBusDataSlotAllocator *allocator, + DBusGlobalMutex mutex); dbus_bool_t _dbus_data_slot_allocator_alloc (DBusDataSlotAllocator *allocator, - DBusRMutex **mutex_loc, int *slot_id_p); void _dbus_data_slot_allocator_free (DBusDataSlotAllocator *allocator, int *slot_id_p); diff --git a/dbus/dbus-internals.c b/dbus/dbus-internals.c index 0e5d807..0e83ba5 100644 --- a/dbus/dbus-internals.c +++ b/dbus/dbus-internals.c @@ -157,41 +157,6 @@ */ /** - * @def _DBUS_LOCK_NAME - * - * Expands to name of a global lock variable. - */ - -/** - * @def _DBUS_DEFINE_GLOBAL_LOCK - * - * Defines a global lock variable with the given name. - * The lock must be added to the list to initialize - * in dbus_threads_init(). - */ - -/** - * @def _DBUS_DECLARE_GLOBAL_LOCK - * - * Expands to declaration of a global lock defined - * with _DBUS_DEFINE_GLOBAL_LOCK. - * The lock must be added to the list to initialize - * in dbus_threads_init(). - */ - -/** - * @def _DBUS_LOCK - * - * Locks a global lock - */ - -/** - * @def _DBUS_UNLOCK - * - * Unlocks a global lock - */ - -/** * Fixed "out of memory" error message, just to avoid * making up a different string every time and wasting * space. @@ -847,7 +812,7 @@ _dbus_read_uuid_file (const DBusString *filename, } } -_DBUS_DEFINE_GLOBAL_LOCK (machine_uuid); +/* protected by _DBUS_LOCK_MACHINE_UUID */ static int machine_uuid_initialized_generation = 0; static DBusGUID machine_uuid; @@ -865,8 +830,10 @@ dbus_bool_t _dbus_get_local_machine_uuid_encoded (DBusString *uuid_str) { dbus_bool_t ok; - - _DBUS_LOCK (machine_uuid); + + if (!_dbus_global_lock (_DBUS_LOCK_MACHINE_UUID)) + return FALSE; + if (machine_uuid_initialized_generation != _dbus_current_generation) { DBusError error = DBUS_ERROR_INIT; @@ -892,7 +859,7 @@ _dbus_get_local_machine_uuid_encoded (DBusString *uuid_str) ok = _dbus_uuid_encode (&machine_uuid, uuid_str); - _DBUS_UNLOCK (machine_uuid); + _dbus_global_unlock (_DBUS_LOCK_MACHINE_UUID); return ok; } diff --git a/dbus/dbus-internals.h b/dbus/dbus-internals.h index bd67eda..1947d31 100644 --- a/dbus/dbus-internals.h +++ b/dbus/dbus-internals.h @@ -296,34 +296,11 @@ typedef void (* DBusShutdownFunction) (void *data); dbus_bool_t _dbus_register_shutdown_func (DBusShutdownFunction function, void *data); -extern int _dbus_current_generation; +/* Only to be used by dbus-threads.c */ +dbus_bool_t _dbus_register_shutdown_func_unlocked (DBusShutdownFunction function, + void *data); -/* Thread initializers */ -#define _DBUS_LOCK_NAME(name) _dbus_lock_##name -#define _DBUS_DECLARE_GLOBAL_LOCK(name) extern DBusRMutex *_dbus_lock_##name -#define _DBUS_DEFINE_GLOBAL_LOCK(name) DBusRMutex *_dbus_lock_##name = NULL -#define _DBUS_LOCK(name) _dbus_rmutex_lock (_dbus_lock_##name) -#define _DBUS_UNLOCK(name) _dbus_rmutex_unlock (_dbus_lock_##name) - -/* 1-5 */ -_DBUS_DECLARE_GLOBAL_LOCK (list); -_DBUS_DECLARE_GLOBAL_LOCK (connection_slots); -_DBUS_DECLARE_GLOBAL_LOCK (pending_call_slots); -_DBUS_DECLARE_GLOBAL_LOCK (server_slots); -_DBUS_DECLARE_GLOBAL_LOCK (message_slots); -/* 5-10 */ -_DBUS_DECLARE_GLOBAL_LOCK (bus); -_DBUS_DECLARE_GLOBAL_LOCK (bus_datas); -_DBUS_DECLARE_GLOBAL_LOCK (shutdown_funcs); -_DBUS_DECLARE_GLOBAL_LOCK (system_users); -_DBUS_DECLARE_GLOBAL_LOCK (message_cache); -/* 10-14 */ -_DBUS_DECLARE_GLOBAL_LOCK (shared_connections); -_DBUS_DECLARE_GLOBAL_LOCK (win_fds); -_DBUS_DECLARE_GLOBAL_LOCK (sid_atom_cache); -_DBUS_DECLARE_GLOBAL_LOCK (machine_uuid); - -#define _DBUS_N_GLOBAL_LOCKS (14) +extern int _dbus_current_generation; dbus_bool_t _dbus_threads_init_debug (void); diff --git a/dbus/dbus-list.c b/dbus/dbus-list.c index 7e11cc8..d83ee6d 100644 --- a/dbus/dbus-list.c +++ b/dbus/dbus-list.c @@ -35,8 +35,8 @@ * Types and functions related to DBusList. */ +/* protected by _DBUS_LOCK_LIST */ static DBusMemPool *list_pool; -_DBUS_DEFINE_GLOBAL_LOCK (list); /** * @defgroup DBusListInternals Linked list implementation details @@ -56,7 +56,8 @@ alloc_link (void *data) { DBusList *link; - _DBUS_LOCK (list); + if (!_dbus_global_lock (_DBUS_LOCK_LIST)) + return NULL; if (list_pool == NULL) { @@ -64,7 +65,7 @@ alloc_link (void *data) if (list_pool == NULL) { - _DBUS_UNLOCK (list); + _dbus_global_unlock (_DBUS_LOCK_LIST); return NULL; } @@ -73,7 +74,7 @@ alloc_link (void *data) { _dbus_mem_pool_free (list_pool); list_pool = NULL; - _DBUS_UNLOCK (list); + _dbus_global_unlock (_DBUS_LOCK_LIST); return NULL; } } @@ -85,22 +86,24 @@ alloc_link (void *data) if (link) link->data = data; - _DBUS_UNLOCK (list); + _dbus_global_unlock (_DBUS_LOCK_LIST); return link; } static void free_link (DBusList *link) -{ - _DBUS_LOCK (list); +{ + /* If we allocated the link, we must have set up global locks first */ + _dbus_global_lock_cannot_fail (_DBUS_LOCK_LIST); + if (_dbus_mem_pool_dealloc (list_pool, link)) { _dbus_mem_pool_free (list_pool); list_pool = NULL; } - _DBUS_UNLOCK (list); + _dbus_global_unlock (_DBUS_LOCK_LIST); } static void @@ -152,9 +155,23 @@ _dbus_list_get_stats (dbus_uint32_t *in_use_p, dbus_uint32_t *in_free_list_p, dbus_uint32_t *allocated_p) { - _DBUS_LOCK (list); + /* If we allocated any links, we must have set up global locks first */ + if (!_dbus_global_lock (_DBUS_LOCK_LIST)) + { + if (in_use_p != NULL) + *in_use_p = 0; + + if (in_free_list_p != NULL) + *in_free_list_p = 0; + + if (allocated_p != NULL) + *allocated_p = 0; + + return; + } + _dbus_mem_pool_get_stats (list_pool, in_use_p, in_free_list_p, allocated_p); - _DBUS_UNLOCK (list); + _dbus_global_unlock (_DBUS_LOCK_LIST); } #endif diff --git a/dbus/dbus-memory.c b/dbus/dbus-memory.c index 317e37e..429f00e 100644 --- a/dbus/dbus-memory.c +++ b/dbus/dbus-memory.c @@ -795,7 +795,7 @@ struct ShutdownClosure void *data; /**< Data for function */ }; -_DBUS_DEFINE_GLOBAL_LOCK (shutdown_funcs); +/* Protected by _DBUS_LOCK_SHUTDOWN_FUNCS */ static ShutdownClosure *registered_globals = NULL; /** @@ -810,6 +810,21 @@ dbus_bool_t _dbus_register_shutdown_func (DBusShutdownFunction func, void *data) { + dbus_bool_t ret; + + if (!_dbus_global_lock (_DBUS_LOCK_SHUTDOWN_FUNCS)) + return FALSE; + + ret = _dbus_register_shutdown_func_unlocked (func, data); + + _dbus_global_unlock (_DBUS_LOCK_SHUTDOWN_FUNCS); + return ret; +} + +dbus_bool_t +_dbus_register_shutdown_func_unlocked (DBusShutdownFunction func, + void *data) +{ ShutdownClosure *c; c = dbus_new (ShutdownClosure, 1); @@ -820,13 +835,9 @@ _dbus_register_shutdown_func (DBusShutdownFunction func, c->func = func; c->data = data; - _DBUS_LOCK (shutdown_funcs); - c->next = registered_globals; registered_globals = c; - _DBUS_UNLOCK (shutdown_funcs); - return TRUE; } diff --git a/dbus/dbus-message.c b/dbus/dbus-message.c index 619bc69..1455c35 100644 --- a/dbus/dbus-message.c +++ b/dbus/dbus-message.c @@ -469,13 +469,13 @@ _dbus_message_set_signature (DBusMessage *message, * * with message cache implemented as array as it is now (0.000069 per): * new_empty_header 1.46 - * mutex_lock 0.56 # i.e. _DBUS_LOCK(message_cache) + * mutex_lock 0.56 # _DBUS_LOCK_MESSAGE_CACHE * mutex_unlock 0.25 * self 0.41 * unref 2.24 * self 0.68 * list_clear 0.43 - * mutex_lock 0.33 # i.e. _DBUS_LOCK(message_cache) + * mutex_lock 0.33 # _DBUS_LOCK_MESSAGE_CACHE * mutex_unlock 0.25 * * with message cache implemented as list (0.000070 per roundtrip): @@ -506,7 +506,7 @@ _dbus_message_set_signature (DBusMessage *message, /** Avoid caching too many messages */ #define MAX_MESSAGE_CACHE_SIZE 5 -_DBUS_DEFINE_GLOBAL_LOCK (message_cache); +/* Protected by _DBUS_LOCK_MESSAGE_CACHE */ static DBusMessage *message_cache[MAX_MESSAGE_CACHE_SIZE]; static int message_cache_count = 0; static dbus_bool_t message_cache_shutdown_registered = FALSE; @@ -516,7 +516,9 @@ dbus_message_cache_shutdown (void *data) { int i; - _DBUS_LOCK (message_cache); + /* if we're calling the shutdown function, we must have initialized global + * locks OK */ + _dbus_global_lock_cannot_fail (_DBUS_LOCK_MESSAGE_CACHE); i = 0; while (i < MAX_MESSAGE_CACHE_SIZE) @@ -530,7 +532,7 @@ dbus_message_cache_shutdown (void *data) message_cache_count = 0; message_cache_shutdown_registered = FALSE; - _DBUS_UNLOCK (message_cache); + _dbus_global_unlock (_DBUS_LOCK_MESSAGE_CACHE); } /** @@ -548,13 +550,14 @@ dbus_message_get_cached (void) message = NULL; - _DBUS_LOCK (message_cache); + if (!_dbus_global_lock (_DBUS_LOCK_MESSAGE_CACHE)) + return NULL; _dbus_assert (message_cache_count >= 0); if (message_cache_count == 0) { - _DBUS_UNLOCK (message_cache); + _dbus_global_unlock (_DBUS_LOCK_MESSAGE_CACHE); return NULL; } @@ -583,8 +586,8 @@ dbus_message_get_cached (void) _dbus_assert (_dbus_atomic_get (&message->refcount) == 0); _dbus_assert (message->counters == NULL); - - _DBUS_UNLOCK (message_cache); + + _dbus_global_unlock (_DBUS_LOCK_MESSAGE_CACHE); return message; } @@ -660,7 +663,8 @@ dbus_message_cache_or_finalize (DBusMessage *message) was_cached = FALSE; - _DBUS_LOCK (message_cache); + if (!_dbus_global_lock (_DBUS_LOCK_MESSAGE_CACHE)) + goto out_without_unlock; if (!message_cache_shutdown_registered) { @@ -710,8 +714,9 @@ dbus_message_cache_or_finalize (DBusMessage *message) out: _dbus_assert (_dbus_atomic_get (&message->refcount) == 0); - _DBUS_UNLOCK (message_cache); - + _dbus_global_unlock (_DBUS_LOCK_MESSAGE_CACHE); + + out_without_unlock: if (!was_cached) dbus_message_finalize (message); } @@ -4423,8 +4428,8 @@ _dbus_message_loader_get_max_message_unix_fds (DBusMessageLoader *loader) return loader->max_message_unix_fds; } -static DBusDataSlotAllocator slot_allocator; -_DBUS_DEFINE_GLOBAL_LOCK (message_slots); +static DBusDataSlotAllocator slot_allocator = + DBUS_DATA_SLOT_ALLOCATOR_INIT (_DBUS_LOCK_MESSAGE_SLOTS); /** * Allocates an integer ID to be used for storing application-specific @@ -4444,7 +4449,6 @@ dbus_bool_t dbus_message_allocate_data_slot (dbus_int32_t *slot_p) { return _dbus_data_slot_allocator_alloc (&slot_allocator, - &_DBUS_LOCK_NAME (message_slots), slot_p); } diff --git a/dbus/dbus-pending-call.c b/dbus/dbus-pending-call.c index 62c6c74..dbd0f19 100644 --- a/dbus/dbus-pending-call.c +++ b/dbus/dbus-pending-call.c @@ -489,8 +489,8 @@ _dbus_pending_call_get_completed_unlocked (DBusPendingCall *pending) return pending->completed; } -static DBusDataSlotAllocator slot_allocator; -_DBUS_DEFINE_GLOBAL_LOCK (pending_call_slots); +static DBusDataSlotAllocator slot_allocator = + DBUS_DATA_SLOT_ALLOCATOR_INIT (_DBUS_LOCK_PENDING_CALL_SLOTS); /** * Stores a pointer on a #DBusPendingCall, along @@ -768,7 +768,6 @@ dbus_pending_call_allocate_data_slot (dbus_int32_t *slot_p) _dbus_return_val_if_fail (slot_p != NULL, FALSE); return _dbus_data_slot_allocator_alloc (&slot_allocator, - &_DBUS_LOCK_NAME (pending_call_slots), slot_p); } diff --git a/dbus/dbus-server.c b/dbus/dbus-server.c index b62c2b4..ce52f15 100644 --- a/dbus/dbus-server.c +++ b/dbus/dbus-server.c @@ -1071,9 +1071,8 @@ dbus_server_set_auth_mechanisms (DBusServer *server, return TRUE; } - -static DBusDataSlotAllocator slot_allocator; -_DBUS_DEFINE_GLOBAL_LOCK (server_slots); +static DBusDataSlotAllocator slot_allocator = + DBUS_DATA_SLOT_ALLOCATOR_INIT (_DBUS_LOCK_SERVER_SLOTS); /** * Allocates an integer ID to be used for storing application-specific @@ -1093,7 +1092,6 @@ dbus_bool_t dbus_server_allocate_data_slot (dbus_int32_t *slot_p) { return _dbus_data_slot_allocator_alloc (&slot_allocator, - (DBusRMutex **)&_DBUS_LOCK_NAME (server_slots), slot_p); } diff --git a/dbus/dbus-sysdeps-pthread.c b/dbus/dbus-sysdeps-pthread.c index 1300ec3..47e7c0f 100644 --- a/dbus/dbus-sysdeps-pthread.c +++ b/dbus/dbus-sysdeps-pthread.c @@ -103,6 +103,8 @@ _dbus_platform_cmutex_new (void) return pmutex; } +/* Note that _dbus_global_lock() relies on this not needing any of the + * initialization done by _dbus_threads_init_platform_specific(). */ DBusRMutex * _dbus_platform_rmutex_new (void) { @@ -139,6 +141,8 @@ _dbus_platform_cmutex_free (DBusCMutex *mutex) dbus_free (mutex); } +/* Note that _dbus_global_lock() relies on this not needing any of the + * initialization done by _dbus_threads_init_platform_specific(). */ void _dbus_platform_rmutex_free (DBusRMutex *mutex) { @@ -152,6 +156,8 @@ _dbus_platform_cmutex_lock (DBusCMutex *mutex) PTHREAD_CHECK ("pthread_mutex_lock", pthread_mutex_lock (&mutex->lock)); } +/* Note that _dbus_global_lock() relies on this not needing any of the + * initialization done by _dbus_threads_init_platform_specific(). */ void _dbus_platform_rmutex_lock (DBusRMutex *mutex) { @@ -164,6 +170,8 @@ _dbus_platform_cmutex_unlock (DBusCMutex *mutex) PTHREAD_CHECK ("pthread_mutex_unlock", pthread_mutex_unlock (&mutex->lock)); } +/* Note that _dbus_global_lock() relies on this not needing any of the + * initialization done by _dbus_threads_init_platform_specific(). */ void _dbus_platform_rmutex_unlock (DBusRMutex *mutex) { diff --git a/dbus/dbus-sysdeps-thread-win.c b/dbus/dbus-sysdeps-thread-win.c index 5cf0cf8..29e7f75 100644 --- a/dbus/dbus-sysdeps-thread-win.c +++ b/dbus/dbus-sysdeps-thread-win.c @@ -98,6 +98,8 @@ _dbus_platform_cmutex_new (void) return (DBusCMutex *) handle; } +/* Note that _dbus_global_lock() relies on this not needing any of the + * initialization done by _dbus_threads_init_platform_specific(). */ DBusRMutex * _dbus_platform_rmutex_new (void) { @@ -112,6 +114,8 @@ _dbus_platform_cmutex_free (DBusCMutex *mutex) CloseHandle ((HANDLE *) mutex); } +/* Note that _dbus_global_lock() relies on this not needing any of the + * initialization done by _dbus_threads_init_platform_specific(). */ void _dbus_platform_rmutex_free (DBusRMutex *mutex) { @@ -124,6 +128,8 @@ _dbus_platform_cmutex_lock (DBusCMutex *mutex) WaitForSingleObject ((HANDLE *) mutex, INFINITE); } +/* Note that _dbus_global_lock() relies on this not needing any of the + * initialization done by _dbus_threads_init_platform_specific(). */ void _dbus_platform_rmutex_lock (DBusRMutex *mutex) { @@ -136,6 +142,8 @@ _dbus_platform_cmutex_unlock (DBusCMutex *mutex) ReleaseMutex ((HANDLE *) mutex); } +/* Note that _dbus_global_lock() relies on this not needing any of the + * initialization done by _dbus_threads_init_platform_specific(). */ void _dbus_platform_rmutex_unlock (DBusRMutex *mutex) { diff --git a/dbus/dbus-sysdeps-win.c b/dbus/dbus-sysdeps-win.c index 4a54981..2f55e59 100644 --- a/dbus/dbus-sysdeps-win.c +++ b/dbus/dbus-sysdeps-win.c @@ -2611,7 +2611,7 @@ dbus_bool_t _dbus_read_local_machine_uuid (DBusGUID *machine_id, } static -HANDLE _dbus_global_lock (const char *mutexname) +HANDLE _dbus_systemwide_lock (const char *mutexname) { HANDLE mutex; DWORD gotMutex; @@ -2638,7 +2638,7 @@ HANDLE _dbus_global_lock (const char *mutexname) } static -void _dbus_global_unlock (HANDLE mutex) +void _dbus_systemwide_unlock (HANDLE mutex) { ReleaseMutex (mutex); CloseHandle (mutex); @@ -2749,7 +2749,7 @@ _dbus_daemon_is_session_bus_address_published (const char *scope) return TRUE; // sync _dbus_daemon_publish_session_bus_address, _dbus_daemon_unpublish_session_bus_address and _dbus_daemon_already_runs - lock = _dbus_global_lock( cUniqueDBusInitMutex ); + lock = _dbus_systemwide_lock( cUniqueDBusInitMutex ); // we use CreateMutex instead of OpenMutex because of possible race conditions, // see http://msdn.microsoft.com/en-us/library/ms684315%28VS.85%29.aspx @@ -2759,7 +2759,7 @@ _dbus_daemon_is_session_bus_address_published (const char *scope) Fortunally the client deletes the mutex in the lock protected area, so checking presence will work too. */ - _dbus_global_unlock( lock ); + _dbus_systemwide_unlock( lock ); _dbus_string_free( &mutex_name ); @@ -2794,7 +2794,7 @@ _dbus_daemon_publish_session_bus_address (const char* address, const char *scope } // sync _dbus_daemon_publish_session_bus_address, _dbus_daemon_unpublish_session_bus_address and _dbus_daemon_already_runs - lock = _dbus_global_lock( cUniqueDBusInitMutex ); + lock = _dbus_systemwide_lock( cUniqueDBusInitMutex ); if (!hDBusDaemonMutex) { @@ -2805,7 +2805,7 @@ _dbus_daemon_publish_session_bus_address (const char* address, const char *scope // acquire the mutex if (WaitForSingleObject( hDBusDaemonMutex, 10 ) != WAIT_OBJECT_0) { - _dbus_global_unlock( lock ); + _dbus_systemwide_unlock( lock ); CloseHandle( hDBusDaemonMutex ); return FALSE; } @@ -2813,7 +2813,7 @@ _dbus_daemon_publish_session_bus_address (const char* address, const char *scope if (!_dbus_get_shm_name(&shm_name,scope)) { _dbus_string_free( &shm_name ); - _dbus_global_unlock( lock ); + _dbus_systemwide_unlock( lock ); return FALSE; } @@ -2831,7 +2831,7 @@ _dbus_daemon_publish_session_bus_address (const char* address, const char *scope // cleanup UnmapViewOfFile( shared_addr ); - _dbus_global_unlock( lock ); + _dbus_systemwide_unlock( lock ); _dbus_verbose( "published session bus address at %s\n",_dbus_string_get_const_data (&shm_name) ); _dbus_string_free( &shm_name ); @@ -2844,7 +2844,7 @@ _dbus_daemon_unpublish_session_bus_address (void) HANDLE lock; // sync _dbus_daemon_publish_session_bus_address, _dbus_daemon_unpublish_session_bus_address and _dbus_daemon_already_runs - lock = _dbus_global_lock( cUniqueDBusInitMutex ); + lock = _dbus_systemwide_lock( cUniqueDBusInitMutex ); CloseHandle( hDBusSharedMem ); @@ -2856,7 +2856,7 @@ _dbus_daemon_unpublish_session_bus_address (void) hDBusDaemonMutex = NULL; - _dbus_global_unlock( lock ); + _dbus_systemwide_unlock( lock ); } static dbus_bool_t @@ -2911,7 +2911,7 @@ _dbus_daemon_already_runs (DBusString *address, DBusString *shm_name, const char } // sync _dbus_daemon_publish_session_bus_address, _dbus_daemon_unpublish_session_bus_address and _dbus_daemon_already_runs - lock = _dbus_global_lock( cUniqueDBusInitMutex ); + lock = _dbus_systemwide_lock( cUniqueDBusInitMutex ); // do checks daemon = CreateMutexA( NULL, FALSE, _dbus_string_get_const_data(&mutex_name) ); @@ -2920,7 +2920,7 @@ _dbus_daemon_already_runs (DBusString *address, DBusString *shm_name, const char ReleaseMutex (daemon); CloseHandle (daemon); - _dbus_global_unlock( lock ); + _dbus_systemwide_unlock( lock ); _dbus_string_free( &mutex_name ); return FALSE; } @@ -2931,7 +2931,7 @@ _dbus_daemon_already_runs (DBusString *address, DBusString *shm_name, const char // cleanup CloseHandle ( daemon ); - _dbus_global_unlock( lock ); + _dbus_systemwide_unlock( lock ); _dbus_string_free( &mutex_name ); return bRet; @@ -2959,7 +2959,7 @@ _dbus_get_autolaunch_address (const char *scope, DBusString *address, return FALSE; } - mutex = _dbus_global_lock ( cDBusAutolaunchMutex ); + mutex = _dbus_systemwide_lock ( cDBusAutolaunchMutex ); if (_dbus_daemon_already_runs(address,&shm_name,scope)) { @@ -3034,7 +3034,7 @@ out: else _DBUS_ASSERT_ERROR_IS_SET (error); - _dbus_global_unlock (mutex); + _dbus_systemwide_unlock (mutex); return retval; } diff --git a/dbus/dbus-sysdeps.c b/dbus/dbus-sysdeps.c index 04fb8d7..0fbf9e7 100644 --- a/dbus/dbus-sysdeps.c +++ b/dbus/dbus-sysdeps.c @@ -46,10 +46,6 @@ #include #endif -_DBUS_DEFINE_GLOBAL_LOCK (win_fds); -_DBUS_DEFINE_GLOBAL_LOCK (sid_atom_cache); -_DBUS_DEFINE_GLOBAL_LOCK (system_users); - #ifdef DBUS_WIN #include #elif (defined __APPLE__) diff --git a/dbus/dbus-threads-internal.h b/dbus/dbus-threads-internal.h index 64e8bac..813dd9e 100644 --- a/dbus/dbus-threads-internal.h +++ b/dbus/dbus-threads-internal.h @@ -33,6 +33,32 @@ */ /** + * A mutex protecting global state within libdbus. They are addressed + * by index, not by pointer. + */ +typedef enum { + /* 0-4 */ + _DBUS_LOCK_LIST, + _DBUS_LOCK_CONNECTION_SLOTS, + _DBUS_LOCK_PENDING_CALL_SLOTS, + _DBUS_LOCK_SERVER_SLOTS, + _DBUS_LOCK_MESSAGE_SLOTS, + /* 5-9 */ + _DBUS_LOCK_BUS, + _DBUS_LOCK_BUS_DATAS, + _DBUS_LOCK_SHUTDOWN_FUNCS, + _DBUS_LOCK_SYSTEM_USERS, + _DBUS_LOCK_MESSAGE_CACHE, + /* 10-11 */ + _DBUS_LOCK_SHARED_CONNECTIONS, + _DBUS_LOCK_MACHINE_UUID, + + _DBUS_N_GLOBAL_LOCKS, + + _DBUS_LOCK_INVALID = 10000 +} DBusGlobalMutex; + +/** * A mutex which is recursive if possible, else non-recursive. * This is typically recursive, but that cannot be relied upon. */ @@ -48,6 +74,10 @@ typedef struct DBusCMutex DBusCMutex; DBUS_BEGIN_DECLS +dbus_bool_t _dbus_global_lock (DBusGlobalMutex mutex) _DBUS_GNUC_WARN_UNUSED_RESULT; +void _dbus_global_lock_cannot_fail (DBusGlobalMutex mutex); +void _dbus_global_unlock (DBusGlobalMutex mutex); + void _dbus_rmutex_lock (DBusRMutex *mutex); void _dbus_rmutex_unlock (DBusRMutex *mutex); void _dbus_rmutex_new_at_location (DBusRMutex **location_p); diff --git a/dbus/dbus-threads.c b/dbus/dbus-threads.c index ddd83cb..aa85680 100644 --- a/dbus/dbus-threads.c +++ b/dbus/dbus-threads.c @@ -344,25 +344,6 @@ _dbus_condvar_wake_one (DBusCondVar *cond) } static void -shutdown_global_locks (void *data) -{ - DBusRMutex ***locks = data; - int i; - - i = 0; - while (i < _DBUS_N_GLOBAL_LOCKS) - { - if (*(locks[i]) != NULL) - _dbus_platform_rmutex_free (*(locks[i])); - - *(locks[i]) = NULL; - ++i; - } - - dbus_free (locks); -} - -static void shutdown_uninitialized_locks (void *data) { _dbus_list_clear (&uninitialized_rmutex_list); @@ -482,71 +463,114 @@ init_uninitialized_locks (void) return FALSE; } +/* All NULL-initialized (ISO C guarantees that partial initializers + * work like that). */ +static DBusRMutex *global_locks[_DBUS_N_GLOBAL_LOCKS] = { NULL }; + +static void +shutdown_global_locks (void *data) +{ + int i; + + for (i = 0; i < _DBUS_N_GLOBAL_LOCKS; i++) + { + _dbus_assert (global_locks[i] != NULL); + _dbus_platform_rmutex_free (global_locks[i]); + global_locks[i] = NULL; + } +} + static dbus_bool_t -init_locks (void) +init_global_locks (void) { + dbus_bool_t ok; int i; - DBusRMutex ***dynamic_global_locks; - DBusRMutex **global_locks[] = { -#define LOCK_ADDR(name) (& _dbus_lock_##name) - LOCK_ADDR (win_fds), - LOCK_ADDR (sid_atom_cache), - LOCK_ADDR (list), - LOCK_ADDR (connection_slots), - LOCK_ADDR (pending_call_slots), - LOCK_ADDR (server_slots), - LOCK_ADDR (message_slots), - LOCK_ADDR (bus), - LOCK_ADDR (bus_datas), - LOCK_ADDR (shutdown_funcs), - LOCK_ADDR (system_users), - LOCK_ADDR (message_cache), - LOCK_ADDR (shared_connections), - LOCK_ADDR (machine_uuid) -#undef LOCK_ADDR - }; - - _dbus_assert (_DBUS_N_ELEMENTS (global_locks) == - _DBUS_N_GLOBAL_LOCKS); - - i = 0; - - dynamic_global_locks = dbus_new (DBusRMutex**, _DBUS_N_GLOBAL_LOCKS); - if (dynamic_global_locks == NULL) - goto failed; - - while (i < _DBUS_N_ELEMENTS (global_locks)) + + for (i = 0; i < _DBUS_N_ELEMENTS (global_locks); i++) { - *global_locks[i] = _dbus_platform_rmutex_new (); + global_locks[i] = _dbus_platform_rmutex_new (); - if (*global_locks[i] == NULL) + if (global_locks[i] == NULL) goto failed; + } - dynamic_global_locks[i] = global_locks[i]; + _dbus_platform_rmutex_lock (global_locks[_DBUS_LOCK_SHUTDOWN_FUNCS]); + ok = _dbus_register_shutdown_func_unlocked (shutdown_global_locks, NULL); + _dbus_platform_rmutex_unlock (global_locks[_DBUS_LOCK_SHUTDOWN_FUNCS]); - ++i; - } - - if (!_dbus_register_shutdown_func (shutdown_global_locks, - dynamic_global_locks)) + if (!ok) goto failed; - if (!init_uninitialized_locks ()) - goto failed; - return TRUE; failed: - dbus_free (dynamic_global_locks); - for (i = i - 1; i >= 0; i--) { - _dbus_platform_rmutex_free (*global_locks[i]); - *global_locks[i] = NULL; + _dbus_platform_rmutex_free (global_locks[i]); + global_locks[i] = NULL; } return FALSE; } +/** + * Lock the given global mutex, possibly using lazy allocation if necessary. + * + * @param mutex the numeric index of the mutex + * @returns #FALSE if not enough memory + */ +dbus_bool_t +_dbus_global_lock (DBusGlobalMutex mutex) +{ + _dbus_assert (mutex >= 0); + _dbus_assert (mutex < _DBUS_N_GLOBAL_LOCKS); + + /* If we have already called dbus_threads_init_default(), this does + * nothing; fine. + * + * If we have not already called it, then either we're currently + * single-threaded, in which case it's safe to do initialization here; + * or we're already multi-threaded, in which case the application is + * already broken and we're unlikely to make it any worse. + */ + if (global_locks[0] == NULL && !init_global_locks ()) + return FALSE; + + _dbus_assert (global_locks[mutex] != NULL); + _dbus_platform_rmutex_lock (global_locks[mutex]); + return TRUE; +} + +/** + * Lock the given global mutex. Every use of this function must be + * accompanied by a comment explaining why the memory allocation cannot fail. + * + * @param mutex the numeric index of the mutex + */ +void +_dbus_global_lock_cannot_fail (DBusGlobalMutex mutex) +{ + _dbus_assert (global_locks[0] != NULL); + + if (!_dbus_global_lock (mutex)) + _dbus_assert_not_reached ("_dbus_global_lock() failed despite prior " + "initialization"); +} + +/** + * Unlock the given global mutex. + * + * @param mutex the numeric index of the mutex + */ +void +_dbus_global_unlock (DBusGlobalMutex mutex) +{ + _dbus_assert (mutex >= 0); + _dbus_assert (mutex < _DBUS_N_GLOBAL_LOCKS); + + _dbus_assert (global_locks[mutex] != NULL); + _dbus_platform_rmutex_unlock (global_locks[mutex]); +} + /** @} */ /* end of internals */ /** @@ -590,7 +614,8 @@ dbus_threads_init (const DBusThreadFunctions *functions) } if (!_dbus_threads_init_platform_specific() || - !init_locks ()) + !init_global_locks () || + !init_uninitialized_locks ()) { _dbus_threads_unlock_platform_specific (); return FALSE; diff --git a/dbus/dbus-userdb-util.c b/dbus/dbus-userdb-util.c index 5af0092..cacc19c 100644 --- a/dbus/dbus-userdb-util.c +++ b/dbus/dbus-userdb-util.c @@ -104,7 +104,11 @@ _dbus_is_console_user (dbus_uid_t uid, #endif /* HAVE_CONSOLE_OWNER_FILE */ - _dbus_user_database_lock_system (); + if (!_dbus_user_database_lock_system ()) + { + _DBUS_SET_OOM (error); + return FALSE; + } db = _dbus_user_database_get_system (); if (db == NULL) @@ -158,7 +162,11 @@ _dbus_get_group_id (const DBusString *groupname, { DBusUserDatabase *db; const DBusGroupInfo *info; - _dbus_user_database_lock_system (); + + /* This can't distinguish OOM from other errors, but that's not a + * regression... FIXME: add a DBusError or something */ + if (!_dbus_user_database_lock_system ()) + return FALSE; db = _dbus_user_database_get_system (); if (db == NULL) @@ -195,7 +203,11 @@ _dbus_get_user_id_and_primary_group (const DBusString *username, { DBusUserDatabase *db; const DBusUserInfo *info; - _dbus_user_database_lock_system (); + + /* This can't distinguish OOM from other errors, but that's not a + * regression... FIXME: add a DBusError or something */ + if (!_dbus_user_database_lock_system ()) + return FALSE; db = _dbus_user_database_get_system (); if (db == NULL) @@ -388,7 +400,10 @@ _dbus_groups_from_uid (dbus_uid_t uid, *group_ids = NULL; *n_group_ids = 0; - _dbus_user_database_lock_system (); + /* This can't distinguish OOM from other errors, but that's not a + * regression... FIXME: add a DBusError or something */ + if (!_dbus_user_database_lock_system ()) + return FALSE; db = _dbus_user_database_get_system (); if (db == NULL) diff --git a/dbus/dbus-userdb.c b/dbus/dbus-userdb.c index 4e8b39a..7bac777 100644 --- a/dbus/dbus-userdb.c +++ b/dbus/dbus-userdb.c @@ -305,12 +305,17 @@ init_system_db (void) /** * Locks global system user database. + * + * @return #FALSE if not enough memory */ -void +dbus_bool_t _dbus_user_database_lock_system (void) { - _DBUS_LOCK (system_users); + if (!_dbus_global_lock (_DBUS_LOCK_SYSTEM_USERS)) + return FALSE; + database_locked = TRUE; + return TRUE; } /** @@ -320,7 +325,7 @@ void _dbus_user_database_unlock_system (void) { database_locked = FALSE; - _DBUS_UNLOCK (system_users); + _dbus_global_unlock (_DBUS_LOCK_SYSTEM_USERS); } /** @@ -345,8 +350,13 @@ _dbus_user_database_get_system (void) void _dbus_user_database_flush_system (void) { - _dbus_user_database_lock_system (); - + if (!_dbus_user_database_lock_system ()) + { + /* not much we can do about that - behave as if init_system_db() + * had failed, because presumably it did */ + return; + } + if (system_db != NULL) _dbus_user_database_flush (system_db); @@ -363,7 +373,9 @@ _dbus_user_database_flush_system (void) dbus_bool_t _dbus_username_from_current_process (const DBusString **username) { - _dbus_user_database_lock_system (); + if (!_dbus_user_database_lock_system ()) + return FALSE; + if (!init_system_db ()) { _dbus_user_database_unlock_system (); @@ -385,7 +397,9 @@ _dbus_username_from_current_process (const DBusString **username) dbus_bool_t _dbus_homedir_from_current_process (const DBusString **homedir) { - _dbus_user_database_lock_system (); + if (!_dbus_user_database_lock_system ()) + return FALSE; + if (!init_system_db ()) { _dbus_user_database_unlock_system (); @@ -410,7 +424,11 @@ _dbus_homedir_from_username (const DBusString *username, { DBusUserDatabase *db; const DBusUserInfo *info; - _dbus_user_database_lock_system (); + + /* This can't distinguish OOM from other errors, but that's not a + * regression... FIXME: add a DBusError or something */ + if (!_dbus_user_database_lock_system ()) + return FALSE; db = _dbus_user_database_get_system (); if (db == NULL) @@ -449,7 +467,11 @@ _dbus_homedir_from_uid (dbus_uid_t uid, { DBusUserDatabase *db; const DBusUserInfo *info; - _dbus_user_database_lock_system (); + + /* This can't distinguish OOM from other errors, but that's not a + * regression... FIXME: add a DBusError or something */ + if (!_dbus_user_database_lock_system ()) + return FALSE; db = _dbus_user_database_get_system (); if (db == NULL) @@ -496,7 +518,10 @@ _dbus_credentials_add_from_user (DBusCredentials *credentials, DBusUserDatabase *db; const DBusUserInfo *info; - _dbus_user_database_lock_system (); + /* This can't distinguish OOM from other errors, but that's not a + * regression... FIXME: add a DBusError or something */ + if (!_dbus_user_database_lock_system ()) + return FALSE; db = _dbus_user_database_get_system (); if (db == NULL) diff --git a/dbus/dbus-userdb.h b/dbus/dbus-userdb.h index cb49d9e..d6b72d8 100644 --- a/dbus/dbus-userdb.h +++ b/dbus/dbus-userdb.h @@ -86,7 +86,7 @@ void _dbus_group_info_free_allocated (DBusGroupInfo *info); #endif /* DBUS_USERDB_INCLUDES_PRIVATE */ DBusUserDatabase* _dbus_user_database_get_system (void); -void _dbus_user_database_lock_system (void); +dbus_bool_t _dbus_user_database_lock_system (void) _DBUS_GNUC_WARN_UNUSED_RESULT; void _dbus_user_database_unlock_system (void); void _dbus_user_database_flush_system (void); -- 1.7.10.4