From f7d79b8de13d4fac2de49ad52ba30578b7282e57 Mon Sep 17 00:00:00 2001 From: Rainer Gerhards Date: Mon, 28 Oct 2013 11:45:09 +0100 Subject: bugfix: imtcp flowControl parameter incorrectly defaulted to "off" This could cause message loss on systems under heavy load and was a change-of-behaviour to previous version. This is a regression most probably introduced in 5.9.0 (but did not try hard to find the exact point of its introduction). --- ChangeLog | 5 +++++ plugins/imtcp/imtcp.c | 4 ++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/ChangeLog b/ChangeLog index 014dacc2..c960d714 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,10 @@ --------------------------------------------------------------------------- Version 7.4.6 [v7.4-stable] 2013-11-?? +- bugfix: imtcp flowControl parameter incorrectly defaulted to "off" + This could cause message loss on systems under heavy load and was + a change-of-behaviour to previous version. This is a regression + most probably introduced in 5.9.0 (but did not try hard to find the + exact point of its introduction). - now requires libestr 0.1.9 as earlier versions lead to problems with number handling in RainerScript - bugfix: memory leak in strlen() RainerScript function diff --git a/plugins/imtcp/imtcp.c b/plugins/imtcp/imtcp.c index d2a0e565..e10a8ba3 100644 --- a/plugins/imtcp/imtcp.c +++ b/plugins/imtcp/imtcp.c @@ -408,7 +408,7 @@ CODESTARTbeginCnfLoad loadModConf->iTCPLstnMax = 20; loadModConf->bSuppOctetFram = 1; loadModConf->iStrmDrvrMode = 0; - loadModConf->bUseFlowControl = 0; + loadModConf->bUseFlowControl = 1; loadModConf->bKeepAlive = 0; loadModConf->bEmitMsgOnClose = 0; loadModConf->iAddtlFrameDelim = TCPSRV_NO_ADDTL_DELIMITER; @@ -631,7 +631,7 @@ resetConfigVariables(uchar __attribute__((unused)) *pp, void __attribute__((unus cs.iTCPLstnMax = 20; cs.bSuppOctetFram = 1; cs.iStrmDrvrMode = 0; - cs.bUseFlowControl = 0; + cs.bUseFlowControl = 1; cs.bKeepAlive = 0; cs.bEmitMsgOnClose = 0; cs.iAddtlFrameDelim = TCPSRV_NO_ADDTL_DELIMITER; -- cgit v1.2.3 From 9a775051f7373176c6e54bee1110965342dd41ad Mon Sep 17 00:00:00 2001 From: Rainer Gerhards Date: Mon, 28 Oct 2013 12:56:02 +0100 Subject: bugfix: potential abort during HUP This could happen when one of imklog, imzmq3, imkmsg, impstats, imjournal, or imuxsock were under heavy load during a HUP. closes: http://bugzilla.adiscon.com/show_bug.cgi?id=489 Thanks to Guy Rozendorn for reporting the problem and Peval Levhshin for his analysis. --- ChangeLog | 6 ++++++ runtime/glbl.c | 25 ++++++++++++++++++------- 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/ChangeLog b/ChangeLog index c960d714..b40e3c71 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,11 @@ --------------------------------------------------------------------------- Version 7.4.6 [v7.4-stable] 2013-11-?? +- bugfix: potential abort during HUP + This could happen when one of imklog, imzmq3, imkmsg, impstats, + imjournal, or imuxsock were under heavy load during a HUP. + closes: http://bugzilla.adiscon.com/show_bug.cgi?id=489 + Thanks to Guy Rozendorn for reporting the problem and Peval Levhshin for + his analysis. - bugfix: imtcp flowControl parameter incorrectly defaulted to "off" This could cause message loss on systems under heavy load and was a change-of-behaviour to previous version. This is a regression diff --git a/runtime/glbl.c b/runtime/glbl.c index b3fe3a1d..bcb37953 100644 --- a/runtime/glbl.c +++ b/runtime/glbl.c @@ -32,6 +32,7 @@ #include #include #include +#include #include #include "rsyslog.h" @@ -379,17 +380,24 @@ GetLocalDomain(void) /* generate the local hostname property. This must be done after the hostname info * has been set as well as PreserveFQDN. * rgerhards, 2009-06-30 + * NOTE: This function DELIBERATELY introduces a small memory leak in order to gain + * speed. Each time it is called when a property name already exists, a new one is + * allocated but the previous one is NOT freed. This is so that current readers can + * continue to use the previous name. Otherwise, we would need to use read/write locks + * to protect the update process. As this function is called extremely infrequently and + * the memory leak is very small, this is totally accessible. Think that otherwise we + * would need to place a read look each time the property is read, which is much more + * frequent (once per message for the modules that use this local hostname!). + * rgerhards, 2013-10-28 */ static rsRetVal GenerateLocalHostNameProperty(void) { - DEFiRet; + prop_t *hostnameNew; uchar *pszName; + DEFiRet; - if(propLocalHostName != NULL) - prop.Destruct(&propLocalHostName); - - CHKiRet(prop.Construct(&propLocalHostName)); + CHKiRet(prop.Construct(&hostnameNew)); if(LocalHostNameOverride == NULL) { if(LocalHostName == NULL) pszName = (uchar*) "[localhost]"; @@ -403,8 +411,11 @@ GenerateLocalHostNameProperty(void) pszName = LocalHostNameOverride; } DBGPRINTF("GenerateLocalHostName uses '%s'\n", pszName); - CHKiRet(prop.SetString(propLocalHostName, pszName, ustrlen(pszName))); - CHKiRet(prop.ConstructFinalize(propLocalHostName)); + CHKiRet(prop.SetString(hostnameNew, pszName, ustrlen(pszName))); + CHKiRet(prop.ConstructFinalize(hostnameNew)); + + propLocalHostName = hostnameNew; + /* inititional MEM LEAK for old value -- see function hdr comment! */ finalize_it: RETiRet; -- cgit v1.2.3 From 17e1ee2539cea6bac16832b488afd52b20a348ac Mon Sep 17 00:00:00 2001 From: Rainer Gerhards Date: Mon, 28 Oct 2013 14:17:56 +0100 Subject: remove memleak introduced by GenerateLocalHostName HUP bugfix --- runtime/glbl.c | 45 ++++++++++++++++++++++++++++++++------------- 1 file changed, 32 insertions(+), 13 deletions(-) diff --git a/runtime/glbl.c b/runtime/glbl.c index bcb37953..41d56c2c 100644 --- a/runtime/glbl.c +++ b/runtime/glbl.c @@ -72,6 +72,7 @@ static int option_DisallowWarning = 1; /* complain if message from disallowed se static int bDisableDNS = 0; /* don't look up IP addresses of remote messages */ static prop_t *propLocalIPIF = NULL;/* IP address to report for the local host (default is 127.0.0.1) */ static prop_t *propLocalHostName = NULL;/* our hostname as FQDN - read-only after startup */ +static prop_t *propLocalHostNameToDelete = NULL;/* see GenerateLocalHostName function hdr comment! */ static uchar *LocalHostName = NULL;/* our hostname - read-only after startup, except HUP */ static uchar *LocalHostNameOverride = NULL;/* user-overridden hostname - read-only after startup */ static uchar *LocalFQDNName = NULL;/* our hostname as FQDN - read-only after startup, except HUP */ @@ -380,24 +381,31 @@ GetLocalDomain(void) /* generate the local hostname property. This must be done after the hostname info * has been set as well as PreserveFQDN. * rgerhards, 2009-06-30 - * NOTE: This function DELIBERATELY introduces a small memory leak in order to gain - * speed. Each time it is called when a property name already exists, a new one is - * allocated but the previous one is NOT freed. This is so that current readers can - * continue to use the previous name. Otherwise, we would need to use read/write locks - * to protect the update process. As this function is called extremely infrequently and - * the memory leak is very small, this is totally accessible. Think that otherwise we - * would need to place a read look each time the property is read, which is much more - * frequent (once per message for the modules that use this local hostname!). + * NOTE: This function tries to avoid locking by not destructing the previous value + * immediately. This is so that current readers can continue to use the previous name. + * Otherwise, we would need to use read/write locks to protect the update process. + * In order to do so, we save the previous value and delete it when we are called again + * the next time. Note that this in theory is racy and can lead to a double-free. + * In practice, however, the window of exposure to trigger this is extremely short + * and as this functions is very infrequently being called (on HUP), the trigger + * condition for this bug is so highly unlikely that it never occurs in practice. + * Probably if you HUP rsyslog every few milliseconds, but who does that... + * To further reduce risk potential, we do only update the property when there + * actually is a hostname change, which makes it even less likely. * rgerhards, 2013-10-28 */ static rsRetVal GenerateLocalHostNameProperty(void) { + uchar *pszPrev; + int lenPrev; prop_t *hostnameNew; uchar *pszName; DEFiRet; - CHKiRet(prop.Construct(&hostnameNew)); + if(propLocalHostNameToDelete != NULL) + prop.Destruct(&propLocalHostNameToDelete); + if(LocalHostNameOverride == NULL) { if(LocalHostName == NULL) pszName = (uchar*) "[localhost]"; @@ -411,11 +419,20 @@ GenerateLocalHostNameProperty(void) pszName = LocalHostNameOverride; } DBGPRINTF("GenerateLocalHostName uses '%s'\n", pszName); - CHKiRet(prop.SetString(hostnameNew, pszName, ustrlen(pszName))); - CHKiRet(prop.ConstructFinalize(hostnameNew)); - propLocalHostName = hostnameNew; - /* inititional MEM LEAK for old value -- see function hdr comment! */ + if(propLocalHostName == NULL) + pszPrev = (uchar*)""; /* make sure strcmp() below does not match */ + else + prop.GetString(propLocalHostName, &pszPrev, &lenPrev); + + if(ustrcmp(pszPrev, pszName)) { + /* we need to update */ + CHKiRet(prop.Construct(&hostnameNew)); + CHKiRet(prop.SetString(hostnameNew, pszName, ustrlen(pszName))); + CHKiRet(prop.ConstructFinalize(hostnameNew)); + propLocalHostNameToDelete = propLocalHostName; + propLocalHostName = hostnameNew; + } finalize_it: RETiRet; @@ -678,6 +695,8 @@ BEGINObjClassExit(glbl, OBJ_IS_CORE_MODULE) /* class, version */ free(LocalHostNameOverride); free(LocalFQDNName); objRelease(prop, CORE_COMPONENT); + if(propLocalHostNameToDelete != NULL) + prop.Destruct(&propLocalHostNameToDelete); DESTROY_ATOMIC_HELPER_MUT(mutTerminateInputs); ENDObjClassExit(glbl) -- cgit v1.2.3 From a5f5648f75d01aaea3ec8c07c55d80ad86579ed2 Mon Sep 17 00:00:00 2001 From: Rainer Gerhards Date: Mon, 28 Oct 2013 17:20:36 +0100 Subject: refactor: simplify localHostName property handling --- runtime/glbl.c | 1 + runtime/msg.c | 17 ++--------------- 2 files changed, 3 insertions(+), 15 deletions(-) diff --git a/runtime/glbl.c b/runtime/glbl.c index e1d4e92d..17ae0451 100644 --- a/runtime/glbl.c +++ b/runtime/glbl.c @@ -447,6 +447,7 @@ finalize_it: static prop_t* GetLocalHostNameProp(void) { + prop.AddRef(propLocalHostName); return(propLocalHostName); } diff --git a/runtime/msg.c b/runtime/msg.c index 9f5bcde2..f795242f 100644 --- a/runtime/msg.c +++ b/runtime/msg.c @@ -359,7 +359,7 @@ MsgSetRcvFromIPWithoutAddRef(msg_t *pThis, prop_t *new) /* set RcvFrom name in msg object WITHOUT calling AddRef. * rgerhards, 2013-01-22 */ -void MsgSetRcvFromWithoutAddRef(msg_t *pThis, prop_t *new) +void MsgSetRcvFrom(msg_t *pThis, prop_t *new) { assert(pThis != NULL); @@ -401,7 +401,7 @@ resolveDNS(msg_t *pMsg) { localRet = net.cvthname(pMsg->rcvFrom.pfrominet, &localName, NULL, &ip); if(localRet == RS_RET_OK) { /* we pass down the props, so no need for AddRef */ - MsgSetRcvFromWithoutAddRef(pMsg, localName); + MsgSetRcvFrom(pMsg, localName); MsgSetRcvFromIPWithoutAddRef(pMsg, ip); } } @@ -2263,19 +2263,6 @@ finalize_it: RETiRet; } - -/* rgerhards 2008-09-10: set RcvFrom name in msg object. This calls AddRef() - * on the property, because this must be done in all current cases and there - * is no case expected where this may not be necessary. - * rgerhards, 2009-06-30 - */ -void MsgSetRcvFrom(msg_t *pThis, prop_t *new) -{ - prop.AddRef(new); - MsgSetRcvFromWithoutAddRef(pThis, new); -} - - /* This is used to set the property via a string. This function should not be * called if there is a reliable way for a caller to make sure that the * same name can be used across multiple messages. However, if it can not -- cgit v1.2.3