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/stream.c') 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 v1.2.3 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/stream.c') 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 v1.2.3 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/stream.c') 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 v1.2.3 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/stream.c') 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 v1.2.3 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/stream.c') 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 v1.2.3 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/stream.c') 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 v1.2.3