diff options
Diffstat (limited to 'tools/omfile.c')
-rw-r--r-- | tools/omfile.c | 46 |
1 files changed, 25 insertions, 21 deletions
diff --git a/tools/omfile.c b/tools/omfile.c index 0f27f1e9..774f0b96 100644 --- a/tools/omfile.c +++ b/tools/omfile.c @@ -48,7 +48,6 @@ #include <libgen.h> #include <unistd.h> #include <sys/file.h> -#include <pthread.h> #ifdef OS_SOLARIS # include <fcntl.h> @@ -66,7 +65,6 @@ #include "stream.h" #include "unicode-helper.h" #include "atomic.h" -#include "debug.h" MODULE_TYPE_OUTPUT @@ -136,7 +134,6 @@ typedef struct _instanceData { int iIOBufSize; /* size of associated io buffer */ int iFlushInterval; /* how fast flush buffer on inactivity? */ bool bFlushOnTXEnd; /* flush write buffers when transaction has ended? */ - pthread_mutex_t mutEx; /* guard ourselves against being called multiple times with the same instance */ } instanceData; @@ -286,17 +283,18 @@ 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 - * needs to be discarded. In this case, neither the file is to be closed - * not the name to be freed. - */ + if(pCache[iEntry]->pName != NULL) { - if(pCache[iEntry]->pStrm != NULL) - strm.Destruct(&pCache[iEntry]->pStrm); d_free(pCache[iEntry]->pName); pCache[iEntry]->pName = NULL; } + if(pCache[iEntry]->pStrm != NULL) { + strm.Destruct(&pCache[iEntry]->pStrm); + if(pCache[iEntry]->pStrm != NULL) /* safety check -- TODO: remove if no longer necessary */ + abort(); + } + if(bFreeEntry) { d_free(pCache[iEntry]); pCache[iEntry] = NULL; @@ -470,11 +468,11 @@ prepareDynFile(instanceData *pData, uchar *newFileName, unsigned iMsgOpts) iOldest = 0; /* we assume the first element to be the oldest - that will change as we loop */ ttOldest = time(NULL) + 1; /* there must always be an older one */ for(i = 0 ; i < pData->iCurrCacheSize ; ++i) { - if(pCache[i] == NULL) { + if(pCache[i] == NULL || pCache[i]->pName == NULL) { 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 +495,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 +537,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; @@ -606,7 +616,6 @@ finalize_it: BEGINcreateInstance CODESTARTcreateInstance pData->pStrm = NULL; - pthread_mutex_init(&pData->mutEx, NULL); ENDcreateInstance @@ -616,7 +625,6 @@ CODESTARTfreeInstance dynaFileFreeCache(pData); } else if(pData->pStrm != NULL) strm.Destruct(&pData->pStrm); - pthread_mutex_destroy(&pData->mutEx); ENDfreeInstance @@ -626,7 +634,6 @@ ENDtryResume BEGINdoAction CODESTARTdoAction - d_pthread_mutex_lock(&pData->mutEx); DBGPRINTF("file to log to: %s\n", pData->f_fname); CHKiRet(writeFile(ppString, iMsgOpts, pData)); if(pData->bFlushOnTXEnd) { @@ -634,7 +641,6 @@ CODESTARTdoAction CHKiRet(strm.Flush(pData->pStrm)); } finalize_it: - d_pthread_mutex_unlock(&pData->mutEx); ENDdoAction @@ -771,7 +777,6 @@ static rsRetVal resetConfigVariables(uchar __attribute__((unused)) *pp, void __a BEGINdoHUP CODESTARTdoHUP - d_pthread_mutex_lock(&pData->mutEx); if(pData->bDynamicName) { dynaFileFreeCacheEntries(pData); } else { @@ -780,7 +785,6 @@ CODESTARTdoHUP pData->pStrm = NULL; } } - d_pthread_mutex_unlock(&pData->mutEx); ENDdoHUP |