From 6946b6b7d9c53979826764f66166b501e783cb2a Mon Sep 17 00:00:00 2001 From: Rainer Gerhards Date: Tue, 22 Sep 2009 12:48:51 +0200 Subject: bugfix: potential race in object loader during use/release of object interface --- runtime/obj.c | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) (limited to 'runtime') diff --git a/runtime/obj.c b/runtime/obj.c index 6ca05cc4..aebea332 100644 --- a/runtime/obj.c +++ b/runtime/obj.c @@ -75,6 +75,7 @@ #include #include #include +#include /* how many objects are supported by rsyslogd? */ #define OBJ_NUM_IDS 100 /* TODO change to a linked list? info: 16 were currently in use 2008-02-29 */ @@ -97,6 +98,7 @@ DEFobjCurrIf(module) DEFobjCurrIf(errmsg) DEFobjCurrIf(strm) static objInfo_t *arrObjInfo[OBJ_NUM_IDS]; /* array with object information pointers */ +static pthread_mutex_t mutObjGlobalOp; /* mutex to guard global operations of the object system */ /* cookies for serialized lines */ @@ -1127,6 +1129,7 @@ UseObj(char *srcFile, uchar *pObjName, uchar *pObjFile, interface_t *pIf) /* DEV debug only: dbgprintf("source file %s requests object '%s', ifIsLoaded %d\n", srcFile, pObjName, pIf->ifIsLoaded); */ + d_pthread_mutex_lock(&mutObjGlobalOp); if(pIf->ifIsLoaded == 1) { ABORT_FINALIZE(RS_RET_OK); /* we are already set */ @@ -1167,6 +1170,8 @@ UseObj(char *srcFile, uchar *pObjName, uchar *pObjFile, interface_t *pIf) pIf->ifIsLoaded = 1; /* we are happy */ finalize_it: + d_pthread_mutex_unlock(&mutObjGlobalOp); + if(pStr != NULL) rsCStrDestruct(&pStr); @@ -1188,15 +1193,16 @@ ReleaseObj(char *srcFile, uchar *pObjName, uchar *pObjFile, interface_t *pIf) /* dev debug only dbgprintf("source file %s releasing object '%s', ifIsLoaded %d\n", srcFile, pObjName, pIf->ifIsLoaded); */ + d_pthread_mutex_lock(&mutObjGlobalOp); if(pObjFile == NULL) FINALIZE; /* if it is not a lodable module, we do not need to do anything... */ if(pIf->ifIsLoaded == 0) { - ABORT_FINALIZE(RS_RET_OK); /* we are not loaded - this is perfectly OK... */ + FINALIZE; /* we are not loaded - this is perfectly OK... */ } else if(pIf->ifIsLoaded == 2) { pIf->ifIsLoaded = 0; /* clean up */ - ABORT_FINALIZE(RS_RET_OK); /* we had a load error and can not continue */ + FINALIZE; /* we had a load error and can not/must not continue */ } CHKiRet(rsCStrConstructFromszStr(&pStr, pObjName)); @@ -1208,6 +1214,8 @@ ReleaseObj(char *srcFile, uchar *pObjName, uchar *pObjFile, interface_t *pIf) pIf->ifIsLoaded = 0; /* indicated "no longer valid" */ finalize_it: + d_pthread_mutex_unlock(&mutObjGlobalOp); + if(pStr != NULL) rsCStrDestruct(&pStr); @@ -1300,8 +1308,9 @@ objClassExit(void) rsRetVal objClassInit(modInfo_t *pModInfo) { - DEFiRet; + pthread_mutexattr_t mutAttr; int i; + DEFiRet; /* first, initialize the object system itself. This must be done * before any other object is created. @@ -1310,6 +1319,13 @@ objClassInit(modInfo_t *pModInfo) arrObjInfo[i] = NULL; } + /* the mutex must be recursive, because objects may call into other + * object identifieres recursively. + */ + pthread_mutexattr_init(&mutAttr); + pthread_mutexattr_settype(&mutAttr, PTHREAD_MUTEX_RECURSIVE); + pthread_mutex_init(&mutObjGlobalOp, &mutAttr); + /* request objects we use */ CHKiRet(objGetObjInterface(&obj)); /* get ourselves ;) */ -- cgit From 4cc2db490a27e4da95f821bcf5eef5c3b281fe17 Mon Sep 17 00:00:00 2001 From: Rainer Gerhards Date: Tue, 22 Sep 2009 14:07:25 +0200 Subject: bugfixes: potential problems in out file zip writer. Problems could lead to abort and/or memory leak. The module is now hardened in a very conservative way, which is sub-optimal from a performance point of view. This should be improved if it has proven reliable in practice. --- runtime/stream.c | 45 +++++++++++++++++++++++++++++++-------------- 1 file changed, 31 insertions(+), 14 deletions(-) (limited to 'runtime') diff --git a/runtime/stream.c b/runtime/stream.c index 8098f778..e8d4a2a0 100644 --- a/runtime/stream.c +++ b/runtime/stream.c @@ -7,6 +7,14 @@ * * File begun on 2008-01-09 by RGerhards * Large modifications in 2009-06 to support using it with omfile, including zip writer. + * Note that this file obtains the zlib wrapper object is needed, but it never frees it + * again. While this sounds like a leak (and one may argue it actually is), there is no + * harm associated with that. The reason is that strm is a core object, so it is terminated + * only when rsyslogd exists. As we could only release on termination (or else bear more + * overhead for keeping track of how many users we have), not releasing zlibw is OK, because + * it will be released when rsyslogd terminates. We may want to revisit this decision if + * 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. * @@ -598,7 +606,7 @@ static rsRetVal strmConstructFinalize(strm_t *pThis) "without zip\n", localRet); } else { /* we use the same size as the original buf, as we would like - * to make sure we can write out everyting with a SINGLE api call! + * to make sure we can write out everything with a SINGLE api call! */ CHKmalloc(pThis->pZipBuf = (Bytef*) malloc(sizeof(uchar) * pThis->sIOBufSize)); } @@ -676,10 +684,6 @@ dbgprintf("XXX: destruct stream %p\n", pThis); if(pThis->fd != -1) strmCloseFile(pThis); - if(pThis->iZipLevel) { /* do we need a zip buf? */ - objRelease(zlibw, LM_ZLIBW_FILENAME); - } - free(pThis->pszDir); free(pThis->pZipBuf); free(pThis->pszCurrFName); @@ -1044,32 +1048,41 @@ finalize_it: * add a config switch so that the user can decide the risk he is ready * to take, but so far this is not yet implemented (not even requested ;)). * rgerhards, 2009-06-04 + * For the time being, we take a very conservative approach and do not run this + * method multithreaded. This is done in an effort to solve a segfault condition + * that seems to be related to the zip code. -- rgerhards, 2009-09-22 + * TODO: make multithreaded again! */ static rsRetVal doZipWrite(strm_t *pThis, uchar *pBuf, size_t lenBuf) { z_stream zstrm; int zRet; /* zlib return state */ + bool bzInitDone = FALSE; + static pthread_mutex_t mut = PTHREAD_MUTEX_INITIALIZER; DEFiRet; assert(pThis != NULL); assert(pBuf != NULL); + pthread_mutex_lock(&mut); + /* allocate deflate state */ zstrm.zalloc = Z_NULL; zstrm.zfree = Z_NULL; zstrm.opaque = Z_NULL; + zstrm.next_in = (Bytef*) pBuf; /* as of zlib doc, this must be set BEFORE DeflateInit2 */ /* see note in file header for the params we use with deflateInit2() */ zRet = zlibw.DeflateInit2(&zstrm, pThis->iZipLevel, Z_DEFLATED, 31, 9, Z_DEFAULT_STRATEGY); if(zRet != Z_OK) { DBGPRINTF("error %d returned from zlib/deflateInit2()\n", zRet); ABORT_FINALIZE(RS_RET_ZLIB_ERR); } + bzInitDone = TRUE; /* now doing the compression */ + zstrm.next_in = (Bytef*) pBuf; /* as of zlib doc, this must be set BEFORE DeflateInit2 */ zstrm.avail_in = lenBuf; - zstrm.next_in = (Bytef*) pBuf; - /* run deflate() on input until output buffer not full, finish - compression if all of source has been read in */ + /* run deflate() on buffer until everything has been compressed */ do { DBGPRINTF("in deflate() loop, avail_in %d, total_in %ld\n", zstrm.avail_in, zstrm.total_in); zstrm.avail_out = pThis->sIOBufSize; @@ -1077,18 +1090,22 @@ doZipWrite(strm_t *pThis, uchar *pBuf, size_t lenBuf) zRet = zlibw.Deflate(&zstrm, Z_FINISH); /* no bad return value */ DBGPRINTF("after deflate, ret %d, avail_out %d\n", zRet, zstrm.avail_out); assert(zRet != Z_STREAM_ERROR); /* state not clobbered */ + if(zstrm.avail_out == pThis->sIOBufSize) + break; /* this is valid, indicates end of compression --> see zlib howto */ CHKiRet(strmPhysWrite(pThis, (uchar*)pThis->pZipBuf, pThis->sIOBufSize - zstrm.avail_out)); } while (zstrm.avail_out == 0); assert(zstrm.avail_in == 0); /* all input will be used */ - - zRet = zlibw.DeflateEnd(&zstrm); - if(zRet != Z_OK) { - DBGPRINTF("error %d returned from zlib/deflateEnd()\n", zRet); - ABORT_FINALIZE(RS_RET_ZLIB_ERR); +finalize_it: + if(bzInitDone) { + zRet = zlibw.DeflateEnd(&zstrm); + if(zRet != Z_OK) { + DBGPRINTF("error %d returned from zlib/deflateEnd()\n", zRet); + } } -finalize_it: + pthread_mutex_unlock(&mut); + RETiRet; } -- cgit From 44844b8af3be216cec340c8ff83fdc6f1d6f1fff Mon Sep 17 00:00:00 2001 From: Rainer Gerhards Date: Tue, 22 Sep 2009 14:22:34 +0200 Subject: minor: increased buffer size to be safe in all cases if the buffer was too small, we would see more API calls, but no failure, so this is no fix! --- runtime/stream.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'runtime') diff --git a/runtime/stream.c b/runtime/stream.c index e8d4a2a0..32467082 100644 --- a/runtime/stream.c +++ b/runtime/stream.c @@ -607,8 +607,9 @@ static rsRetVal strmConstructFinalize(strm_t *pThis) } else { /* we use the same size as the original buf, as we would like * to make sure we can write out everything with a SINGLE api call! + * We add another 128 bytes to take care of the gzip header and "all eventualities". */ - CHKmalloc(pThis->pZipBuf = (Bytef*) malloc(sizeof(uchar) * pThis->sIOBufSize)); + CHKmalloc(pThis->pZipBuf = (Bytef*) malloc(sizeof(uchar) * pThis->sIOBufSize + 128)); } } @@ -854,7 +855,6 @@ dbgprintf("XXX: doAsyncWriteInternal: strm %p, len %ld\n", pThis, (long) lenBuf) if(++pThis->iCnt == 1) pthread_cond_signal(&pThis->notEmpty); -finalize_it: RETiRet; } -- cgit From 37ba1df6e37b2d020001f390ccb2462ae04fc9c1 Mon Sep 17 00:00:00 2001 From: Rainer Gerhards Date: Thu, 24 Sep 2009 09:48:38 +0200 Subject: bugfix: random data could be appended to message, possibly causing segfaults --- runtime/datetime.c | 114 ++++++++++++++++++++++++++++++++++++----------------- runtime/datetime.h | 4 +- runtime/parser.c | 11 ++++-- runtime/rsyslog.h | 1 + 4 files changed, 88 insertions(+), 42 deletions(-) (limited to 'runtime') diff --git a/runtime/datetime.c b/runtime/datetime.c index 40ab4e9c..fc56b27c 100644 --- a/runtime/datetime.c +++ b/runtime/datetime.c @@ -140,6 +140,7 @@ static void getCurrTime(struct syslogTime *t, time_t *ttSeconds) * DO NOT PUT ANY OTHER CODE IN THIS BEGIN ... END BLOCK!!!! */ + /** * Parse a 32 bit integer number from a string. * @@ -147,17 +148,21 @@ static void getCurrTime(struct syslogTime *t, time_t *ttSeconds) * must be positioned at the first digit. Will be updated * so that on return it points to the first character AFTER * the integer parsed. + * \param pLenStr pointer to string length, decremented on exit by + * characters processed + * Note that if an empty string (len < 1) is passed in, + * the method always returns zero. * \retval The number parsed. */ - -static int srSLMGParseInt32(uchar** ppsz) +static int srSLMGParseInt32(uchar** ppsz, int *pLenStr) { register int i; i = 0; - while(isdigit((int) **ppsz)) { + while(*pLenStr > 0 && isdigit((int) **ppsz)) { i = i * 10 + **ppsz - '0'; ++(*ppsz); + --(*pLenStr); } return i; @@ -169,9 +174,13 @@ static int srSLMGParseInt32(uchar** ppsz) * updates the parse pointer position. The pTime parameter * is guranteed to be updated only if a new valid timestamp * could be obtained (restriction added 2008-09-16 by rgerhards). + * This method now also checks the maximum string length it is passed. + * If a *valid* timestamp is found, the string length is decremented + * by the number of characters processed. If it is not a valid timestamp, + * the length is kept unmodified. -- rgerhards, 2009-09-23 */ static rsRetVal -ParseTIMESTAMP3339(struct syslogTime *pTime, uchar** ppszTS) +ParseTIMESTAMP3339(struct syslogTime *pTime, uchar** ppszTS, int *pLenStr) { uchar *pszTS = *ppszTS; /* variables to temporarily hold time information while we parse */ @@ -186,6 +195,7 @@ ParseTIMESTAMP3339(struct syslogTime *pTime, uchar** ppszTS) char OffsetMode; /* UTC offset + or - */ char OffsetHour; /* UTC offset in hours */ int OffsetMinute; /* UTC offset in minutes */ + int lenStr; /* end variables to temporarily hold time information while we parse */ DEFiRet; @@ -193,48 +203,50 @@ ParseTIMESTAMP3339(struct syslogTime *pTime, uchar** ppszTS) assert(ppszTS != NULL); assert(pszTS != NULL); - year = srSLMGParseInt32(&pszTS); + lenStr = *pLenStr; + year = srSLMGParseInt32(&pszTS, &lenStr); /* We take the liberty to accept slightly malformed timestamps e.g. in * the format of 2003-9-1T1:0:0. This doesn't hurt on receiving. Of course, * with the current state of affairs, we would never run into this code * here because at postion 11, there is no "T" in such cases ;) */ - if(*pszTS++ != '-') + if(lenStr == 0 || *pszTS++ != '-') ABORT_FINALIZE(RS_RET_INVLD_TIME); - month = srSLMGParseInt32(&pszTS); + month = srSLMGParseInt32(&pszTS, &lenStr); if(month < 1 || month > 12) ABORT_FINALIZE(RS_RET_INVLD_TIME); - if(*pszTS++ != '-') + if(lenStr == 0 || *pszTS++ != '-') ABORT_FINALIZE(RS_RET_INVLD_TIME); - day = srSLMGParseInt32(&pszTS); + day = srSLMGParseInt32(&pszTS, &lenStr); if(day < 1 || day > 31) ABORT_FINALIZE(RS_RET_INVLD_TIME); - if(*pszTS++ != 'T') + if(lenStr == 0 || *pszTS++ != 'T') ABORT_FINALIZE(RS_RET_INVLD_TIME); - hour = srSLMGParseInt32(&pszTS); + hour = srSLMGParseInt32(&pszTS, &lenStr); if(hour < 0 || hour > 23) ABORT_FINALIZE(RS_RET_INVLD_TIME); - if(*pszTS++ != ':') + if(lenStr == 0 || *pszTS++ != ':') ABORT_FINALIZE(RS_RET_INVLD_TIME); - minute = srSLMGParseInt32(&pszTS); + minute = srSLMGParseInt32(&pszTS, &lenStr); if(minute < 0 || minute > 59) ABORT_FINALIZE(RS_RET_INVLD_TIME); - if(*pszTS++ != ':') + if(lenStr == 0 || *pszTS++ != ':') ABORT_FINALIZE(RS_RET_INVLD_TIME); - second = srSLMGParseInt32(&pszTS); + second = srSLMGParseInt32(&pszTS, &lenStr); if(second < 0 || second > 60) ABORT_FINALIZE(RS_RET_INVLD_TIME); /* Now let's see if we have secfrac */ - if(*pszTS == '.') { + if(lenStr > 0 && *pszTS == '.') { + --lenStr; uchar *pszStart = ++pszTS; - secfrac = srSLMGParseInt32(&pszTS); + secfrac = srSLMGParseInt32(&pszTS, &lenStr); secfracPrecision = (int) (pszTS - pszStart); } else { secfracPrecision = 0; @@ -242,23 +254,27 @@ ParseTIMESTAMP3339(struct syslogTime *pTime, uchar** ppszTS) } /* check the timezone */ - if(*pszTS == 'Z') - { + if(lenStr == 0) + ABORT_FINALIZE(RS_RET_INVLD_TIME); + + if(*pszTS == 'Z') { + --lenStr; pszTS++; /* eat Z */ OffsetMode = 'Z'; OffsetHour = 0; OffsetMinute = 0; } else if((*pszTS == '+') || (*pszTS == '-')) { OffsetMode = *pszTS; + --lenStr; pszTS++; - OffsetHour = srSLMGParseInt32(&pszTS); + OffsetHour = srSLMGParseInt32(&pszTS, &lenStr); if(OffsetHour < 0 || OffsetHour > 23) ABORT_FINALIZE(RS_RET_INVLD_TIME); - if(*pszTS++ != ':') + if(lenStr == 0 || *pszTS++ != ':') ABORT_FINALIZE(RS_RET_INVLD_TIME); - OffsetMinute = srSLMGParseInt32(&pszTS); + OffsetMinute = srSLMGParseInt32(&pszTS, &lenStr); if(OffsetMinute < 0 || OffsetMinute > 59) ABORT_FINALIZE(RS_RET_INVLD_TIME); } else { @@ -267,10 +283,12 @@ ParseTIMESTAMP3339(struct syslogTime *pTime, uchar** ppszTS) } /* OK, we actually have a 3339 timestamp, so let's indicated this */ - if(*pszTS == ' ') + if(lenStr > 0 && *pszTS == ' ') { + --lenStr; ++pszTS; - else + } else { ABORT_FINALIZE(RS_RET_INVLD_TIME); + } /* we had success, so update parse pointer and caller-provided timestamp */ *ppszTS = pszTS; @@ -286,6 +304,7 @@ ParseTIMESTAMP3339(struct syslogTime *pTime, uchar** ppszTS) pTime->OffsetMode = OffsetMode; pTime->OffsetHour = OffsetHour; pTime->OffsetMinute = OffsetMinute; + *pLenStr = lenStr; finalize_it: RETiRet; @@ -304,9 +323,13 @@ finalize_it: * permits us to use a pre-aquired timestamp and thus avoids to do * a (costly) time() call. Thanks to David Lang for insisting on * time() call reduction ;). + * This method now also checks the maximum string length it is passed. + * If a *valid* timestamp is found, the string length is decremented + * by the number of characters processed. If it is not a valid timestamp, + * the length is kept unmodified. -- rgerhards, 2009-09-23 */ static rsRetVal -ParseTIMESTAMP3164(struct syslogTime *pTime, uchar** ppszTS) +ParseTIMESTAMP3164(struct syslogTime *pTime, uchar** ppszTS, int *pLenStr) { /* variables to temporarily hold time information while we parse */ int month; @@ -316,6 +339,7 @@ ParseTIMESTAMP3164(struct syslogTime *pTime, uchar** ppszTS) int minute; int second; /* end variables to temporarily hold time information while we parse */ + int lenStr; uchar *pszTS; DEFiRet; @@ -323,6 +347,8 @@ ParseTIMESTAMP3164(struct syslogTime *pTime, uchar** ppszTS) pszTS = *ppszTS; assert(pszTS != NULL); assert(pTime != NULL); + assert(pLenStr != NULL); + lenStr = *pLenStr; /* If we look at the month (Jan, Feb, Mar, Apr, May, Jun, Jul, Aug, Sep, Oct, Nov, Dec), * we may see the following character sequences occur: @@ -341,6 +367,9 @@ ParseTIMESTAMP3164(struct syslogTime *pTime, uchar** ppszTS) * june, when it first manifested. This also lead to invalid parsing of the rest * of the message, as the time stamp was not detected to be correct. - rgerhards */ + if(lenStr < 3) + ABORT_FINALIZE(RS_RET_INVLD_TIME); + switch(*pszTS++) { case 'J': @@ -455,26 +484,31 @@ ParseTIMESTAMP3164(struct syslogTime *pTime, uchar** ppszTS) ABORT_FINALIZE(RS_RET_INVLD_TIME); } + lenStr -= 3; + /* done month */ - if(*pszTS++ != ' ') + if(lenStr == 0 || *pszTS++ != ' ') ABORT_FINALIZE(RS_RET_INVLD_TIME); /* we accept a slightly malformed timestamp when receiving. This is * we accept one-digit days */ - if(*pszTS == ' ') + if(*pszTS == ' ') { + --lenStr; ++pszTS; + } - day = srSLMGParseInt32(&pszTS); + day = srSLMGParseInt32(&pszTS, &lenStr); if(day < 1 || day > 31) ABORT_FINALIZE(RS_RET_INVLD_TIME); - if(*pszTS++ != ' ') + if(lenStr == 0 || *pszTS++ != ' ') ABORT_FINALIZE(RS_RET_INVLD_TIME); + --lenStr; /* time part */ - hour = srSLMGParseInt32(&pszTS); + hour = srSLMGParseInt32(&pszTS, &lenStr); if(hour > 1970 && hour < 2100) { /* if so, we assume this actually is a year. This is a format found * e.g. in Cisco devices. @@ -484,23 +518,26 @@ ParseTIMESTAMP3164(struct syslogTime *pTime, uchar** ppszTS) year = hour; /* re-query the hour, this time it must be valid */ - if(*pszTS++ != ' ') + if(lenStr == 0 || *pszTS++ != ' ') ABORT_FINALIZE(RS_RET_INVLD_TIME); - hour = srSLMGParseInt32(&pszTS); + --lenStr; + hour = srSLMGParseInt32(&pszTS, &lenStr); } if(hour < 0 || hour > 23) ABORT_FINALIZE(RS_RET_INVLD_TIME); - if(*pszTS++ != ':') + if(lenStr == 0 || *pszTS++ != ':') ABORT_FINALIZE(RS_RET_INVLD_TIME); - minute = srSLMGParseInt32(&pszTS); + --lenStr; + minute = srSLMGParseInt32(&pszTS, &lenStr); if(minute < 0 || minute > 59) ABORT_FINALIZE(RS_RET_INVLD_TIME); - if(*pszTS++ != ':') + if(lenStr == 0 || *pszTS++ != ':') ABORT_FINALIZE(RS_RET_INVLD_TIME); - second = srSLMGParseInt32(&pszTS); + --lenStr; + second = srSLMGParseInt32(&pszTS, &lenStr); if(second < 0 || second > 60) ABORT_FINALIZE(RS_RET_INVLD_TIME); @@ -508,8 +545,10 @@ ParseTIMESTAMP3164(struct syslogTime *pTime, uchar** ppszTS) * invalid format, it occurs frequently enough (e.g. with Cisco devices) * to permit it as a valid case. -- rgerhards, 2008-09-12 */ - if(*pszTS++ == ':') + if(lenStr == 0 || *pszTS++ == ':') { ++pszTS; /* just skip past it */ + --lenStr; + } /* we had success, so update parse pointer and caller-provided timestamp * fields we do not have are not updated in the caller's timestamp. This @@ -526,6 +565,7 @@ ParseTIMESTAMP3164(struct syslogTime *pTime, uchar** ppszTS) pTime->second = second; pTime->secfracPrecision = 0; pTime->secfrac = 0; + *pLenStr = lenStr; finalize_it: RETiRet; diff --git a/runtime/datetime.h b/runtime/datetime.h index efb0a0af..6377a4a4 100644 --- a/runtime/datetime.h +++ b/runtime/datetime.h @@ -36,8 +36,8 @@ typedef struct datetime_s { /* interfaces */ BEGINinterface(datetime) /* name must also be changed in ENDinterface macro! */ void (*getCurrTime)(struct syslogTime *t, time_t *ttSeconds); - rsRetVal (*ParseTIMESTAMP3339)(struct syslogTime *pTime, uchar** ppszTS); - rsRetVal (*ParseTIMESTAMP3164)(struct syslogTime *pTime, uchar** pszTS); + rsRetVal (*ParseTIMESTAMP3339)(struct syslogTime *pTime, uchar** ppszTS, int *); + rsRetVal (*ParseTIMESTAMP3164)(struct syslogTime *pTime, uchar** pszTS, int *); int (*formatTimestampToMySQL)(struct syslogTime *ts, char* pDst, size_t iLenDst); int (*formatTimestampToPgSQL)(struct syslogTime *ts, char *pDst, size_t iLenDst); int (*formatTimestamp3339)(struct syslogTime *ts, char* pBuf, size_t iLenBuf); diff --git a/runtime/parser.c b/runtime/parser.c index 0b45bfd5..079bcf5e 100644 --- a/runtime/parser.c +++ b/runtime/parser.c @@ -274,6 +274,7 @@ finalize_it: RETiRet; } + /* Parse a received message. The object's rawmsg property is taken and * parsed according to the relevant standards. This can later be * extended to support configured parsers. @@ -284,6 +285,7 @@ rsRetVal parseMsg(msg_t *pMsg) DEFiRet; uchar *msg; int pri; + int lenMsg; int iPriText; CHKiRet(sanitizeMessage(pMsg)); @@ -292,8 +294,11 @@ rsRetVal parseMsg(msg_t *pMsg) DBGPRINTF("msg parser: flags %x, from '%s', msg '%s'\n", pMsg->msgFlags, pMsg->pszRcvFrom, pMsg->pszRawMsg); /* pull PRI */ - pri = DEFUPRI; + lenMsg = pMsg->iLenRawMsg; + if(lenMsg == 0) + ABORT_FINALIZE(RS_RET_EMPTY_MSG); msg = pMsg->pszRawMsg; + pri = DEFUPRI; iPriText = 0; if(*msg == '<') { /* while we process the PRI, we also fill the PRI textual representation @@ -301,7 +306,7 @@ rsRetVal parseMsg(msg_t *pMsg) * but it offers us performance... */ pri = 0; - while(isdigit((int) *++msg)) { + while(--lenMsg > 0 && isdigit((int) *++msg)) { pMsg->bufPRI[iPriText++ % 4] = *msg; /* mod 4 to guard against malformed messages! */ pri = 10 * pri + (*msg - '0'); } @@ -342,7 +347,7 @@ rsRetVal parseMsg(msg_t *pMsg) /* finalize message object */ pMsg->msgFlags &= ~NEEDS_PARSING; /* this message is now parsed */ - MsgPrepareEnqueue(pMsg); /* "historical" name - preparese for multi-threading */ + MsgPrepareEnqueue(pMsg); /* "historical" name - prepare for multi-threading */ finalize_it: RETiRet; diff --git a/runtime/rsyslog.h b/runtime/rsyslog.h index 32177a9f..13b54731 100644 --- a/runtime/rsyslog.h +++ b/runtime/rsyslog.h @@ -280,6 +280,7 @@ enum rsRetVal_ /** return value. All methods return this if not specified oth RS_RET_ACTION_FAILED = -2123, /**< action failed and is now suspended (consider this permanent for the time being) */ RS_RET_NONFATAL_CONFIG_ERR = -2124, /**< non-fatal error during config processing */ RS_RET_FILENAME_INVALID = -2140, /**< filename invalid, not found, no access, ... */ + RS_RET_EMPTY_MSG = -2141, /**< provided (raw) MSG is empty */ /* RainerScript error messages (range 1000.. 1999) */ RS_RET_SYSVAR_NOT_FOUND = 1001, /**< system variable could not be found (maybe misspelled) */ -- cgit From e0dde79a806f46790df6104a6bacdbddaa938fd6 Mon Sep 17 00:00:00 2001 From: Rainer Gerhards Date: Thu, 24 Sep 2009 10:49:11 +0200 Subject: bugfix: potential segfault in stream writer on destruction Most severely affected omfile. The problem was that some buffers were freed before the asynchronous writer thread was shut down. So the writer thread accessed invalid data, which may even already be overwritten. Symptoms (with omfile) were segfaults, grabled data and files with random names placed around the file system (most prominently into the root directory). Special thanks to Aaron for helping to track this down. --- runtime/stream.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) (limited to 'runtime') diff --git a/runtime/stream.c b/runtime/stream.c index 32467082..9e88eca1 100644 --- a/runtime/stream.c +++ b/runtime/stream.c @@ -685,11 +685,6 @@ dbgprintf("XXX: destruct stream %p\n", pThis); if(pThis->fd != -1) strmCloseFile(pThis); - free(pThis->pszDir); - free(pThis->pZipBuf); - free(pThis->pszCurrFName); - free(pThis->pszFName); - if(pThis->bAsyncWrite) { stopWriter(pThis); pthread_mutex_destroy(&pThis->mut); @@ -702,6 +697,15 @@ dbgprintf("XXX: destruct stream %p\n", pThis); } else { free(pThis->pIOBuf); } + + /* IMPORTANT: we MUST free this only AFTER the ansyncWriter has been stopped, else + * we get random errors... + */ + free(pThis->pszDir); + free(pThis->pZipBuf); + free(pThis->pszCurrFName); + free(pThis->pszFName); + ENDobjDestruct(strm) -- cgit From eb9da005ba2d21e2f0b2a8ad23ad925d7d628a15 Mon Sep 17 00:00:00 2001 From: Rainer Gerhards Date: Thu, 24 Sep 2009 11:02:00 +0200 Subject: (temporary?) removal of very conservative locks in stream.c ...after we seem to have identified the root cause of the segfault. I leave them commented out so that we can re-activate it if need arises (aka "get some practice drill first"). --- runtime/stream.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'runtime') diff --git a/runtime/stream.c b/runtime/stream.c index 9e88eca1..ff4515fc 100644 --- a/runtime/stream.c +++ b/runtime/stream.c @@ -1068,7 +1068,7 @@ doZipWrite(strm_t *pThis, uchar *pBuf, size_t lenBuf) assert(pThis != NULL); assert(pBuf != NULL); - pthread_mutex_lock(&mut); + //pthread_mutex_lock(&mut); /* allocate deflate state */ zstrm.zalloc = Z_NULL; @@ -1108,7 +1108,7 @@ finalize_it: } } - pthread_mutex_unlock(&mut); + //pthread_mutex_unlock(&mut); RETiRet; } -- cgit From d15eb7fa83fc4ed237a6e692b7a5e3648038333a Mon Sep 17 00:00:00 2001 From: Rainer Gerhards Date: Thu, 24 Sep 2009 16:05:40 +0200 Subject: bugfix: this morning's race patch was incomplete, completing now we needed to release ALL resources (including file handles!) only after the the async writer thread has terminated (else it may access them). In this case, we had a file handle leak, because the handle was sometimes only opened in the async writer, but the close was attempted before the writer even started (in some cases). --- runtime/stream.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) (limited to 'runtime') diff --git a/runtime/stream.c b/runtime/stream.c index ff4515fc..3348fb74 100644 --- a/runtime/stream.c +++ b/runtime/stream.c @@ -677,14 +677,11 @@ CODESTARTobjDestruct(strm) /* Note: mutex will be unlocked in stopWriter! */ d_pthread_mutex_lock(&pThis->mut); +dbgprintf("XXX: destruct stream 1 %p\n", pThis); if(pThis->tOperationsMode != STREAMMODE_READ) strmFlush(pThis); -dbgprintf("XXX: destruct stream %p\n", pThis); - /* ... then free resources */ - if(pThis->fd != -1) - strmCloseFile(pThis); - +dbgprintf("XXX: destruct stream 2 %p\n", pThis); if(pThis->bAsyncWrite) { stopWriter(pThis); pthread_mutex_destroy(&pThis->mut); @@ -697,10 +694,15 @@ dbgprintf("XXX: destruct stream %p\n", pThis); } else { free(pThis->pIOBuf); } +dbgprintf("XXX: destruct stream 3 (doing close) %p\n", pThis); - /* IMPORTANT: we MUST free this only AFTER the ansyncWriter has been stopped, else + /* Finally, we can free the resources. + * IMPORTANT: we MUST free this only AFTER the ansyncWriter has been stopped, else * we get random errors... */ + if(pThis->fd != -1) + strmCloseFile(pThis); + free(pThis->pszDir); free(pThis->pZipBuf); free(pThis->pszCurrFName); -- cgit From 8bab264ba168b5fee36a7b45020e5e2172c74224 Mon Sep 17 00:00:00 2001 From: Rainer Gerhards Date: Tue, 29 Sep 2009 09:50:39 +0200 Subject: minor cleanup & preparation for 4.5.4 --- runtime/stream.c | 9 --------- 1 file changed, 9 deletions(-) (limited to 'runtime') diff --git a/runtime/stream.c b/runtime/stream.c index 3348fb74..ac90df28 100644 --- a/runtime/stream.c +++ b/runtime/stream.c @@ -677,11 +677,9 @@ CODESTARTobjDestruct(strm) /* Note: mutex will be unlocked in stopWriter! */ d_pthread_mutex_lock(&pThis->mut); -dbgprintf("XXX: destruct stream 1 %p\n", pThis); if(pThis->tOperationsMode != STREAMMODE_READ) strmFlush(pThis); -dbgprintf("XXX: destruct stream 2 %p\n", pThis); if(pThis->bAsyncWrite) { stopWriter(pThis); pthread_mutex_destroy(&pThis->mut); @@ -694,7 +692,6 @@ dbgprintf("XXX: destruct stream 2 %p\n", pThis); } else { free(pThis->pIOBuf); } -dbgprintf("XXX: destruct stream 3 (doing close) %p\n", pThis); /* Finally, we can free the resources. * IMPORTANT: we MUST free this only AFTER the ansyncWriter has been stopped, else @@ -850,7 +847,6 @@ doAsyncWriteInternal(strm_t *pThis, size_t lenBuf) DEFiRet; ISOBJ_TYPE_assert(pThis, strm); -dbgprintf("XXX: doAsyncWriteInternal: strm %p, len %ld\n", pThis, (long) lenBuf); while(pThis->iCnt >= STREAM_ASYNC_NUMBUFS) d_pthread_cond_wait(&pThis->notFull, &pThis->mut); @@ -1065,13 +1061,10 @@ doZipWrite(strm_t *pThis, uchar *pBuf, size_t lenBuf) z_stream zstrm; int zRet; /* zlib return state */ bool bzInitDone = FALSE; - static pthread_mutex_t mut = PTHREAD_MUTEX_INITIALIZER; DEFiRet; assert(pThis != NULL); assert(pBuf != NULL); - //pthread_mutex_lock(&mut); - /* allocate deflate state */ zstrm.zalloc = Z_NULL; zstrm.zfree = Z_NULL; @@ -1110,8 +1103,6 @@ finalize_it: } } - //pthread_mutex_unlock(&mut); - RETiRet; } -- cgit From 536415cf3dba053b0d2294b7f1dc8e34328e795f Mon Sep 17 00:00:00 2001 From: Rainer Gerhards Date: Tue, 29 Sep 2009 14:22:11 +0200 Subject: bugfix: invalid handling of zero-sized messages could lead to mis-addressing and potential memory corruption/segfault --- runtime/parser.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'runtime') diff --git a/runtime/parser.c b/runtime/parser.c index 079bcf5e..7eff0801 100644 --- a/runtime/parser.c +++ b/runtime/parser.c @@ -167,6 +167,7 @@ sanitizeMessage(msg_t *pMsg) size_t iMaxLine; assert(pMsg != NULL); + assert(pMsg->iLenRawMsg > 0); # ifdef USE_NETZIP CHKiRet(uncompressMessage(pMsg)); @@ -288,6 +289,9 @@ rsRetVal parseMsg(msg_t *pMsg) int lenMsg; int iPriText; + if(pMsg->iLenRawMsg == 0) + ABORT_FINALIZE(RS_RET_EMPTY_MSG); + CHKiRet(sanitizeMessage(pMsg)); /* we needed to sanitize first, because we otherwise do not have a C-string we can print... */ @@ -295,8 +299,6 @@ rsRetVal parseMsg(msg_t *pMsg) /* pull PRI */ lenMsg = pMsg->iLenRawMsg; - if(lenMsg == 0) - ABORT_FINALIZE(RS_RET_EMPTY_MSG); msg = pMsg->pszRawMsg; pri = DEFUPRI; iPriText = 0; -- cgit From 724ba2b27f4a2c583b3e257e33a2f0b4b9649ef1 Mon Sep 17 00:00:00 2001 From: Rainer Gerhards Date: Thu, 1 Oct 2009 18:47:16 +0200 Subject: bumped version number, corrected error message code (minor nit) --- runtime/rsyslog.h | 1 + 1 file changed, 1 insertion(+) (limited to 'runtime') diff --git a/runtime/rsyslog.h b/runtime/rsyslog.h index 8a043dde..eb5b4537 100644 --- a/runtime/rsyslog.h +++ b/runtime/rsyslog.h @@ -367,6 +367,7 @@ enum rsRetVal_ /** return value. All methods return this if not specified oth RS_RET_FILENAME_INVALID = -2140, /**< filename invalid, not found, no access, ... */ RS_RET_ZLIB_ERR = -2141, /**< error during zlib call */ RS_RET_VAR_NOT_FOUND = -2142, /**< variable not found */ + RS_RET_PEER_CLOSED_CONN = -2144, /**< remote peer closed connection (information, no error) */ /* RainerScript error messages (range 1000.. 1999) */ RS_RET_SYSVAR_NOT_FOUND = 1001, /**< system variable could not be found (maybe misspelled) */ -- cgit