bugfix: potential re-use of free()ed file stream object in omfile

when dynaCache is enabled, the cache is full, a new entry needs to
be allocated, thus the LRU discarded, then a new entry is opend and that
fails. In that case, it looks like the discarded stream may be reused
improperly (based on code analysis, test case and confirmation pending)

3 files changed, 27 insertions, 8 deletions

diff --git a/ChangeLog b/ChangeLog
index dc12e856..7507208d 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -2,6 +2,11 @@
 Version 4.6.2  [v4-stable] (rgerhards), 2010-03-??
 - new feature: "." action type added to support writing files to relative
   pathes (this is primarily meant as a debug aid)
+- bugfix: potential re-use of free()ed file stream object in omfile
+  when dynaCache is enabled, the cache is full, a new entry needs to
+  be allocated, thus the LRU discarded, then a new entry is opend and that
+  fails. In that case, it looks like the discarded stream may be reused
+  improperly (based on code analysis, test case and confirmation pending)
 - added new property replacer option "date-rfc3164-buggyday" primarily
   to ease migration from syslog-ng. See property replacer doc for
   details. [backport from 5.5.3 because urgently needed by some]
diff --git a/action.c b/action.c
index 7cafb5e7..724dea4b 100644
--- a/action.c
+++ b/action.c
@@ -4,7 +4,7 @@
  *
  * File begun on 2007-08-06 by RGerhards (extracted from syslogd.c)
  *
- * Copyright 2007-2009 Rainer Gerhards and Adiscon GmbH.
+ * Copyright 2007-2010 Rainer Gerhards and Adiscon GmbH.
  *
  * This file is part of rsyslog.
  *
diff --git a/tools/omfile.c b/tools/omfile.c
index 0f27f1e9..ae5f350d 100644
--- a/tools/omfile.c
+++ b/tools/omfile.c
@@ -286,13 +286,15 @@ dynaFileDelCacheEntry(dynaFileCacheEntry **pCache, int iEntry, int bFreeEntry)
 
 	DBGPRINTF("Removed entry %d for file '%s' from dynaCache.\n", iEntry,
 		pCache[iEntry]->pName == NULL ? UCHAR_CONSTANT("[OPEN FAILED]") : pCache[iEntry]->pName);
-	/* if the name is NULL, this is an improperly initilized entry which
+// RG: check the "open failed" case -- can this cause trouble (but do we have that situation?)
+	/* if the name is NULL, this is an improperly initialized entry which
 	 * needs to be discarded. In this case, neither the file is to be closed
-	 * not the name to be freed.
+	 * nor the name to be freed.
 	 */
 	if(pCache[iEntry]->pName != NULL) {
 		if(pCache[iEntry]->pStrm != NULL)
 			strm.Destruct(&pCache[iEntry]->pStrm);
+// RG: pStrm should now be NULL...
 		d_free(pCache[iEntry]->pName);
 		pCache[iEntry]->pName = NULL;
 	}
@@ -474,7 +476,7 @@ prepareDynFile(instanceData *pData, uchar *newFileName, unsigned iMsgOpts)
 			if(iFirstFree == -1)
 				iFirstFree = i;
 		} else { /* got an element, let's see if it matches */
-			if(!ustrcmp(newFileName, pCache[i]->pName)) {
+			if(!ustrcmp(newFileName, pCache[i]->pName)) {  // RG:  name == NULL?
 				/* we found our element! */
 				pData->pStrm = pCache[i]->pStrm;
 				pData->iCurrElt = i;
@@ -497,24 +499,38 @@ prepareDynFile(instanceData *pData, uchar *newFileName, unsigned iMsgOpts)
 	 * is error-prone, so I prefer to do it here. -- rgerhards, 2010-03-02
 	 */
 	pData->iCurrElt = -1;
+	/* similarly, we need to set the current pStrm to NULL, because otherwise, if prepareFile() fails,
+	 * we may end up using an old stream. This bug depends on how exactly prepareFile fails,
+	 * but it* could be triggered in the common case of a failed open() system call.
+	 * rgerhards, 2010-03-22
+	 */
+	pData->pStrm = NULL;
 
 	if(iFirstFree == -1 && (pData->iCurrCacheSize < pData->iDynaFileCacheSize)) {
 		/* there is space left, so set it to that index */
 		iFirstFree = pData->iCurrCacheSize++;
 	}
 
+// RG: this is the begin of a potential problem area
+	/* Note that the following code sequence does not work with the cache entry itself,
+	 * but rather with pData->pStrm, the (sole) stream pointer in the non-dynafile case.
+	 * The cache array is only updated after the open was successful. -- rgerhards, 2010-03-21
+	 */
 	if(iFirstFree == -1) {
 		dynaFileDelCacheEntry(pCache, iOldest, 0);
 		iFirstFree = iOldest; /* this one *is* now free ;) */
 	} else {
 		/* we need to allocate memory for the cache structure */
+		/* TODO: performance note: we could alloc all entries on startup, thus saving malloc
+		 *       overhead -- this may be something to consider in v5...
+		 */
 		CHKmalloc(pCache[iFirstFree] = (dynaFileCacheEntry*) calloc(1, sizeof(dynaFileCacheEntry)));
 	}
 
 	/* Ok, we finally can open the file */
 	localRet = prepareFile(pData, newFileName); /* ignore exact error, we check fd below */
 
-	/* file is either open now or an error state set */
+	/* file is either open now or an error state set */ // RG: better check localRet?
 	if(pData->pStrm == NULL) {
 		/* do not report anything if the message is an internally-generated
 		 * message. Otherwise, we could run into a never-ending loop. The bad
@@ -525,13 +541,11 @@ prepareDynFile(instanceData *pData, uchar *newFileName, unsigned iMsgOpts)
 		} else {
 			errmsg.LogError(0, NO_ERRCODE, "Could not open dynamic file '%s' - discarding message", newFileName);
 		}
-		dynaFileDelCacheEntry(pCache, iFirstFree, 1);
 		ABORT_FINALIZE(localRet);
 	}
 
 	if((pCache[iFirstFree]->pName = ustrdup(newFileName)) == NULL) {
-		/* we need to discard the entry, otherwise things could lead to a segfault! */
-		dynaFileDelCacheEntry(pCache, iFirstFree, 1);
+		strm.Destruct(&pData->pStrm); /* need to free failed entry! */
 		ABORT_FINALIZE(RS_RET_OUT_OF_MEMORY);
 	}
 	pCache[iFirstFree]->pStrm = pData->pStrm;