From 6cf9bf703de35dc530170fcfdd5bb81041e8c2c9 Mon Sep 17 00:00:00 2001 From: Rainer Gerhards Date: Tue, 2 Mar 2010 11:32:43 +0100 Subject: 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. --- tools/omfile.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) (limited to 'tools/omfile.c') 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); -- cgit