bugfix: potential (but very impropable) segfaults in omfile

- bugfix: potential segfault in omfile when a dynafile open failed
  In that case, a partial cache entry was written, and some internal
  pointers (iCurrElt) not correctly updated. In the next iteration, that
  could lead to a segfault, especially if iCurrElt then points to the
  then-partial record. Not very likely, but could happen in practice.
- bugfix (theoretical): potential segfault in omfile under low memory
  condition. This is only a theoretical bug, because it would only
  happen when strdup() fails to allocate memory - which is highly
  unlikely and will probably lead to all other sorts of errors.

2 files changed, 23 insertions, 2 deletions

diff --git a/ChangeLog b/ChangeLog
index a16c89da..893e67fd 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -6,6 +6,15 @@ Version 4.6.1  [v4-stable] (rgerhards), 2010-02-??
 - bugfix: fixed problem that caused compilation on FreeBSD 9.0 to fail.
   bugtracker: http://bugzilla.adiscon.com/show_bug.cgi?id=181
   Thanks to Christiano for reporting.
+- bugfix: potential segfault in omfile when a dynafile open failed
+  In that case, a partial cache entry was written, and some internal
+  pointers (iCurrElt) not correctly updated. In the next iteration, that
+  could lead to a segfault, especially if iCurrElt then points to the
+  then-partial record. Not very likely, but could happen in practice.
+- bugfix (theoretical): potential segfault in omfile under low memory
+  condition. This is only a theoretical bug, because it would only 
+  happen when strdup() fails to allocate memory - which is highly 
+  unlikely and will probably lead to all other sorts of errors.
 - bugfix: comment char ('#') in literal terminated script parsing
   and thus could not be used.
   but tracker: http://bugzilla.adiscon.com/show_bug.cgi?id=119
diff --git a/tools/omfile.c b/tools/omfile.c
index 069fc078..eb56201c 100644
--- a/tools/omfile.c
+++ b/tools/omfile.c
@@ -318,6 +318,7 @@ dynaFileFreeCacheEntries(instanceData *pData)
 	for(i = 0 ; i < pData->iCurrCacheSize ; ++i) {
 		dynaFileDelCacheEntry(pData->dynCache, i, 1);
 	}
+	pData->iCurrElt = -1; /* invalidate current element */
 	ENDfunc;
 }
 
@@ -486,6 +487,14 @@ prepareDynFile(instanceData *pData, uchar *newFileName, unsigned iMsgOpts)
 	}
 
 	/* we have not found an entry */
+
+	/* invalidate iCurrElt as we may error-exit out of this function when the currrent
+	 * iCurrElt has been freed or otherwise become unusable. This is a precaution, and
+	 * performance-wise it may be better to do that in each of the exits. However, that
+	 * is error-prone, so I prefer to do it here. -- rgerhards, 2010-03-02
+	 */
+	pData->iCurrElt = -1;
+
 	if(iFirstFree == -1 && (pData->iCurrCacheSize < pData->iDynaFileCacheSize)) {
 		/* there is space left, so set it to that index */
 		iFirstFree = pData->iCurrCacheSize++;
@@ -517,7 +526,11 @@ prepareDynFile(instanceData *pData, uchar *newFileName, unsigned iMsgOpts)
 		ABORT_FINALIZE(localRet);
 	}
 
-	CHKmalloc(pCache[iFirstFree]->pName = ustrdup(newFileName));
+	if((pCache[iFirstFree]->pName = ustrdup(newFileName)) == NULL) {
+		/* we need to discard the entry, otherwise things could lead to a segfault! */
+		dynaFileDelCacheEntry(pCache, iFirstFree, 1);
+		ABORT_FINALIZE(RS_RET_OUT_OF_MEMORY);
+	}
 	pCache[iFirstFree]->pStrm = pData->pStrm;
 	pCache[iFirstFree]->lastUsed = time(NULL); // monotonically increasing value! TODO: performance
 	pData->iCurrElt = iFirstFree;
@@ -752,7 +765,6 @@ BEGINdoHUP
 CODESTARTdoHUP
 	if(pData->bDynamicName) {
 		dynaFileFreeCacheEntries(pData);
-		pData->iCurrElt = -1; /* invalidate current element */
 	} else {
 		if(pData->pStrm != NULL) {
 			strm.Destruct(&pData->pStrm);