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 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