From f1c9f5da133f4c6c8b52c5948cc0f61e32c210f1 Mon Sep 17 00:00:00 2001 From: Rainer Gerhards Date: Thu, 3 Apr 2008 08:48:25 +0000 Subject: bugfix: memory leaks in script engine --- syslogd.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) (limited to 'syslogd.c') diff --git a/syslogd.c b/syslogd.c index 1c31fb61..4bed94c8 100644 --- a/syslogd.c +++ b/syslogd.c @@ -170,6 +170,7 @@ DEFobjCurrIf(datetime) DEFobjCurrIf(conf) DEFobjCurrIf(expr) DEFobjCurrIf(vm) +DEFobjCurrIf(var) DEFobjCurrIf(module) DEFobjCurrIf(errmsg) DEFobjCurrIf(net) /* TODO: make go away! */ @@ -928,8 +929,8 @@ static rsRetVal shouldProcessThisMessage(selector_t *f, msg_t *pMsg, int *bProce unsigned short pbMustBeFreed; char *pszPropVal; int bRet = 0; - vm_t *pVM; - var_t *pResult; + vm_t *pVM = NULL; + var_t *pResult = NULL; assert(f != NULL); assert(pMsg != NULL); @@ -995,7 +996,7 @@ static rsRetVal shouldProcessThisMessage(selector_t *f, msg_t *pMsg, int *bProce CHKiRet(vm.ExecProg(pVM, f->f_filterData.f_expr->pVmprg)); CHKiRet(vm.PopBoolFromStack(pVM, &pResult)); dbgprintf("result of expression evaluation: %lld\n", pResult->val.num); - CHKiRet(vm.Destruct(&pVM)); + /* VM is destructed on function exit */ bRet = (pResult->val.num) ? 1 : 0; } else { assert(f->f_filter_type == FILTER_PROP); /* assert() just in case... */ @@ -1051,6 +1052,12 @@ static rsRetVal shouldProcessThisMessage(selector_t *f, msg_t *pMsg, int *bProce } finalize_it: + /* destruct in any case, not just on error, but it makes error handling much easier */ + if(pVM != NULL) { + var.Destruct(&pResult); + vm.Destruct(&pVM); + } + *bProcessMsg = bRet; RETiRet; } @@ -2838,6 +2845,8 @@ InitGlobalClasses(void) /* initialize and use classes. We must be very careful with the order of events. Some * classes use others and if we do not initialize them in the right order, we may end * up with an invalid call. The most important thing that can happen is that an error + pErrObj = "var"; + CHKiRet(objUse(var, CORE_COMPONENT)); * is detected and needs to be logged, wich in turn requires a broader number of classes * to be available. The solution is that we take care in the order of calls AND use a * class immediately after it is initialized. And, of course, we load those classes -- cgit v1.2.3 From c3d20d39fdca2e5fd9c08f13ef5ab15c4e9ad6ae Mon Sep 17 00:00:00 2001 From: Rainer Gerhards Date: Fri, 4 Apr 2008 09:46:54 +0000 Subject: removed the 32 character size limit (from RFC3164) on the tag. This had bad effects on existing envrionments, as sysklogd didn't obey it either (probably another bug in RFC3164...). We now receive the full size, but will modify the outputs so that only 32 characters max are used by default. If you need large tags in the output, you need to provide custom templates. --- syslogd.c | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) (limited to 'syslogd.c') diff --git a/syslogd.c b/syslogd.c index 4bed94c8..99e0efcc 100644 --- a/syslogd.c +++ b/syslogd.c @@ -422,8 +422,8 @@ static uchar template_SyslogProtocol23Format[] = "\"<%PRI%>1 %TIMESTAMP:::date-r static uchar template_TraditionalFileFormat[] = "\"%TIMESTAMP% %HOSTNAME% %syslogtag%%msg:::drop-last-lf%\n\""; static uchar template_FileFormat[] = "\"%TIMESTAMP:::date-rfc3339% %HOSTNAME% %syslogtag%%msg:::drop-last-lf%\n\""; static uchar template_WallFmt[] = "\"\r\n\7Message from syslogd@%HOSTNAME% at %timegenerated% ...\r\n %syslogtag%%msg%\n\r\""; -static uchar template_ForwardFormat[] = "\"<%PRI%>%TIMESTAMP:::date-rfc3339% %HOSTNAME% %syslogtag%%msg%\""; -static uchar template_TraditionalForwardFormat[] = "\"<%PRI%>%TIMESTAMP% %HOSTNAME% %syslogtag%%msg%\""; +static uchar template_ForwardFormat[] = "\"<%PRI%>%TIMESTAMP:::date-rfc3339% %HOSTNAME% %syslogtag:1:32%%msg%\""; +static uchar template_TraditionalForwardFormat[] = "\"<%PRI%>%TIMESTAMP% %HOSTNAME% %syslogtag:1:32%%msg%\""; static uchar template_StdUsrMsgFmt[] = "\" %syslogtag%%msg%\n\r\""; static uchar template_StdDBFmt[] = "\"insert into SystemEvents (Message, Facility, FromHost, Priority, DeviceReportedTime, ReceivedAt, InfoUnitID, SysLogTag) values ('%msg%', %syslogfacility%, '%HOSTNAME%', %syslogpriority%, '%timereported:::date-mysql%', '%timegenerated:::date-mysql%', %iut%, '%syslogtag%')\",SQL"; static uchar template_StdPgSQLFmt[] = "\"insert into SystemEvents (Message, Facility, FromHost, Priority, DeviceReportedTime, ReceivedAt, InfoUnitID, SysLogTag) values ('%msg%', %syslogfacility%, '%HOSTNAME%', %syslogpriority%, '%timereported:::date-pgsql%', '%timegenerated:::date-pgsql%', %iut%, '%syslogtag%')\",STDSQL"; @@ -1408,7 +1408,6 @@ static int parseLegacySyslogMsg(msg_t *pMsg, int flags) * machine that we received the message from and the tag will be empty. This * is meant to be an interim solution, but for now it is in the code. */ - if(bParseHOSTNAMEandTAG && !(flags & INTERNAL_MSG)) { /* parse HOSTNAME - but only if this is network-received! * rger, 2005-11-14: we still have a problem with BSD messages. These messages @@ -1466,16 +1465,18 @@ static int parseLegacySyslogMsg(msg_t *pMsg, int flags) MsgSetHOSTNAME(pMsg, getRcvFrom(pMsg)); } - /* now parse TAG - that should be present in message from - * all sources. + /* now parse TAG - that should be present in message from all sources. * This code is somewhat not compliant with RFC 3164. As of 3164, * the TAG field is ended by any non-alphanumeric character. In * practice, however, the TAG often contains dashes and other things, * which would end the TAG. So it is not desirable. As such, we only * accept colon and SP to be terminators. Even there is a slight difference: * a colon is PART of the TAG, while a SP is NOT part of the tag - * (it is CONTENT). Finally, we allow only up to 32 characters for - * TAG, as it is specified in RFC 3164. + * (it is CONTENT). Starting 2008-04-04, we have removed the 32 character + * size limit (from RFC3164) on the tag. This had bad effects on existing + * envrionments, as sysklogd didn't obey it either (probably another bug + * in RFC3164...). We now receive the full size, but will modify the + * outputs so that only 32 characters max are used by default. */ /* The following code in general is quick & dirty - I need to get * it going for a test, rgerhards 2004-11-16 */ @@ -1489,15 +1490,10 @@ static int parseLegacySyslogMsg(msg_t *pMsg, int flags) rsCStrSetAllocIncrement(pStrB, 33); pWork = pBuf; iCnt = 0; - while(*p2parse && *p2parse != ':' && *p2parse != ' ' && iCnt < 32) { + while(*p2parse && *p2parse != ':' && *p2parse != ' ') { rsCStrAppendChar(pStrB, *p2parse++); ++iCnt; } - if (iCnt == 32) { - while(*p2parse && *p2parse != ':' && *p2parse != ' ') { - ++p2parse; - } - } if(*p2parse == ':') { ++p2parse; rsCStrAppendChar(pStrB, ':'); -- cgit v1.2.3 From eb5339d4ab214e1b01a81d9f26e595148f60d939 Mon Sep 17 00:00:00 2001 From: Rainer Gerhards Date: Fri, 4 Apr 2008 12:19:24 +0000 Subject: bugfix: some messages were emited without hostname --- syslogd.c | 81 +++++++++++++++++++++++++++++++-------------------------------- 1 file changed, 40 insertions(+), 41 deletions(-) (limited to 'syslogd.c') diff --git a/syslogd.c b/syslogd.c index 99e0efcc..355689c0 100644 --- a/syslogd.c +++ b/syslogd.c @@ -2962,6 +2962,46 @@ int realMain(int argc, char **argv) int bImUxSockLoaded = 0; /* already generated a $ModLoad imuxsock? */ uchar legacyConfLine[80]; + gethostname(LocalHostName, sizeof(LocalHostName)); + if ( (p = strchr(LocalHostName, '.')) ) { + *p++ = '\0'; + LocalDomain = p; + } + else + { + LocalDomain = ""; + + /* It's not clearly defined whether gethostname() + * should return the simple hostname or the fqdn. A + * good piece of software should be aware of both and + * we want to distribute good software. Joey + * + * Good software also always checks its return values... + * If syslogd starts up before DNS is up & /etc/hosts + * doesn't have LocalHostName listed, gethostbyname will + * return NULL. + */ + /* TODO: gethostbyname() is not thread-safe, but replacing it is + * not urgent as we do not run on multiple threads here. rgerhards, 2007-09-25 + */ + hent = gethostbyname(LocalHostName); + if(hent) { + snprintf(LocalHostName, sizeof(LocalHostName), "%s", hent->h_name); + + if ( (p = strchr(LocalHostName, '.')) ) + { + *p++ = '\0'; + LocalDomain = p; + } + } + } + + /* Convert to lower case to recognize the correct domain laterly + */ + for (p = (char *)LocalDomain; *p ; p++) + if (isupper((int) *p)) + *p = (char)tolower((int)*p); + CHKiRet(InitGlobalClasses()); /* doing some core initializations */ @@ -3232,47 +3272,6 @@ int realMain(int argc, char **argv) } myPid = getpid(); /* save our pid for further testing (also used for messages) */ - - gethostname(LocalHostName, sizeof(LocalHostName)); - if ( (p = strchr(LocalHostName, '.')) ) { - *p++ = '\0'; - LocalDomain = p; - } - else - { - LocalDomain = ""; - - /* It's not clearly defined whether gethostname() - * should return the simple hostname or the fqdn. A - * good piece of software should be aware of both and - * we want to distribute good software. Joey - * - * Good software also always checks its return values... - * If syslogd starts up before DNS is up & /etc/hosts - * doesn't have LocalHostName listed, gethostbyname will - * return NULL. - */ - /* TODO: gethostbyname() is not thread-safe, but replacing it is - * not urgent as we do not run on multiple threads here. rgerhards, 2007-09-25 - */ - hent = gethostbyname(LocalHostName); - if(hent) { - snprintf(LocalHostName, sizeof(LocalHostName), "%s", hent->h_name); - - if ( (p = strchr(LocalHostName, '.')) ) - { - *p++ = '\0'; - LocalDomain = p; - } - } - } - - /* Convert to lower case to recognize the correct domain laterly - */ - for (p = (char *)LocalDomain; *p ; p++) - if (isupper((int) *p)) - *p = (char)tolower((int)*p); - memset(&sigAct, 0, sizeof (sigAct)); sigemptyset(&sigAct.sa_mask); -- cgit v1.2.3 From 26000d7bf153606fcfe6f1665ff7b441348a20f1 Mon Sep 17 00:00:00 2001 From: Rainer Gerhards Date: Fri, 4 Apr 2008 17:04:54 +0000 Subject: bugfix: segfault with expression-based filters - so far untested --- syslogd.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) (limited to 'syslogd.c') diff --git a/syslogd.c b/syslogd.c index 355689c0..f21fce7e 100644 --- a/syslogd.c +++ b/syslogd.c @@ -1053,10 +1053,11 @@ static rsRetVal shouldProcessThisMessage(selector_t *f, msg_t *pMsg, int *bProce finalize_it: /* destruct in any case, not just on error, but it makes error handling much easier */ - if(pVM != NULL) { - var.Destruct(&pResult); + if(pVM != NULL) vm.Destruct(&pVM); - } + + if(pResult != NULL) + var.Destruct(&pResult); *bProcessMsg = bRet; RETiRet; -- cgit v1.2.3 From 833f4642621699890d05c041d3ace5de24f369ba Mon Sep 17 00:00:00 2001 From: Rainer Gerhards Date: Mon, 7 Apr 2008 12:14:49 +0200 Subject: fixed segfault when using expression based-filters This was introduced by an invalid CVS patch. --- syslogd.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'syslogd.c') diff --git a/syslogd.c b/syslogd.c index f21fce7e..97d700f6 100644 --- a/syslogd.c +++ b/syslogd.c @@ -1056,6 +1056,7 @@ finalize_it: if(pVM != NULL) vm.Destruct(&pVM); +RUNLOG_VAR("%p", var); if(pResult != NULL) var.Destruct(&pResult); @@ -2838,12 +2839,11 @@ InitGlobalClasses(void) /* the following classes were intialized by objClassInit() */ CHKiRet(objUse(errmsg, CORE_COMPONENT)); CHKiRet(objUse(module, CORE_COMPONENT)); + CHKiRet(objUse(var, CORE_COMPONENT)); /* initialize and use classes. We must be very careful with the order of events. Some * classes use others and if we do not initialize them in the right order, we may end * up with an invalid call. The most important thing that can happen is that an error - pErrObj = "var"; - CHKiRet(objUse(var, CORE_COMPONENT)); * is detected and needs to be logged, wich in turn requires a broader number of classes * to be available. The solution is that we take care in the order of calls AND use a * class immediately after it is initialized. And, of course, we load those classes -- cgit v1.2.3 From 2f282692b572f25395aef6cfb6d656105c564a5e Mon Sep 17 00:00:00 2001 From: Rainer Gerhards Date: Mon, 7 Apr 2008 12:36:18 +0200 Subject: cleanup; var object was not dereferenced on termination --- syslogd.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) (limited to 'syslogd.c') diff --git a/syslogd.c b/syslogd.c index 97d700f6..4d9f8257 100644 --- a/syslogd.c +++ b/syslogd.c @@ -1056,7 +1056,6 @@ finalize_it: if(pVM != NULL) vm.Destruct(&pVM); -RUNLOG_VAR("%p", var); if(pResult != NULL) var.Destruct(&pResult); @@ -2900,6 +2899,7 @@ GlobalClassExit(void) objRelease(conf, CORE_COMPONENT); objRelease(expr, CORE_COMPONENT); objRelease(vm, CORE_COMPONENT); + objRelease(var, CORE_COMPONENT); objRelease(datetime, CORE_COMPONENT); /* TODO: implement the rest of the deinit */ @@ -2925,10 +2925,7 @@ GlobalClassExit(void) CHKiRet(templateInit()); #endif /* dummy "classes */ -dbgprintf("pre strExit()\n"); strExit(); -dbgprintf("post strExit()\n"); - #if 0 CHKiRet(objGetObjInterface(&obj)); /* this provides the root pointer for all other queries */ -- cgit v1.2.3