From 50855878062dbcf8d2861ea169d3ca686c65b23b Mon Sep 17 00:00:00 2001 From: Rainer Gerhards Date: Fri, 17 Dec 2010 18:18:57 +0100 Subject: bugfix: imfile potentially duplicates lines This can happen when 0 bytes are read from the input file, and some writer appends data to the file BEFORE we check if a rollover happens. The check for rollover uses the inode and size as a criterion. So far, we checked for equality of sizes, which is not given in this scenario, but that does not indicate a rollover. From the source code comments: Note that when we check the size, we MUST NOT check for equality. The reason is that the file may have been written right after we did try to read (so the file size has increased). That is NOT in indicator of a rollover (this is an actual bug scenario we experienced). So we need to check if the new size is smaller than what we already have seen! --- runtime/stream.c | 5 +++++ 1 file changed, 5 insertions(+) (limited to 'runtime') diff --git a/runtime/stream.c b/runtime/stream.c index 869324ec..baf69881 100644 --- a/runtime/stream.c +++ b/runtime/stream.c @@ -422,10 +422,15 @@ strmHandleEOFMonitor(strm_t *pThis) ABORT_FINALIZE(RS_RET_IO_ERROR); if(stat((char*) pThis->pszCurrFName, &statName) == -1) ABORT_FINALIZE(RS_RET_IO_ERROR); + DBGPRINTF("stream checking for file change on '%s', inode %u/%u, size %lld/%lld\n", + pThis->pszCurrFName, (unsigned) statOpen.st_ino, + (unsigned) statName.st_ino, + pThis->iCurrOffs, (long long) statName.st_size); if(statOpen.st_ino == statName.st_ino && pThis->iCurrOffs == statName.st_size) { ABORT_FINALIZE(RS_RET_EOF); } else { /* we had a file change! */ + DBGPRINTF("we had a file change on '%s'\n", pThis->pszCurrFName); CHKiRet(strmCloseFile(pThis)); CHKiRet(strmOpenFile(pThis)); } -- cgit From 25ff60f5238204ddde0eb7b76ca346372f496ba7 Mon Sep 17 00:00:00 2001 From: Rainer Gerhards Date: Fri, 17 Dec 2010 18:34:39 +0100 Subject: forgot the actual patch with the last commit :( --- runtime/stream.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'runtime') diff --git a/runtime/stream.c b/runtime/stream.c index baf69881..a2eeb039 100644 --- a/runtime/stream.c +++ b/runtime/stream.c @@ -426,7 +426,7 @@ strmHandleEOFMonitor(strm_t *pThis) pThis->pszCurrFName, (unsigned) statOpen.st_ino, (unsigned) statName.st_ino, pThis->iCurrOffs, (long long) statName.st_size); - if(statOpen.st_ino == statName.st_ino && pThis->iCurrOffs == statName.st_size) { + if(statOpen.st_ino == statName.st_ino && pThis->iCurrOffs >= statName.st_size) { ABORT_FINALIZE(RS_RET_EOF); } else { /* we had a file change! */ -- cgit From cdc27aea8f9be98b3f886532191c73bfbc42b8a4 Mon Sep 17 00:00:00 2001 From: Rainer Gerhards Date: Mon, 10 Jan 2011 08:22:50 +0100 Subject: imfile bugfix: potential duplication of log content Under some circumstances an invalid truncation was detected. This code has now been removed, a file change (and thus resent) is only detected if the inode number changes. --- runtime/stream.c | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) (limited to 'runtime') diff --git a/runtime/stream.c b/runtime/stream.c index a2eeb039..af38bcb7 100644 --- a/runtime/stream.c +++ b/runtime/stream.c @@ -400,6 +400,12 @@ finalize_it: * If we are monitoring a file, someone may have rotated it. In this case, we * also need to close it and reopen it under the same name. * rgerhards, 2008-02-13 + * The previous code also did a check for file truncation, in which case the + * file was considered rewritten. However, this potential border case turned + * out to be a big trouble spot on busy systems. It caused massive message + * duplication (I guess stat() can return a too-low number under some + * circumstances). So starting as of now, we only check the inode number and + * a file change is detected only if the inode changes. -- rgerhards, 2011-01-10 */ static rsRetVal strmHandleEOFMonitor(strm_t *pThis) @@ -409,24 +415,14 @@ strmHandleEOFMonitor(strm_t *pThis) struct stat statName; ISOBJ_TYPE_assert(pThis, strm); - /* find inodes of both current descriptor as well as file now in file - * system. If they are different, the file has been rotated (or - * otherwise rewritten). We also check the size, because the inode - * does not change if the file is truncated (this, BTW, is also a case - * where we actually loose log lines, because we can not do anything - * against truncation...). We do NOT rely on the time of last - * modificaton because that may not be available under all - * circumstances. -- rgerhards, 2008-02-13 - */ if(fstat(pThis->fd, &statOpen) == -1) ABORT_FINALIZE(RS_RET_IO_ERROR); if(stat((char*) pThis->pszCurrFName, &statName) == -1) ABORT_FINALIZE(RS_RET_IO_ERROR); - DBGPRINTF("stream checking for file change on '%s', inode %u/%u, size %lld/%lld\n", + DBGPRINTF("stream checking for file change on '%s', inode %u/%u", pThis->pszCurrFName, (unsigned) statOpen.st_ino, - (unsigned) statName.st_ino, - pThis->iCurrOffs, (long long) statName.st_size); - if(statOpen.st_ino == statName.st_ino && pThis->iCurrOffs >= statName.st_size) { + (unsigned) statName.st_ino); + if(statOpen.st_ino == statName.st_ino) { ABORT_FINALIZE(RS_RET_EOF); } else { /* we had a file change! */ -- cgit From 6bad782f154b7f838c7371bf99c13f6dc4ec4101 Mon Sep 17 00:00:00 2001 From: Rainer Gerhards Date: Thu, 10 Feb 2011 17:54:09 +0100 Subject: bugfix: abort if imfile reads file line of more than 64KiB Thanks to Peter Eisentraut for reporting and analysing this problem. bug tracker: http://bugzilla.adiscon.com/show_bug.cgi?id=221 --- runtime/stringbuf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'runtime') diff --git a/runtime/stringbuf.c b/runtime/stringbuf.c index 93995b38..8b2fe455 100644 --- a/runtime/stringbuf.c +++ b/runtime/stringbuf.c @@ -156,7 +156,7 @@ rsRetVal rsCStrExtendBuf(cstr_t *pThis, size_t iMinNeeded) { uchar *pNewBuf; - unsigned short iNewSize; + size_t iNewSize; DEFiRet; /* first compute the new size needed */ -- cgit From d13ec67f9ab9c59a88952af6aec372fd082401ca Mon Sep 17 00:00:00 2001 From: Rainer Gerhards Date: Mon, 2 May 2011 12:03:42 +0200 Subject: slightly more informative errmsg when TCP connection is aborted --- runtime/strmsrv.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'runtime') diff --git a/runtime/strmsrv.c b/runtime/strmsrv.c index 3dc53a97..8cebf810 100644 --- a/runtime/strmsrv.c +++ b/runtime/strmsrv.c @@ -520,6 +520,7 @@ Run(strmsrv_t *pThis) strms_sess_t *pNewSess; nssel_t *pSel; ssize_t iRcvd; + rsRetVal localRet; ISOBJ_TYPE_assert(pThis, strmsrv); @@ -579,11 +580,12 @@ Run(strmsrv_t *pThis) break; case RS_RET_OK: /* valid data received, process it! */ - if(strms_sess.DataRcvd(pThis->pSessions[iSTRMSess], buf, iRcvd) != RS_RET_OK) { + localRet = strms_sess.DataRcvd(pThis->pSessions[iSTRMSess], buf, iRcvd); + if(localRet != RS_RET_OK) { /* in this case, something went awfully wrong. * We are instructed to terminate the session. */ - errmsg.LogError(0, NO_ERRCODE, "Tearing down STRM Session %d - see " + errmsg.LogError(0, localRet, "Tearing down STRM Session %d - see " "previous messages for reason(s)\n", iSTRMSess); pThis->pOnErrClose(pThis->pSessions[iSTRMSess]); strms_sess.Destruct(&pThis->pSessions[iSTRMSess]); -- cgit From 678a904620222b9113db8b5f89e988a064b05cd9 Mon Sep 17 00:00:00 2001 From: Rainer Gerhards Date: Tue, 3 May 2011 09:41:35 +0200 Subject: bugfix: memory and file descriptor leak in stream processing Leaks could occur under some circumstances if the file stream handler errored out during the open call. Among others, this could cause very big memory leaks if there were a problem with unreadable disk queue files. In regard to the memory leak, this closes: http://bugzilla.adiscon.com/show_bug.cgi?id=256 --- runtime/stream.c | 11 +++++++++++ 1 file changed, 11 insertions(+) (limited to 'runtime') diff --git a/runtime/stream.c b/runtime/stream.c index af38bcb7..a12dda41 100644 --- a/runtime/stream.c +++ b/runtime/stream.c @@ -258,6 +258,7 @@ static rsRetVal strmOpenFile(strm_t *pThis) if(pThis->fd != -1) ABORT_FINALIZE(RS_RET_OK); + pThis->pszCurrFName = NULL; /* used to prevent mem leak in case of error */ if(pThis->pszFName == NULL) ABORT_FINALIZE(RS_RET_FILE_PREFIX_MISSING); @@ -289,6 +290,16 @@ static rsRetVal strmOpenFile(strm_t *pThis) (pThis->tOperationsMode == STREAMMODE_READ) ? "READ" : "WRITE", pThis->fd); finalize_it: + if(iRet != RS_RET_OK) { + if(pThis->pszCurrFName != NULL) { + free(pThis->pszCurrFName); + pThis->pszCurrFName = NULL; /* just to prevent mis-adressing down the road... */ + } + if(pThis->fd != -1) { + close(pThis->fd); + pThis->fd = -1; + } + } RETiRet; } -- cgit