From 8805b0f25ff1409a41ecc2e054896e653e4cfa55 Mon Sep 17 00:00:00 2001 From: Rainer Gerhards Date: Tue, 24 Jul 2012 11:11:39 +0200 Subject: bugfix: DA queue could cause abort ...due to invalid mutex synchronisation in DA worker. In case of idle queue, mutex was incorrectly locked. --- ChangeLog | 1 + runtime/queue.c | 8 +++++--- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/ChangeLog b/ChangeLog index 23c55ce8..d12e14c4 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,6 @@ --------------------------------------------------------------------------- Version 5.8.13 [V5-stable] 2012-06-?? +- bugfix: DA queue could cause abort - bugfix: "last message repeated n times" message was missing hostname Thanks to Zdenek Salvet for finding this bug and to Bodik for reporting - bugfix "$PreserveFQDN on" was not honored in some modules diff --git a/runtime/queue.c b/runtime/queue.c index d78ab2e3..280ebd94 100644 --- a/runtime/queue.c +++ b/runtime/queue.c @@ -1743,6 +1743,7 @@ ConsumerDA(qqueue_t *pThis, wti_t *pWti) { int i; int iCancelStateSave; + int bNeedReLock = 0; /**< do we need to lock the mutex again? */ DEFiRet; ISOBJ_TYPE_assert(pThis, qqueue); @@ -1752,6 +1753,7 @@ ConsumerDA(qqueue_t *pThis, wti_t *pWti) /* we now have a non-idle batch of work, so we can release the queue mutex and process it */ d_pthread_mutex_unlock(pThis->mut); + bNeedReLock = 1; /* at this spot, we may be cancelled */ pthread_setcancelstate(PTHREAD_CANCEL_ENABLE, &iCancelStateSave); @@ -1770,10 +1772,10 @@ ConsumerDA(qqueue_t *pThis, wti_t *pWti) /* but now cancellation is no longer permitted */ pthread_setcancelstate(iCancelStateSave, NULL); - /* now we are done, but need to re-aquire the mutex */ - d_pthread_mutex_lock(pThis->mut); - finalize_it: + /* now we are done, but potentially need to re-aquire the mutex */ + if(bNeedReLock) + d_pthread_mutex_lock(pThis->mut); DBGOPRINT((obj_t*) pThis, "DAConsumer returns with iRet %d\n", iRet); RETiRet; } -- cgit From f043778bdc23c7b2baf18c1fc35ba47fa4d8386c Mon Sep 17 00:00:00 2001 From: Rainer Gerhards Date: Tue, 24 Jul 2012 11:20:21 +0200 Subject: bugfix: strm class could abort under some circumstances ... it could pass a NULL pointer to unlink. Depending on OS implementation, this could (or could not...) lead to a segfault. --- runtime/stream.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/runtime/stream.c b/runtime/stream.c index 6b88d3f4..bb1a0a42 100644 --- a/runtime/stream.c +++ b/runtime/stream.c @@ -16,7 +16,7 @@ * it turns out to be problematic. Then, we need to quasi-refcount the number of accesses * to the object. * - * Copyright 2008, 2009 Rainer Gerhards and Adiscon GmbH. + * Copyright 2008-2012 Rainer Gerhards and Adiscon GmbH. * * This file is part of the rsyslog runtime library. * @@ -361,7 +361,7 @@ static rsRetVal strmCloseFile(strm_t *pThis) pThis->fdDir = -1; } - if(pThis->bDeleteOnClose) { + if(pThis->bDeleteOnClose && pThis->pszCurrFName != NULL) { if(unlink((char*) pThis->pszCurrFName) == -1) { char errStr[1024]; int err = errno; @@ -369,14 +369,12 @@ static rsRetVal strmCloseFile(strm_t *pThis) DBGPRINTF("error %d unlinking '%s' - ignored: %s\n", errno, pThis->pszCurrFName, errStr); } - } - - pThis->iCurrOffs = 0; /* we are back at begin of file */ - if(pThis->pszCurrFName != NULL) { free(pThis->pszCurrFName); /* no longer needed in any case (just for open) */ pThis->pszCurrFName = NULL; } + pThis->iCurrOffs = 0; /* we are back at begin of file */ + RETiRet; } -- cgit From 617a7aaa1dc4569e6c151a14889bffe808f984c5 Mon Sep 17 00:00:00 2001 From: Andre Lorbach Date: Tue, 31 Jul 2012 08:19:37 -0700 Subject: bugfix: DA queue fixed handling of bad queue files. --- ChangeLog | 4 ++++ runtime/queue.c | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/ChangeLog b/ChangeLog index d12e14c4..a081893b 100644 --- a/ChangeLog +++ b/ChangeLog @@ -8,6 +8,10 @@ Version 5.8.13 [V5-stable] 2012-06-?? - bugfix: randomized IP option header in omudpspoof caused problems closes: http://bugzilla.adiscon.com/show_bug.cgi?id=327 Thanks to Rick Brown for helping to test out the patch. +- bugfix: DA queue fixed handling of bad queue files. If old queue files + existed, they were not truncated when being reused. this could lead to + extra data being read from them and in consequence data format errors, + which could cause trouble to the queue handler. --------------------------------------------------------------------------- Version 5.8.12 [V5-stable] 2012-06-06 - add small delay (50ms) after sending shutdown message diff --git a/runtime/queue.c b/runtime/queue.c index 280ebd94..6a1cf446 100644 --- a/runtime/queue.c +++ b/runtime/queue.c @@ -707,7 +707,7 @@ static rsRetVal qConstructDisk(qqueue_t *pThis) CHKiRet(strm.SetbSync(pThis->tVars.disk.pWrite, pThis->bSyncQueueFiles)); CHKiRet(strm.SetDir(pThis->tVars.disk.pWrite, glbl.GetWorkDir(), strlen((char*)glbl.GetWorkDir()))); CHKiRet(strm.SetiMaxFiles(pThis->tVars.disk.pWrite, 10000000)); - CHKiRet(strm.SettOperationsMode(pThis->tVars.disk.pWrite, STREAMMODE_WRITE)); + CHKiRet(strm.SettOperationsMode(pThis->tVars.disk.pWrite, STREAMMODE_WRITE_TRUNC)); CHKiRet(strm.SetsType(pThis->tVars.disk.pWrite, STREAMTYPE_FILE_CIRCULAR)); CHKiRet(strm.ConstructFinalize(pThis->tVars.disk.pWrite)); -- cgit From 4d01df57c2f378ceda3bcc400a9df5e50a0c007b Mon Sep 17 00:00:00 2001 From: Rainer Gerhards Date: Wed, 1 Aug 2012 10:14:37 +0200 Subject: bugfix: potential abort if output plugin logged message during shutdown note that none of the rsyslog-provided plugins does this Thanks to bodik and Rohit Prasad for alerting us on this bug and analyzing it. fixes: http://bugzilla.adiscon.com/show_bug.cgi?id=347 --- ChangeLog | 13 +++++++++---- tools/syslogd.c | 22 +++++++++++++++++++--- 2 files changed, 28 insertions(+), 7 deletions(-) diff --git a/ChangeLog b/ChangeLog index a081893b..eb8230dc 100644 --- a/ChangeLog +++ b/ChangeLog @@ -8,10 +8,15 @@ Version 5.8.13 [V5-stable] 2012-06-?? - bugfix: randomized IP option header in omudpspoof caused problems closes: http://bugzilla.adiscon.com/show_bug.cgi?id=327 Thanks to Rick Brown for helping to test out the patch. -- bugfix: DA queue fixed handling of bad queue files. If old queue files - existed, they were not truncated when being reused. this could lead to - extra data being read from them and in consequence data format errors, - which could cause trouble to the queue handler. +- bugfix: DA queue fixed handling of bad queue files + If old queue files existed, they were not truncated when being reused. + This could lead to extra data being read from them and in consequence + data format errors, which could cause trouble to the queue handler. +- bugfix: potential abort if output plugin logged message during shutdown + note that none of the rsyslog-provided plugins does this + Thanks to bodik and Rohit Prasad for alerting us on this bug and + analyzing it. + fixes: http://bugzilla.adiscon.com/show_bug.cgi?id=347 --------------------------------------------------------------------------- Version 5.8.12 [V5-stable] 2012-06-06 - add small delay (50ms) after sending shutdown message diff --git a/tools/syslogd.c b/tools/syslogd.c index cbbb66bc..8fc1d1e1 100644 --- a/tools/syslogd.c +++ b/tools/syslogd.c @@ -725,11 +725,19 @@ submitMsg(msg_t *pMsg) ISOBJ_TYPE_assert(pMsg, msg); pRuleset = MsgGetRuleset(pMsg); - pQueue = (pRuleset == NULL) ? pMsgQueue : ruleset.GetRulesetQueue(pRuleset); + + /* if a plugin logs a message during shutdown, the queue may no longer exist */ + if(pQueue == NULL) { + DBGPRINTF("submitMsg() could not submit message - " + "queue does (no longer?) exist - ignored\n"); + FINALIZE; + } + MsgPrepareEnqueue(pMsg); qqueueEnqObj(pQueue, pMsg->flowCtlType, (void*) pMsg); +finalize_it: RETiRet; } @@ -750,12 +758,20 @@ multiSubmitMsg(multi_submit_t *pMultiSub) if(pMultiSub->nElem == 0) FINALIZE; + pRuleset = MsgGetRuleset(pMultiSub->ppMsgs[0]); + pQueue = (pRuleset == NULL) ? pMsgQueue : ruleset.GetRulesetQueue(pRuleset); + + /* if a plugin logs a message during shutdown, the queue may no longer exist */ + if(pQueue == NULL) { + DBGPRINTF("multiSubmitMsg() could not submit message - " + "queue does (no longer?) exist - ignored\n"); + FINALIZE; + } + for(i = 0 ; i < pMultiSub->nElem ; ++i) { MsgPrepareEnqueue(pMultiSub->ppMsgs[i]); } - pRuleset = MsgGetRuleset(pMultiSub->ppMsgs[0]); - pQueue = (pRuleset == NULL) ? pMsgQueue : ruleset.GetRulesetQueue(pRuleset); iRet = pQueue->MultiEnq(pQueue, pMultiSub); pMultiSub->nElem = 0; -- cgit From 5d2a114743a06f1611980ae75d704b2af5ab97ed Mon Sep 17 00:00:00 2001 From: Rainer Gerhards Date: Wed, 1 Aug 2012 10:32:09 +0200 Subject: undo last queue patch - caused a regression some more elaborate patch is needed and will be provided --- ChangeLog | 4 ---- runtime/queue.c | 2 +- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/ChangeLog b/ChangeLog index eb8230dc..2efbaa8a 100644 --- a/ChangeLog +++ b/ChangeLog @@ -8,10 +8,6 @@ Version 5.8.13 [V5-stable] 2012-06-?? - bugfix: randomized IP option header in omudpspoof caused problems closes: http://bugzilla.adiscon.com/show_bug.cgi?id=327 Thanks to Rick Brown for helping to test out the patch. -- bugfix: DA queue fixed handling of bad queue files - If old queue files existed, they were not truncated when being reused. - This could lead to extra data being read from them and in consequence - data format errors, which could cause trouble to the queue handler. - bugfix: potential abort if output plugin logged message during shutdown note that none of the rsyslog-provided plugins does this Thanks to bodik and Rohit Prasad for alerting us on this bug and diff --git a/runtime/queue.c b/runtime/queue.c index 6a1cf446..280ebd94 100644 --- a/runtime/queue.c +++ b/runtime/queue.c @@ -707,7 +707,7 @@ static rsRetVal qConstructDisk(qqueue_t *pThis) CHKiRet(strm.SetbSync(pThis->tVars.disk.pWrite, pThis->bSyncQueueFiles)); CHKiRet(strm.SetDir(pThis->tVars.disk.pWrite, glbl.GetWorkDir(), strlen((char*)glbl.GetWorkDir()))); CHKiRet(strm.SetiMaxFiles(pThis->tVars.disk.pWrite, 10000000)); - CHKiRet(strm.SettOperationsMode(pThis->tVars.disk.pWrite, STREAMMODE_WRITE_TRUNC)); + CHKiRet(strm.SettOperationsMode(pThis->tVars.disk.pWrite, STREAMMODE_WRITE)); CHKiRet(strm.SetsType(pThis->tVars.disk.pWrite, STREAMTYPE_FILE_CIRCULAR)); CHKiRet(strm.ConstructFinalize(pThis->tVars.disk.pWrite)); -- cgit From 46f7f10e4375553e6a94d7e51b604462808bbff1 Mon Sep 17 00:00:00 2001 From: Rainer Gerhards Date: Wed, 1 Aug 2012 15:37:00 +0200 Subject: more elaborate debug logging for object deserializer --- runtime/obj.c | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/runtime/obj.c b/runtime/obj.c index f9afecd8..fda6051d 100644 --- a/runtime/obj.c +++ b/runtime/obj.c @@ -611,6 +611,8 @@ static rsRetVal objDeserializeProperty(var_t *pProp, strm_t *pStrm) number_t i; number_t iLen; uchar c; + int step = 0; /* which step was successful? */ + int64 offs; assert(pProp != NULL); @@ -631,13 +633,16 @@ static rsRetVal objDeserializeProperty(var_t *pProp, strm_t *pStrm) NEXTC; } CHKiRet(cstrFinalize(pProp->pcsName)); + step = 1; /* property type */ CHKiRet(objDeserializeNumber(&i, pStrm)); pProp->varType = i; + step = 2; /* size (needed for strings) */ CHKiRet(objDeserializeNumber(&iLen, pStrm)); + step = 3; /* we now need to deserialize the value */ switch(pProp->varType) { @@ -654,12 +659,46 @@ static rsRetVal objDeserializeProperty(var_t *pProp, strm_t *pStrm) dbgprintf("invalid VARTYPE %d\n", pProp->varType); break; } + step = 4; /* we should now be at the end of the line. So the next char must be \n */ NEXTC; if(c != '\n') ABORT_FINALIZE(RS_RET_INVALID_PROPFRAME); finalize_it: + if(Debug && iRet != RS_RET_OK) { + strm.GetCurrOffset(pStrm, &offs); + if(step == 0) { + dbgprintf("error %d deserializing property name, offset %lld, step %d\n", + iRet, offs, step); + } + if(step >= 1) { + dbgprintf("error property name: '%s'\n", rsCStrGetSzStrNoNULL(pProp->pcsName)); + } + if(step >= 2) { + dbgprintf("error var type: '%d'\n", pProp->varType); + } + if(step >= 3) { + dbgprintf("error len: '%d'\n", (int) iLen); + } + if(step >= 4) { + switch(pProp->varType) { + case VARTYPE_STR: + dbgprintf("error data string: '%s'\n", + rsCStrGetSzStrNoNULL(pProp->val.pStr)); + break; + case VARTYPE_NUMBER: + dbgprintf("error number: %d\n", (int) pProp->val.num); + break; + case VARTYPE_SYSLOGTIME: + dbgprintf("syslog time was successfully parsed (but " + "is not displayed\n"); + break; + default: + break; + } + } + } RETiRet; } -- cgit From 9fb96ec34312b6da7b496db0b3d5eb86442fed06 Mon Sep 17 00:00:00 2001 From: Rainer Gerhards Date: Wed, 1 Aug 2012 15:56:42 +0200 Subject: nitfix: remove quirck in new debug message code --- runtime/obj.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/runtime/obj.c b/runtime/obj.c index fda6051d..666dd6b0 100644 --- a/runtime/obj.c +++ b/runtime/obj.c @@ -668,10 +668,8 @@ static rsRetVal objDeserializeProperty(var_t *pProp, strm_t *pStrm) finalize_it: if(Debug && iRet != RS_RET_OK) { strm.GetCurrOffset(pStrm, &offs); - if(step == 0) { - dbgprintf("error %d deserializing property name, offset %lld, step %d\n", - iRet, offs, step); - } + dbgprintf("error %d deserializing property name, offset %lld, step %d\n", + iRet, offs, step); if(step >= 1) { dbgprintf("error property name: '%s'\n", rsCStrGetSzStrNoNULL(pProp->pcsName)); } -- cgit From d92ad440da788fea9f17bfa4b0185f12644bf714 Mon Sep 17 00:00:00 2001 From: Rainer Gerhards Date: Wed, 22 Aug 2012 14:30:12 +0200 Subject: bugfix: multiple main queues with same queue file name were not detected This lead to queue file corruption. While the root cause is a config error, it is a bug that this important and hard to find config error was not detected by rsyslog. --- ChangeLog | 4 ++++ runtime/ruleset.c | 6 ++++-- tools/syslogd.c | 41 ++++++++++++++++++++++++++++++++++++++++- 3 files changed, 48 insertions(+), 3 deletions(-) diff --git a/ChangeLog b/ChangeLog index 2efbaa8a..32409fd7 100644 --- a/ChangeLog +++ b/ChangeLog @@ -13,6 +13,10 @@ Version 5.8.13 [V5-stable] 2012-06-?? Thanks to bodik and Rohit Prasad for alerting us on this bug and analyzing it. fixes: http://bugzilla.adiscon.com/show_bug.cgi?id=347 +- bugfix: multiple main queues with same queue file name was not detected + This lead to queue file corruption. While the root cause is a config + error, it is a bug that this important and hard to find config error + was not detected by rsyslog. --------------------------------------------------------------------------- Version 5.8.12 [V5-stable] 2012-06-06 - add small delay (50ms) after sending shutdown message diff --git a/runtime/ruleset.c b/runtime/ruleset.c index 8e241c8a..d608f3ad 100644 --- a/runtime/ruleset.c +++ b/runtime/ruleset.c @@ -500,6 +500,7 @@ debugPrintAll(void) static rsRetVal rulesetCreateQueue(void __attribute__((unused)) *pVal, int *pNewVal) { + uchar *rsname; DEFiRet; if(pCurrRuleset == NULL) { @@ -517,8 +518,9 @@ rulesetCreateQueue(void __attribute__((unused)) *pVal, int *pNewVal) if(pNewVal == 0) FINALIZE; /* if it is turned off, we do not need to change anything ;) */ - dbgprintf("adding a ruleset-specific \"main\" queue"); - CHKiRet(createMainQueue(&pCurrRuleset->pQueue, UCHAR_CONSTANT("ruleset"))); + rsname = (pCurrRuleset->pszName == NULL) ? (uchar*) "[NONAME]" : pCurrRuleset->pszName; + DBGPRINTF("adding a ruleset-specific \"main\" queue for ruleset '%s'\n", rsname); + CHKiRet(createMainQueue(&pCurrRuleset->pQueue, rsname)); finalize_it: RETiRet; diff --git a/tools/syslogd.c b/tools/syslogd.c index 8fc1d1e1..d8e50327 100644 --- a/tools/syslogd.c +++ b/tools/syslogd.c @@ -231,6 +231,11 @@ typedef struct legacyOptsLL_s { } legacyOptsLL_t; legacyOptsLL_t *pLegacyOptsLL = NULL; +struct queuefilenames_s { + struct queuefilenames_s *next; + uchar *name; +} *queuefilenames = NULL; + /* global variables for config file state */ int iCompatibilityMode = 0; /* version we should be compatible with; 0 means sysklogd. It is the default, so if no -c option is given, we make ourselvs @@ -1031,6 +1036,14 @@ static void doDie(int sig) static void freeAllDynMemForTermination(void) { + struct queuefilenames_s *qfn, *qfnDel; + + for(qfn = queuefilenames ; qfn != NULL ; ) { + qfnDel = qfn; + qfn = qfn->next; + free(qfnDel->name); + free(qfnDel); + } free(pszMainMsgQFName); free(pModDir); free(pszConfDAGFile); @@ -1559,6 +1572,10 @@ startInputModules(void) */ rsRetVal createMainQueue(qqueue_t **ppQueue, uchar *pszQueueName) { + struct queuefilenames_s *qfn; + uchar *qfname = NULL; + static int qfn_renamenum = 0; + uchar qfrenamebuf[1024]; DEFiRet; /* switch the message object to threaded operation, if necessary */ @@ -1584,10 +1601,32 @@ rsRetVal createMainQueue(qqueue_t **ppQueue, uchar *pszQueueName) errmsg.LogError(0, NO_ERRCODE, "Invalid " #directive ", error %d. Ignored, running with default setting", iRet); \ } + if(pszMainMsgQFName != NULL) { + /* check if the queue file name is unique, else emit an error */ + for(qfn = queuefilenames ; qfn != NULL ; qfn = qfn->next) { + dbgprintf("check queue file name '%s' vs '%s'\n", qfn->name, pszMainMsgQFName); + if(!ustrcmp(qfn->name, pszMainMsgQFName)) { + snprintf((char*)qfrenamebuf, sizeof(qfrenamebuf), "%d-%s-%s", + ++qfn_renamenum, pszMainMsgQFName, + (pszQueueName == NULL) ? "NONAME" : (char*)pszQueueName); + qfname = ustrdup(qfrenamebuf); + errmsg.LogError(0, NO_ERRCODE, "Error: queue file name '%s' already in use " + " - using '%s' instead", pszMainMsgQFName, qfname); + break; + } + } + if(qfname == NULL) + qfname = ustrdup(pszMainMsgQFName); + qfn = malloc(sizeof(struct queuefilenames_s)); + qfn->name = qfname; + qfn->next = queuefilenames; + queuefilenames = qfn; + } + setQPROP(qqueueSetMaxFileSize, "$MainMsgQueueFileSize", iMainMsgQueMaxFileSize); setQPROP(qqueueSetsizeOnDiskMax, "$MainMsgQueueMaxDiskSpace", iMainMsgQueMaxDiskSpace); setQPROP(qqueueSetiDeqBatchSize, "$MainMsgQueueDequeueBatchSize", iMainMsgQueDeqBatchSize); - setQPROPstr(qqueueSetFilePrefix, "$MainMsgQueueFileName", pszMainMsgQFName); + setQPROPstr(qqueueSetFilePrefix, "$MainMsgQueueFileName", qfname); setQPROP(qqueueSetiPersistUpdCnt, "$MainMsgQueueCheckpointInterval", iMainMsgQPersistUpdCnt); setQPROP(qqueueSetbSyncQueueFiles, "$MainMsgQueueSyncQueueFiles", bMainMsgQSyncQeueFiles); setQPROP(qqueueSettoQShutdown, "$MainMsgQueueTimeoutShutdown", iMainMsgQtoQShutdown ); -- cgit