bugfix: potential race condition when adding messages to queue

There was a wrong order of mutex lock operations. It is hard to
believe that really caused problems, but in theory it could and with
threading we often see that theory becomes practice if something is only
used long enough on a fast enough machine with enough CPUs ;)

5 files changed, 22 insertions, 13 deletions

diff --git a/ChangeLog b/ChangeLog
index 125b0ada..23ac95fb 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -13,6 +13,11 @@ Version 3.21.5  [DEVEL] (rgerhards), 2008-09-??
   local host as it knows itself.
 - re-enabled gcc builtin atomic operations and added a proper
   ./configure check
+- bugfix: potential race condition when adding messages to queue
+  There was a wrong order of mutex lock operations. It is hard to
+  believe that really caused problems, but in theory it could and with
+  threading we often see that theory becomes practice if something is only
+  used long enough on a fast enough machine with enough CPUs ;)
 ---------------------------------------------------------------------------
 Version 3.21.4  [DEVEL] (rgerhards), 2008-09-04
 - removed compile time fixed message size limit (was 2K), limit can now
diff --git a/runtime/atomic.h b/runtime/atomic.h
index d6811628..d15f78ee 100644
--- a/runtime/atomic.h
+++ b/runtime/atomic.h
@@ -41,11 +41,15 @@
  * They simply came in too late. -- rgerhards, 2008-04-02
  */
 #ifdef HAVE_ATOMIC_BUILTINS
-#	define ATOMIC_INC(data) ((void) __sync_fetch_and_add(&data, 1))
-#	define ATOMIC_DEC_AND_FETCH(data) __sync_sub_and_fetch(&data, 1)
+#	define ATOMIC_INC(data) ((void) __sync_fetch_and_add(&(data), 1))
+#	define ATOMIC_DEC_AND_FETCH(data) __sync_sub_and_fetch(&(data), 1)
+#	define ATOMIC_FETCH_32BIT(data) ((unsigned) __sync_fetch_and_and(&(data), 0xffffffff))
+#	define ATOMIC_STORE_1_TO_32BIT(data) __sync_lock_test_and_set(&(data), 1)
 #else
 #	warning "atomic builtins not available, using nul operations"
 #	define ATOMIC_INC(data) (++(data))
+#	define ATOMIC_FETCH_32BIT(data) (data)
+#	define ATOMIC_STORE_1_TO_32BIT(data) (data) = 1
 #endif
 
 #endif /* #ifndef INCLUDED_ATOMIC_H */
diff --git a/runtime/debug.h b/runtime/debug.h
index 214b7c05..d9d576b5 100644
--- a/runtime/debug.h
+++ b/runtime/debug.h
@@ -130,7 +130,8 @@ void dbgPrintAllDebugInfo(void);
 
 
 /* debug aides */
-#ifdef RTINST
+//#ifdef RTINST
+#if 0 // temporarily removed for helgrind
 #define d_pthread_mutex_lock(x)      dbgMutexLock(x, pdbgFuncDB, __LINE__, dbgCALLStaCK_POP_POINT )
 #define d_pthread_mutex_unlock(x)    dbgMutexUnlock(x, pdbgFuncDB, __LINE__, dbgCALLStaCK_POP_POINT )
 #define d_pthread_cond_wait(cond, mut)   dbgCondWait(cond, mut, pdbgFuncDB, __LINE__, dbgCALLStaCK_POP_POINT )
diff --git a/runtime/queue.c b/runtime/queue.c
index 7e7d4152..c0a37019 100644
--- a/runtime/queue.c
+++ b/runtime/queue.c
@@ -2171,17 +2171,17 @@ queueEnqObj(queue_t *pThis, flowControl_t flowCtlType, void *pUsr)
 
 finalize_it:
 	if(pThis->qType != QUEUETYPE_DIRECT) {
-		d_pthread_mutex_unlock(pThis->mut);
+		/* make sure at least one worker is running. */
+		if(pThis->qType != QUEUETYPE_DIRECT) {
+			queueAdviseMaxWorkers(pThis);
+		}
+		/* and release the mutex */
 		i = pthread_cond_signal(&pThis->notEmpty);
+		d_pthread_mutex_unlock(pThis->mut);
 		dbgoprint((obj_t*) pThis, "EnqueueMsg signaled condition (%d)\n", i);
 		pthread_setcancelstate(iCancelStateSave, NULL);
 	}
 
-	/* make sure at least one worker is running. */
-	if(pThis->qType != QUEUETYPE_DIRECT) {
-		queueAdviseMaxWorkers(pThis);
-	}
-
 	RETiRet;
 }
 
diff --git a/tools/syslogd.c b/tools/syslogd.c
index b6e1d826..3a637dd8 100644
--- a/tools/syslogd.c
+++ b/tools/syslogd.c
@@ -2832,9 +2832,8 @@ static rsRetVal mainThread()
 
 	CHKiRet(init());
 
-	if(Debug) {
+	if(Debug && debugging_on) {
 		dbgprintf("Debugging enabled, SIGUSR1 to turn off debugging.\n");
-		debugging_on = 1;
 	}
 	/* Send a signal to the parent so it can terminate.
 	 */
@@ -3082,9 +3081,9 @@ doGlblProcessInit(void)
 			fputs(" Already running.\n", stderr);
 			exit(1); /* "good" exit, done if syslogd is already running */
 		}
-	}
-	else
+	} else {
 		debugging_on = 1;
+	}
 
 	/* tuck my process id away */
 	dbgprintf("Writing pidfile %s.\n", PidFile);