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.

2 files changed, 35 insertions, 14 deletions

diff --git a/ChangeLog b/ChangeLog
index 7258310b..a12f258e 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -2,6 +2,10 @@
 Version 4.5.4  [v4-beta] (rgerhards), 2009-09-??
 - bugfix: potential race in object loader (obj.c) during use/release
   of object interface
+- 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.
 ---------------------------------------------------------------------------
 Version 4.5.3  [v4-beta] (rgerhards), 2009-09-17
 - bugfix: repeated messages were incorrectly processed
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;
 }