From 5ed58ba958f88080cf9bb479145695420940cc15 Mon Sep 17 00:00:00 2001 From: Rainer Gerhards Date: Thu, 20 Oct 2005 09:06:04 +0000 Subject: fixed potential race condition with domark(); improved debug output --- NEWS | 4 ++++ syslogd.c | 82 ++++++++++++++++++++++++++++++++++++++++++--------------------- 2 files changed, 59 insertions(+), 27 deletions(-) diff --git a/NEWS b/NEWS index a39bd0b6..7d15e282 100644 --- a/NEWS +++ b/NEWS @@ -1,5 +1,9 @@ --------------------------------------------------------------------------- Version 1.11.2 (RGer), 2005-10-20 +- fixed a potential race condition. In the original code, marking was done + by an alarm handler, which could lead to all sorts of bad things. This + has been changed now. See comments in syslogd.c/domark() for details. +- improved debug output for property-based filters --------------------------------------------------------------------------- Version 1.11.1 (RGer), 2005-10-19 - support for BSD-style program name and host blocks diff --git a/syslogd.c b/syslogd.c index dd77166d..d36549a9 100644 --- a/syslogd.c +++ b/syslogd.c @@ -297,18 +297,19 @@ static int should_use_so_bsdcompat(void) #define SO_BSDCOMPAT 0 #endif -char *ConfFile = _PATH_LOGCONF; -char *PidFile = _PATH_LOGPID; -char ctty[] = _PATH_CONSOLE; +static char *ConfFile = _PATH_LOGCONF; +static char *PidFile = _PATH_LOGPID; +static char ctty[] = _PATH_CONSOLE; -char **parts; +static char **parts; -int inetm = 0; -pid_t myPid; /* our pid for use in self-generated messages, e.g. on startup */ +static int inetm = 0; +static pid_t myPid; /* our pid for use in self-generated messages, e.g. on startup */ static int debugging_on = 0; static int nlogs = -1; static int restart = 0; +static int bRequestDoMark = 0; /* do mark processing? */ #define MAXFUNIX 20 int nfunix = 1; @@ -761,7 +762,8 @@ static void endtty(); static void wallmsg(register struct filed *f); static void reapchild(); static const char *cvthname(struct sockaddr_in *f); -static void domark(); +static void domark(void); +static void domarkAlarmHdlr(); static void debug_switch(); static void logerror(char *type); static void logerrorInt(char *type, int errCode); @@ -2894,7 +2896,7 @@ int main(int argc, char **argv) (void) signal(SIGINT, Debug ? die : SIG_IGN); (void) signal(SIGQUIT, Debug ? die : SIG_IGN); (void) signal(SIGCHLD, reapchild); - (void) signal(SIGALRM, domark); + (void) signal(SIGALRM, domarkAlarmHdlr); (void) signal(SIGUSR1, Debug ? debug_switch : SIG_IGN); (void) signal(SIGPIPE, SIG_IGN); (void) signal(SIGXFSZ, SIG_IGN); /* do not abort if 2gig file limit is hit */ @@ -3030,8 +3032,14 @@ int main(int argc, char **argv) nfds = select(maxfds+1, (fd_set *) &readfds, (fd_set *) NULL, (fd_set *) NULL, (struct timeval *) NULL); #endif - if ( restart ) - { + if(bRequestDoMark) { + domark(); + bRequestDoMark = 0; + /* We do not use continue, because domark() is carried out + * only when something else happened. + */ + } + if (restart) { dprintf("\nReceived SIGHUP, reloading rsyslogd.\n"); init(); restart = 0; @@ -3751,13 +3759,21 @@ int shouldProcessThisMessage(struct filed *f, struct msg *pMsg) free(pszPropVal); if(Debug) { - printf("Property filter '%s' ", rsCStrGetSzStr(f->f_filterData.prop.pCSPropName)); + char *pszPropVal; + unsigned short pbMustBeFreed; + pszPropVal = MsgGetProp(pMsg, NULL, + f->f_filterData.prop.pCSPropName, &pbMustBeFreed); + printf("Filter: check for property '%s' (value '%s') ", + rsCStrGetSzStr(f->f_filterData.prop.pCSPropName), + pszPropVal); if(f->f_filterData.prop.isNegated) printf("NOT "); - printf("%s '%s' does %smatch.\n", + printf("%s '%s': %s\n", getFIOPName(f->f_filterData.prop.operation), rsCStrGetSzStr(f->f_filterData.prop.pCSCompValue), - iRet ? "" : "not "); + iRet ? "TRUE" : "FALSE"); + if(pbMustBeFreed) + free(pszPropVal); } } @@ -4909,18 +4925,22 @@ static const char *cvthname(struct sockaddr_in *f) return (hp->h_name); } -/* This method is called by an alarm handler. As such, we can have potentially - * race-conditons. It might be a very good idea to change this to real threading, - * for now we "just" need to be very careful about potential side-effects. For an - * example (and explanation), see: + +/* This method writes mark messages and - some time later - flushes reapeat + * messages (not doing this currently). + * This method was initially called by an alarm handler. As such, it could potentially + * have race-conditons. For details, see * http://lkml.org/lkml/2005/3/26/37 * http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=301511 - * TODO: I assume there are actually some bugs here, we are just lucky that - * the interval is so seldom that they (usually) do not manifest. It would - * probably be a good idea to redisign this whole thing... - * rgerhards, 2005-10-19 + * I have now changed it so that the alarm handler only sets a global variable, telling + * the main thread that it must do mark processing. So domark() is now called from the + * main thread itself, which is the only thing to make sure rsyslogd will not do + * strange things. The way it originally was seemed to work because mark occurs very + * seldom. However, the code called was anything else but reentrant, so it was like + * russian roulette. + * rgerhards, 2005-10-20 */ -void domark() +static void domark(void) { register struct filed *f; if (MarkInterval > 0) { @@ -4941,12 +4961,22 @@ void domark() } } } - (void) signal(SIGALRM, domark); +} + + +/* This is the alarm handler setting the global variable for + * domark request. See domark() comments for further details. + * rgerhards, 2005-10-20 + */ +static void domarkAlarmHdlr() +{ + bRequestDoMark = 1; /* request alarm */ + (void) signal(SIGALRM, domarkAlarmHdlr); (void) alarm(TIMERINTVL); } -void debug_switch() +static void debug_switch() { dprintf("Switching debugging_on to %s\n", (debugging_on == 0) ? "true" : "false"); debugging_on = (debugging_on == 0) ? 1 : 0; @@ -4987,8 +5017,7 @@ static void logerrorInt(char *type, int errCode) /* * Print syslogd errors some place. */ -static void logerror(type) - char *type; +static void logerror(char *type) { char buf[1024]; @@ -5157,7 +5186,6 @@ static rsRetVal addAllowedSenderLine(char* pName, char** ppRestOfConfLine) return RS_RET_ERR; } -printf("addAllow..., name '%s', line: '%s'\n", pName, *ppRestOfConfLine); /* OK, we now know the protocol and have valid list pointers. * So let's process the entries. We are using the parse class * for this. -- cgit