From 35673b12c42429786f6229ff9fcef7001a6b21ab Mon Sep 17 00:00:00 2001 From: Rainer Gerhards Date: Tue, 27 Jan 2009 22:41:14 +0100 Subject: bugfix: proper message locking on message destruct It looks like a race was introduced by not locking the message mutex in msgDestruct(). In theory, I thought, the decrement should be atomic, but the whole operation may be reordered. Also it has potential for task switches. If so, that would lead to a too-early destruction and thus a potential double free - exactly what we have seen from time to time. So I think this fix addresses the issue. I have also removed anything that looks like atomic operations are supported in this version - they are not. This was very late added, found to be non-portable and pulled from that release. --- msg.c | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/msg.c b/msg.c index bd1e425e..107df1f9 100644 --- a/msg.c +++ b/msg.c @@ -272,11 +272,8 @@ BEGINobjDestruct(msg) /* be sure to specify the object type also in END and CODE int currRefCount; CODESTARTobjDestruct(msg) /* DEV Debugging only ! dbgprintf("msgDestruct\t0x%lx, Ref now: %d\n", (unsigned long)pM, pM->iRefCount - 1); */ -# ifdef DO_HAVE_ATOMICS - currRefCount = ATOMIC_DEC_AND_FETCH(pThis->iRefCount); -# else - currRefCount = --pThis->iRefCount; -# endif + MsgLock(pM); + currRefCount = --pThis->iRefCount; if(currRefCount == 0) { /* DEV Debugging Only! dbgprintf("msgDestruct\t0x%lx, RefCount now 0, doing DESTROY\n", (unsigned long)pThis); */ @@ -328,8 +325,10 @@ CODESTARTobjDestruct(msg) rsCStrDestruct(&pThis->pCSPROCID); if(pThis->pCSMSGID != NULL) rsCStrDestruct(&pThis->pCSMSGID); + MsgUnlock(pM); funcDeleteMutex(pThis); } else { + MsgUnlock(pM); pThis = NULL; /* tell framework not to destructing the object! */ } ENDobjDestruct(msg) @@ -472,13 +471,9 @@ finalize_it: msg_t *MsgAddRef(msg_t *pM) { assert(pM != NULL); -# ifdef DO_HAVE_ATOMICS - ATOMIC_INC(pM->iRefCount); -# else - MsgLock(pM); - pM->iRefCount++; - MsgUnlock(pM); -# endif + MsgLock(pM); + pM->iRefCount++; + MsgUnlock(pM); /* DEV debugging only! dbgprintf("MsgAddRef\t0x%x done, Ref now: %d\n", (int)pM, pM->iRefCount);*/ return(pM); } -- cgit v1.2.3 From 5453699428b8f5a7dd6ac23abe7b838dd19101b5 Mon Sep 17 00:00:00 2001 From: Rainer Gerhards Date: Wed, 28 Jan 2009 13:02:45 +0100 Subject: fixed copy&paste error ... one should at least compile before comitting - sorry, been so eager to push that out. --- msg.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/msg.c b/msg.c index 107df1f9..2261593a 100644 --- a/msg.c +++ b/msg.c @@ -272,7 +272,7 @@ BEGINobjDestruct(msg) /* be sure to specify the object type also in END and CODE int currRefCount; CODESTARTobjDestruct(msg) /* DEV Debugging only ! dbgprintf("msgDestruct\t0x%lx, Ref now: %d\n", (unsigned long)pM, pM->iRefCount - 1); */ - MsgLock(pM); + MsgLock(pThis); currRefCount = --pThis->iRefCount; if(currRefCount == 0) { @@ -325,10 +325,10 @@ CODESTARTobjDestruct(msg) rsCStrDestruct(&pThis->pCSPROCID); if(pThis->pCSMSGID != NULL) rsCStrDestruct(&pThis->pCSMSGID); - MsgUnlock(pM); + MsgUnlock(pThis); funcDeleteMutex(pThis); } else { - MsgUnlock(pM); + MsgUnlock(pThis); pThis = NULL; /* tell framework not to destructing the object! */ } ENDobjDestruct(msg) -- cgit v1.2.3 From 82d7abc1f4b03f37ff94c5f184d49502b6cf489b Mon Sep 17 00:00:00 2001 From: Rainer Gerhards Date: Fri, 30 Jan 2009 13:41:12 +0100 Subject: bugfix: invalid mutex access in msg.c --- ChangeLog | 3 +++ runtime/msg.c | 7 +++++-- runtime/msg.h | 1 + tools/syslogd.c | 2 +- 4 files changed, 10 insertions(+), 3 deletions(-) diff --git a/ChangeLog b/ChangeLog index bccb1f89..360a2bdc 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,4 +1,7 @@ --------------------------------------------------------------------------- +Version 3.20.4 [v3-stable] (rgerhards), 2009-02-?? +- bugfix: invalid mutex access in msg.c +--------------------------------------------------------------------------- Version 3.20.3 [v3-stable] (rgerhards), 2009-01-19 - doc bugfix: v3-compatiblity document had typo in config directive thanks to Andrej for reporting this diff --git a/runtime/msg.c b/runtime/msg.c index c8dbf2c2..064ed44b 100644 --- a/runtime/msg.c +++ b/runtime/msg.c @@ -188,6 +188,7 @@ static void MsgPrepareEnqueueLockingCase(msg_t *pThis) * rgerhards, 2008-07-14 */ pthread_mutexattr_destroy(&pThis->mutAttr); + pThis->bDoLock = 1; ENDfunc } @@ -197,14 +198,16 @@ static void MsgLockLockingCase(msg_t *pThis) { /* DEV debug only! dbgprintf("MsgLock(0x%lx)\n", (unsigned long) pThis); */ assert(pThis != NULL); - pthread_mutex_lock(&pThis->mut); + if(pThis->bDoLock == 1) /* TODO: this is a testing hack, we should find a way with better performance! -- rgerhards, 2009-01-27 */ + pthread_mutex_lock(&pThis->mut); } static void MsgUnlockLockingCase(msg_t *pThis) { /* DEV debug only! dbgprintf("MsgUnlock(0x%lx)\n", (unsigned long) pThis); */ assert(pThis != NULL); - pthread_mutex_unlock(&pThis->mut); + if(pThis->bDoLock == 1) /* TODO: this is a testing hack, we should find a way with better performance! -- rgerhards, 2009-01-27 */ + pthread_mutex_unlock(&pThis->mut); } /* delete the mutex object on message destruction (locking case) diff --git a/runtime/msg.h b/runtime/msg.h index c428237a..fadbb48a 100644 --- a/runtime/msg.h +++ b/runtime/msg.h @@ -51,6 +51,7 @@ struct msg { BEGINobjInstance; /* Data to implement generic object - MUST be the first data element! */ pthread_mutexattr_t mutAttr; +short bDoLock; /* use the mutex? */ pthread_mutex_t mut; int iRefCount; /* reference counter (0 = unused) */ short bParseHOSTNAME; /* should the hostname be parsed from the message? */ diff --git a/tools/syslogd.c b/tools/syslogd.c index b32ce029..2c66daac 100644 --- a/tools/syslogd.c +++ b/tools/syslogd.c @@ -1607,7 +1607,7 @@ logmsg(msg_t *pMsg, int flags) assert(pMsg != NULL); assert(pMsg->pszUxTradMsg != NULL); msg = (char*) pMsg->pszUxTradMsg; - dbgprintf("logmsg: flags %x, pri %s, from '%s', msg %s\n", flags, getPRI(pMsg), getRcvFrom(pMsg), msg); + dbgprintf("logmsg: flags %x, from '%s', msg %s\n", flags, getRcvFrom(pMsg), msg); /* rger 2005-11-24 (happy thanksgiving!): we now need to check if we have * a traditional syslog message or one formatted according to syslog-protocol. -- cgit v1.2.3 From a2bfabc1c0cb1afd02d53ae731779486b566ddb6 Mon Sep 17 00:00:00 2001 From: Rainer Gerhards Date: Fri, 30 Jan 2009 13:43:57 +0100 Subject: finished adding race condition fix (mostly done by merge) --- ChangeLog | 3 +++ 1 file changed, 3 insertions(+) diff --git a/ChangeLog b/ChangeLog index 360a2bdc..d3bfdb0a 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,8 @@ --------------------------------------------------------------------------- Version 3.20.4 [v3-stable] (rgerhards), 2009-02-?? +- bugfix: inconsistent use of mutex/atomic operations could cause segfault + details are too many, for full analysis see blog post at: + http://blog.gerhards.net/2009/01/rsyslog-data-race-analysis.html - bugfix: invalid mutex access in msg.c --------------------------------------------------------------------------- Version 3.20.3 [v3-stable] (rgerhards), 2009-01-19 -- cgit v1.2.3