From b35d0d30f520fd24109b73f7cb977666c12c8b3b Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Tue, 3 Sep 2013 13:28:49 +0100 Subject: [PATCH] _dbus_modify_sigpipe: be thread-safe This needs new atomic primitives: we don't have "set to a value", and in fact that's a bit annoying to implement in terms of gcc intrinsics. "Set to 0" and "set to nonzero" are easy, though. Also add a trivial sanity-check for the atomic primitives (it doesn't verify that they're atomic, but does verify that they return the right things). Bug: https://bugs.freedesktop.org/show_bug.cgi?id=68610 --- dbus/dbus-connection.c | 13 +++++---- dbus/dbus-sysdeps-unix.c | 36 +++++++++++++++++++++++++ dbus/dbus-sysdeps-win.c | 22 ++++++++++++++++ dbus/dbus-sysdeps.h | 2 ++ test/Makefile.am | 5 ++++ test/internals/atomic.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 141 insertions(+), 5 deletions(-) create mode 100644 test/internals/atomic.c diff --git a/dbus/dbus-connection.c b/dbus/dbus-connection.c index 759a649..f3718dd 100644 --- a/dbus/dbus-connection.c +++ b/dbus/dbus-connection.c @@ -245,9 +245,9 @@ struct DBusPreallocatedSend }; #if HAVE_DECL_MSG_NOSIGNAL -static dbus_bool_t _dbus_modify_sigpipe = FALSE; +static DBusAtomic _dbus_modify_sigpipe = { FALSE }; #else -static dbus_bool_t _dbus_modify_sigpipe = TRUE; +static DBusAtomic _dbus_modify_sigpipe = { TRUE }; #endif /** @@ -1328,7 +1328,7 @@ _dbus_connection_new_for_transport (DBusTransport *transport) if (objects == NULL) goto error; - if (_dbus_modify_sigpipe) + if (_dbus_atomic_get (&_dbus_modify_sigpipe) != 0) _dbus_disable_sigpipe (); /* initialized to 0: use atomic op to avoid mixing atomic and non-atomic */ @@ -6014,8 +6014,11 @@ dbus_connection_get_data (DBusConnection *connection, */ void dbus_connection_set_change_sigpipe (dbus_bool_t will_modify_sigpipe) -{ - _dbus_modify_sigpipe = will_modify_sigpipe != FALSE; +{ + if (will_modify_sigpipe) + _dbus_atomic_set_nonzero (&_dbus_modify_sigpipe); + else + _dbus_atomic_set_zero (&_dbus_modify_sigpipe); } /** diff --git a/dbus/dbus-sysdeps-unix.c b/dbus/dbus-sysdeps-unix.c index 1cb4a58..7ac3b93 100644 --- a/dbus/dbus-sysdeps-unix.c +++ b/dbus/dbus-sysdeps-unix.c @@ -2511,6 +2511,42 @@ _dbus_atomic_get (DBusAtomic *atomic) } /** + * Atomically set the value of an integer to 0. + * + * @param atomic pointer to the integer to set + */ +void +_dbus_atomic_set_zero (DBusAtomic *atomic) +{ +#if DBUS_USE_SYNC + /* Atomic version of "*atomic &= 0; return *atomic" */ + __sync_and_and_fetch (&atomic->value, 0); +#else + pthread_mutex_lock (&atomic_mutex); + atomic->value = 0; + pthread_mutex_unlock (&atomic_mutex); +#endif +} + +/** + * Atomically set the value of an integer to something nonzero. + * + * @param atomic pointer to the integer to set + */ +void +_dbus_atomic_set_nonzero (DBusAtomic *atomic) +{ +#if DBUS_USE_SYNC + /* Atomic version of "*atomic |= 1; return *atomic" */ + __sync_or_and_fetch (&atomic->value, 1); +#else + pthread_mutex_lock (&atomic_mutex); + atomic->value = 1; + pthread_mutex_unlock (&atomic_mutex); +#endif +} + +/** * Wrapper for poll(). * * @param fds the file descriptors to poll diff --git a/dbus/dbus-sysdeps-win.c b/dbus/dbus-sysdeps-win.c index 9c4a06b..22cefc3 100644 --- a/dbus/dbus-sysdeps-win.c +++ b/dbus/dbus-sysdeps-win.c @@ -3247,6 +3247,28 @@ _dbus_atomic_get (DBusAtomic *atomic) } /** + * Atomically set the value of an integer to 0. + * + * @param atomic pointer to the integer to set + */ +void +_dbus_atomic_set_zero (DBusAtomic *atomic) +{ + InterlockedExchange (&atomic->value, 0); +} + +/** + * Atomically set the value of an integer to something nonzero. + * + * @param atomic pointer to the integer to set + */ +void +_dbus_atomic_set_nonzero (DBusAtomic *atomic) +{ + InterlockedExchange (&atomic->value, 1); +} + +/** * Called when the bus daemon is signaled to reload its configuration; any * caches should be nuked. Of course any caches that need explicit reload * are probably broken, but c'est la vie. diff --git a/dbus/dbus-sysdeps.h b/dbus/dbus-sysdeps.h index e586946..8b48d92 100644 --- a/dbus/dbus-sysdeps.h +++ b/dbus/dbus-sysdeps.h @@ -246,6 +246,8 @@ struct DBusAtomic dbus_int32_t _dbus_atomic_inc (DBusAtomic *atomic); dbus_int32_t _dbus_atomic_dec (DBusAtomic *atomic); dbus_int32_t _dbus_atomic_get (DBusAtomic *atomic); +void _dbus_atomic_set_zero (DBusAtomic *atomic); +void _dbus_atomic_set_nonzero (DBusAtomic *atomic); /* AIX uses different values for poll */ diff --git a/test/Makefile.am b/test/Makefile.am index 405aa1c..bff342d 100644 --- a/test/Makefile.am +++ b/test/Makefile.am @@ -105,6 +105,10 @@ shell_test_LDADD = libdbus-testutils-internal.la spawn_test_CPPFLAGS = $(static_cppflags) spawn_test_LDADD = $(top_builddir)/dbus/libdbus-internal.la +test_atomic_SOURCES = internals/atomic.c +test_atomic_CPPFLAGS = $(static_cppflags) +test_atomic_LDADD = $(top_builddir)/dbus/libdbus-internal.la + test_printf_SOURCES = internals/printf.c test_printf_CPPFLAGS = $(static_cppflags) test_printf_LDADD = $(top_builddir)/dbus/libdbus-internal.la @@ -125,6 +129,7 @@ testexec_PROGRAMS = installable_tests = \ shell-test \ + test-atomic \ test-printf \ $(NULL) installable_manual_tests = \ diff --git a/test/internals/atomic.c b/test/internals/atomic.c new file mode 100644 index 0000000..9e3276e --- /dev/null +++ b/test/internals/atomic.c @@ -0,0 +1,68 @@ +/* Regression test for atomic ops + * + * Author: Simon McVittie + * Copyright © 2013 Collabora Ltd. + * + * Permission is hereby granted, free of charge, to any person + * obtaining a copy of this software and associated documentation files + * (the "Software"), to deal in the Software without restriction, + * including without limitation the rights to use, copy, modify, merge, + * publish, distribute, sublicense, and/or sell copies of the Software, + * and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be + * included in all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS + * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN + * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + */ + +#include + +#include +#include +#include + +int +main (int argc, + char **argv) +{ + DBusAtomic a = { 0 }; + DBusAtomic b = { 123 }; + dbus_int32_t old; + + _dbus_assert (_dbus_atomic_get (&a) == 0); + _dbus_assert (_dbus_atomic_get (&b) == 123); + + _dbus_assert (a.value == 0); + old = _dbus_atomic_dec (&a); + _dbus_assert (old == 0); + _dbus_assert (a.value == -1); + + _dbus_assert (b.value == 123); + old = _dbus_atomic_inc (&b); + _dbus_assert (old == 123); + _dbus_assert (b.value == 124); + + _dbus_atomic_set_nonzero (&a); + /* careful: this is not necessarily 1 */ + _dbus_assert (a.value != 0); + + _dbus_atomic_set_nonzero (&b); + _dbus_assert (b.value != 0); + + _dbus_atomic_set_zero (&a); + _dbus_assert (a.value == 0); + + _dbus_atomic_set_zero (&b); + _dbus_assert (b.value == 0); + + return 0; +} -- 1.8.4.rc3