summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
-rw-r--r--ChangeLog5
-rw-r--r--action.c2
-rw-r--r--tools/omfile.c28
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;