[asterisk-commits] tilghman: branch 1.4 r273793 - in /branches/1.4: ./ channels/ include/asterisk/

SVN commits to the Asterisk project asterisk-commits at lists.digium.com
Fri Jul 2 16:36:48 CDT 2010


Author: tilghman
Date: Fri Jul  2 16:36:39 2010
New Revision: 273793

URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=273793
Log:
Have the DEADLOCK_AVOIDANCE macro warn when an unlock fails, to help catch potentially large software bugs.

(closes issue #17407)
 Reported by: pdf
 Patches: 
       20100527__issue17407.diff.txt uploaded by tilghman (license 14)
 
Review: https://reviewboard.asterisk.org/r/751/

Modified:
    branches/1.4/channels/chan_agent.c
    branches/1.4/channels/chan_dahdi.c
    branches/1.4/channels/chan_h323.c
    branches/1.4/channels/chan_local.c
    branches/1.4/configure
    branches/1.4/configure.ac
    branches/1.4/include/asterisk/autoconfig.h.in
    branches/1.4/include/asterisk/compiler.h
    branches/1.4/include/asterisk/lock.h

Modified: branches/1.4/channels/chan_agent.c
URL: http://svnview.digium.com/svn/asterisk/branches/1.4/channels/chan_agent.c?view=diff&rev=273793&r1=273792&r2=273793
==============================================================================
--- branches/1.4/channels/chan_agent.c (original)
+++ branches/1.4/channels/chan_agent.c Fri Jul  2 16:36:39 2010
@@ -707,7 +707,12 @@
 	ast_mutex_lock(&p->lock);
 	if (p->chan && !ast_check_hangup(p->chan)) {
 		while (ast_channel_trylock(p->chan)) {
-			ast_channel_unlock(ast);
+			int res;
+			if ((res = ast_channel_unlock(ast))) {
+				ast_log(LOG_ERROR, "chan_agent bug! Channel was not locked upon entry to agent_indicate: %s\n", strerror(res));
+				ast_mutex_unlock(&p->lock);
+				return -1;
+			}
 			usleep(1);
 			ast_channel_lock(ast);
 		}

Modified: branches/1.4/channels/chan_dahdi.c
URL: http://svnview.digium.com/svn/asterisk/branches/1.4/channels/chan_dahdi.c?view=diff&rev=273793&r1=273792&r2=273793
==============================================================================
--- branches/1.4/channels/chan_dahdi.c (original)
+++ branches/1.4/channels/chan_dahdi.c Fri Jul  2 16:36:39 2010
@@ -4399,8 +4399,12 @@
 						/* Here we have to retain the lock on both the main channel, the 3-way channel, and
 						   the private structure -- not especially easy or clean */
 						while (p->subs[SUB_THREEWAY].owner && ast_mutex_trylock(&p->subs[SUB_THREEWAY].owner->lock)) {
+							int res;
 							/* Yuck, didn't get the lock on the 3-way, gotta release everything and re-grab! */
-							ast_mutex_unlock(&p->lock);
+							if ((res = ast_mutex_unlock(&p->lock))) {
+								ast_log(LOG_ERROR, "chan_dahdi bug! '&p->lock' was not locked upon entry to 'dahdi_handle_dtmfup': %s\n", strerror(res));
+								return NULL;
+							}
 							DEADLOCK_AVOIDANCE(&ast->lock);
 							/* We can grab ast and p in that order, without worry.  We should make sure
 							   nothing seriously bad has happened though like some sort of bizarre double

Modified: branches/1.4/channels/chan_h323.c
URL: http://svnview.digium.com/svn/asterisk/branches/1.4/channels/chan_h323.c?view=diff&rev=273793&r1=273792&r2=273793
==============================================================================
--- branches/1.4/channels/chan_h323.c (original)
+++ branches/1.4/channels/chan_h323.c Fri Jul  2 16:36:39 2010
@@ -313,10 +313,8 @@
 	if (pvt) {
 		ast_mutex_lock(&pvt->lock);
 		/* Don't hold pvt lock while trying to lock the channel */
-		while(pvt->owner && ast_channel_trylock(pvt->owner)) {
-			ast_mutex_unlock(&pvt->lock);
-			usleep(1);
-			ast_mutex_lock(&pvt->lock);
+		while (pvt->owner && ast_channel_trylock(pvt->owner)) {
+			DEADLOCK_AVOIDANCE(&pvt->lock);
 		}
 
 		if (pvt->owner) {

Modified: branches/1.4/channels/chan_local.c
URL: http://svnview.digium.com/svn/asterisk/branches/1.4/channels/chan_local.c?view=diff&rev=273793&r1=273792&r2=273793
==============================================================================
--- branches/1.4/channels/chan_local.c (original)
+++ branches/1.4/channels/chan_local.c Fri Jul  2 16:36:39 2010
@@ -183,10 +183,18 @@
 
 	/* Ensure that we have both channels locked */
 	while (other && ast_channel_trylock(other)) {
-		ast_mutex_unlock(&p->lock);
+		int res;
+		if ((res = ast_mutex_unlock(&p->lock))) {
+			ast_log(LOG_ERROR, "chan_local bug! '&p->lock' was not locked when entering local_queue_frame! (%s)\n", strerror(res));
+			return -1;
+		}
 		if (us && us_locked) {
 			do {
-				ast_channel_unlock(us);
+				if (ast_channel_unlock(us)) {
+					ast_log(LOG_ERROR, "chan_local bug! Our channel was not locked, yet arguments indicated that it was!!\n");
+					ast_mutex_lock(&p->lock);
+					return -1;
+				}
 				usleep(1);
 				ast_channel_lock(us);
 			} while (ast_mutex_trylock(&p->lock));

Modified: branches/1.4/configure.ac
URL: http://svnview.digium.com/svn/asterisk/branches/1.4/configure.ac?view=diff&rev=273793&r1=273792&r2=273793
==============================================================================
--- branches/1.4/configure.ac (original)
+++ branches/1.4/configure.ac Fri Jul  2 16:36:39 2010
@@ -450,6 +450,7 @@
 AST_GCC_ATTRIBUTE(unused)
 AST_GCC_ATTRIBUTE(always_inline)
 AST_GCC_ATTRIBUTE(deprecated)
+AST_GCC_ATTRIBUTE(warn_unused_result)
 
 AC_MSG_CHECKING(for -ffunction-sections support)
 saved_CFLAGS="${CFLAGS}"

Modified: branches/1.4/include/asterisk/autoconfig.h.in
URL: http://svnview.digium.com/svn/asterisk/branches/1.4/include/asterisk/autoconfig.h.in?view=diff&rev=273793&r1=273792&r2=273793
==============================================================================
--- branches/1.4/include/asterisk/autoconfig.h.in (original)
+++ branches/1.4/include/asterisk/autoconfig.h.in Fri Jul  2 16:36:39 2010
@@ -66,6 +66,10 @@
 /* Define to 1 if your GCC C compiler supports the 'unused' attribute. */
 #undef HAVE_ATTRIBUTE_unused
 
+/* Define to 1 if your GCC C compiler supports the 'warn_unused_result'
+   attribute. */
+#undef HAVE_ATTRIBUTE_warn_unused_result
+
 /* Define to 1 if you have the `bzero' function. */
 #undef HAVE_BZERO
 
@@ -467,7 +471,7 @@
 /* Define to 1 if you have the `strtoq' function. */
 #undef HAVE_STRTOQ
 
-/* Define to 1 if `st_blksize' is a member of `struct stat'. */
+/* Define to 1 if `st_blksize' is member of `struct stat'. */
 #undef HAVE_STRUCT_STAT_ST_BLKSIZE
 
 /* Define to 1 if you have the mISDN Supplemental Services library. */
@@ -656,11 +660,11 @@
 /* Define to the one symbol short name of this package. */
 #undef PACKAGE_TARNAME
 
-/* Define to the home page for this package. */
-#undef PACKAGE_URL
-
 /* Define to the version of this package. */
 #undef PACKAGE_VERSION
+
+/* Define to 1 if the C compiler supports function prototypes. */
+#undef PROTOTYPES
 
 /* Define to necessary symbol if this constant uses a non-standard name on
    your system. */
@@ -680,6 +684,11 @@
 
 /* Define to the type of arg 5 for `select'. */
 #undef SELECT_TYPE_ARG5
+
+/* Define to 1 if the `setvbuf' function takes the buffering type as its
+   second argument and the buffer pointer as the third, as on System V before
+   release 3. */
+#undef SETVBUF_REVERSED
 
 /* The size of `int', as computed by sizeof. */
 #undef SIZEOF_INT
@@ -701,46 +710,50 @@
 /* Define to 1 if your <sys/time.h> declares `struct tm'. */
 #undef TM_IN_SYS_TIME
 
-/* Enable extensions on AIX 3, Interix.  */
+/* Define to 1 if on AIX 3.
+   System headers sometimes define this.
+   We just want to avoid a redefinition error message.  */
 #ifndef _ALL_SOURCE
 # undef _ALL_SOURCE
 #endif
+
+/* Number of bits in a file offset, on hosts where this is settable. */
+#undef _FILE_OFFSET_BITS
+
 /* Enable GNU extensions on systems that have them.  */
 #ifndef _GNU_SOURCE
 # undef _GNU_SOURCE
 #endif
-/* Enable threading extensions on Solaris.  */
+
+/* Define to 1 to make fseeko visible on some hosts (e.g. glibc 2.2). */
+#undef _LARGEFILE_SOURCE
+
+/* Define for large files, on AIX-style hosts. */
+#undef _LARGE_FILES
+
+/* Define to 1 if on MINIX. */
+#undef _MINIX
+
+/* Define to 2 if the system does not provide POSIX.1 features except with
+   this defined. */
+#undef _POSIX_1_SOURCE
+
+/* Define to 1 if you need to in order for `stat' and other things to work. */
+#undef _POSIX_SOURCE
+
+/* Enable extensions on Solaris.  */
+#ifndef __EXTENSIONS__
+# undef __EXTENSIONS__
+#endif
 #ifndef _POSIX_PTHREAD_SEMANTICS
 # undef _POSIX_PTHREAD_SEMANTICS
 #endif
-/* Enable extensions on HP NonStop.  */
 #ifndef _TANDEM_SOURCE
 # undef _TANDEM_SOURCE
 #endif
-/* Enable general extensions on Solaris.  */
-#ifndef __EXTENSIONS__
-# undef __EXTENSIONS__
-#endif
-
-
-/* Number of bits in a file offset, on hosts where this is settable. */
-#undef _FILE_OFFSET_BITS
-
-/* Define to 1 to make fseeko visible on some hosts (e.g. glibc 2.2). */
-#undef _LARGEFILE_SOURCE
-
-/* Define for large files, on AIX-style hosts. */
-#undef _LARGE_FILES
-
-/* Define to 1 if on MINIX. */
-#undef _MINIX
-
-/* Define to 2 if the system does not provide POSIX.1 features except with
-   this defined. */
-#undef _POSIX_1_SOURCE
-
-/* Define to 1 if you need to in order for `stat' and other things to work. */
-#undef _POSIX_SOURCE
+
+/* Define like PROTOTYPES; this can be used by system headers. */
+#undef __PROTOTYPES
 
 /* Define to empty if `const' does not conform to ANSI C. */
 #undef const

Modified: branches/1.4/include/asterisk/compiler.h
URL: http://svnview.digium.com/svn/asterisk/branches/1.4/include/asterisk/compiler.h?view=diff&rev=273793&r1=273792&r2=273793
==============================================================================
--- branches/1.4/include/asterisk/compiler.h (original)
+++ branches/1.4/include/asterisk/compiler.h Fri Jul  2 16:36:39 2010
@@ -59,4 +59,10 @@
 #define attribute_deprecated
 #endif
 
+#ifdef HAVE_ATTRIBUTE_warn_unused_result
+#define attribute_warn_unused_result __attribute__((warn_unused_result))
+#else
+#define attribute_warn_unused_result
+#endif
+
 #endif /* _ASTERISK_COMPILER_H */

Modified: branches/1.4/include/asterisk/lock.h
URL: http://svnview.digium.com/svn/asterisk/branches/1.4/include/asterisk/lock.h?view=diff&rev=273793&r1=273792&r2=273793
==============================================================================
--- branches/1.4/include/asterisk/lock.h (original)
+++ branches/1.4/include/asterisk/lock.h Fri Jul  2 16:36:39 2010
@@ -55,6 +55,7 @@
 #include "asterisk/time.h"
 #endif
 #include "asterisk/logger.h"
+#include "asterisk/compiler.h"
 
 /* internal macro to profile mutexes. Only computes the delay on
  * non-blocking calls.
@@ -204,13 +205,21 @@
 	do { \
 		char __filename[80], __func[80], __mutex_name[80]; \
 		int __lineno; \
-		int __res = ast_find_lock_info(lock, __filename, sizeof(__filename), &__lineno, __func, sizeof(__func), __mutex_name, sizeof(__mutex_name)); \
-		ast_mutex_unlock(lock); \
+		int __res2, __res = ast_find_lock_info(lock, __filename, sizeof(__filename), &__lineno, __func, sizeof(__func), __mutex_name, sizeof(__mutex_name)); \
+		__res2 = ast_mutex_unlock(lock); \
 		usleep(1); \
 		if (__res < 0) { /* Shouldn't ever happen, but just in case... */ \
-			ast_mutex_lock(lock); \
+			if (__res2 == 0) { \
+				ast_mutex_lock(lock); \
+			} else { \
+				ast_log(LOG_WARNING, "Could not unlock mutex '%s': %s and no lock info found!  I will NOT try to relock.\n", #lock, strerror(__res2)); \
+			} \
 		} else { \
-			__ast_pthread_mutex_lock(__filename, __lineno, __func, __mutex_name, lock); \
+			if (__res2 == 0) { \
+				__ast_pthread_mutex_lock(__filename, __lineno, __func, __mutex_name, lock); \
+			} else { \
+				ast_log(LOG_WARNING, "Could not unlock mutex '%s': %s.  {{{Originally locked at %s line %d: (%s) '%s'}}}  I will NOT try to relock.\n", #lock, strerror(__res2), __filename, __lineno, __func, __mutex_name); \
+			} \
 		} \
 	} while (0)
 
@@ -433,6 +442,8 @@
 }
 
 static inline int __ast_pthread_mutex_trylock(const char *filename, int lineno, const char *func,
+                                              const char* mutex_name, ast_mutex_t *t) attribute_warn_unused_result;
+static inline int __ast_pthread_mutex_trylock(const char *filename, int lineno, const char *func,
                                               const char* mutex_name, ast_mutex_t *t)
 {
 	int res;
@@ -712,9 +723,15 @@
 #else /* !DEBUG_THREADS */
 
 #define	DEADLOCK_AVOIDANCE(lock) \
-	ast_mutex_unlock(lock); \
-	usleep(1); \
-	ast_mutex_lock(lock);
+	do { \
+		int __res; \
+		if (!(__res = ast_mutex_unlock(lock))) { \
+			usleep(1); \
+			ast_mutex_lock(lock); \
+		} else { \
+			ast_log(LOG_WARNING, "Failed to unlock mutex '%s' (%s).  I will NOT try to relock. {{{ THIS IS A BUG. }}}\n", #lock, strerror(__res)); \
+		} \
+	} while (0)
 
 
 typedef pthread_mutex_t ast_mutex_t;
@@ -1367,7 +1384,7 @@
 #define ast_channel_lock(x)		ast_mutex_lock(&x->lock)
 /*! \brief Unlock a channel. If DEBUG_CHANNEL_LOCKS is defined 
 	in the Makefile, print relevant output for debugging */
-#define ast_channel_unlock(x)		ast_mutex_unlock(&x->lock)
+#define ast_channel_unlock(x)	ast_mutex_unlock(&x->lock)
 /*! \brief Try locking a channel. If DEBUG_CHANNEL_LOCKS is defined 
 	in the Makefile, print relevant output for debugging */
 #define ast_channel_trylock(x)		ast_mutex_trylock(&x->lock)
@@ -1389,7 +1406,7 @@
 #define ast_channel_trylock(a) __ast_channel_trylock(a, __FILE__, __LINE__, __PRETTY_FUNCTION__)
 /*! \brief Lock AST channel (and print debugging output)
 \note   You need to enable DEBUG_CHANNEL_LOCKS for this function */
-int __ast_channel_trylock(struct ast_channel *chan, const char *file, int lineno, const char *func);
+int __ast_channel_trylock(struct ast_channel *chan, const char *file, int lineno, const char *func) attribute_warn_unused_result;
 #endif
 
 #endif /* _ASTERISK_LOCK_H */




More information about the asterisk-commits mailing list