Re: Fix for ulogd2-SIGHUP crash (and a few questions)

[Pablo Neira Ayuso <pablo@netfilter.org>]

[-- Attachment #1: Type: text/plain, Size: 1993 bytes --]

Hi Hugo,

I've been spining around my email box today and I found this. Please,
next time cc: netfilter-devel.

Hugo Mildenberger wrote:
> Pablo,
> 
> thanks for checking in timer.h.  Attached you'll find a 
> patch which fixes the logfile-reopen problem after 
> sending a SIGHUP to ulogd. The reason was that 
> config_parse_file() stuffed a stack-allocated string 
> named 'args' (stemming from wordbuf) to logfile_open  
> which in turn got assigned to a global variable 
> named 'ulogd_logfile'. 

I have applied the following patch based on yours, the credits are
stored in git changelog so IMO there's no need to keep it in the C file.

> There is reason to  assume that there are more issues 
> like this during module configuration. Currently I have 
> not yet understood which modules might be affected.
> 
> 
> Some other issues:
> 
> 1.) Perhaps using inotify instead of (or in addition to) 
> SIGUSR1?

I don't think that we need such thing, I prefer that the sysadmin have
to explicitly order a reload of the configuration file.

> 2.) grepping through kernel modules, I found in 
> nfnetlink_log.c:470 that beneath socket's fuid also 
> fgid is transmitted, while ulogd_inppkt_NFLOG.c
> and printpkt.c only want to know about uid.

I think that Eric has fixed this a one of his patchsets.

> 3.) For some reasons, I would be happy to see also the 
> pid of the socket owning process appearing at least in 
> a mysql table. I know that the somehow related "owner 
> match"  is marked "broken on SMP", but the code for 
> non-smp machines is still in place. Could you just give 
> me a hint, what the problem on smp regarding pid/owner 
> match really is, and if there are reason not to 
> transmit this information via nfnetlink_log.c.

See patch: "Remove tasklist_lock abuse in ipt{,6}owner" from Patrick
McHardy. The owner match in linux kernel <= 2.6.14 gets semaphore in a
BH which can produce a deadlock in SMP.

-- 
"Los honestos son inadaptados sociales" -- Les Luthiers


[-- Attachment #2: x --]
[-- Type: text/plain, Size: 2090 bytes --]

diff --git a/src/ulogd.c b/src/ulogd.c
index 3a1e3d9..8c8dc14 100644
--- a/src/ulogd.c
+++ b/src/ulogd.c
@@ -75,8 +75,8 @@
 
 /* global variables */
 static FILE *logfile = NULL;		/* logfile pointer */
-static char *ulogd_configfile = ULOGD_CONFIGFILE;
-static char *ulogd_logfile = ULOGD_LOGFILE_DEFAULT;
+static char *ulogd_logfile = NULL;
+static const char *ulogd_configfile = ULOGD_CONFIGFILE;
 static FILE syslog_dummy;
 
 static int info_mode = 0;
@@ -880,8 +880,10 @@ static void ulogd_main_loop(void)
 /* open the logfile */
 static int logfile_open(const char *name)
 {
-	if (name)
-		ulogd_logfile = name;
+	if (name) {
+	        free(ulogd_logfile);
+		ulogd_logfile = strdup(name);
+	}
 
 	if (!strcmp(name, "stdout")) {
 		logfile = stdout;
@@ -891,12 +893,12 @@ static int logfile_open(const char *name)
 	} else {
 		logfile = fopen(ulogd_logfile, "a");
 		if (!logfile) {
-			fprintf(stderr, "ERROR: can't open logfile %s: %s\n", 
+			fprintf(stderr, "ERROR: can't open logfile '%s': %s\n", 
 				name, strerror(errno));
 			exit(2);
 		}
 	}
-	ulogd_log(ULOGD_INFO, "ulogd Version %s starting\n", ULOGD_VERSION);
+	ulogd_log(ULOGD_INFO, "ulogd Version %s (re-)starting\n", ULOGD_VERSION);
 	return 0;
 }
 
@@ -959,8 +961,10 @@ static void sigterm_handler(int signal)
 
 	deliver_signal_pluginstances(signal);
 
-	if (logfile != stdout)
+	if (logfile != NULL  && logfile != stdout) {
 		fclose(logfile);
+		logfile = NULL;
+	}
 
 	exit(0);
 }
@@ -975,8 +979,13 @@ static void signal_handler(int signal)
 		if (logfile != stdout && logfile != &syslog_dummy) {
 			fclose(logfile);
 			logfile = fopen(ulogd_logfile, "a");
-			if (!logfile)
+ 			if (!logfile) {
+				fprintf(stderr, 
+					"ERROR: can't open logfile %s: %s\n", 
+					ulogd_logfile, strerror(errno));
 				sigterm_handler(signal);
+			}
+	
 		}
 		break;
 	default:
@@ -1021,6 +1030,7 @@ int main(int argc, char* argv[])
 	uid_t uid = 0;
 	gid_t gid = 0;
 
+	ulogd_logfile = strdup(ULOGD_LOGFILE_DEFAULT);
 
 	while ((argch = getopt_long(argc, argv, "c:dh::Vu:i:", opts, NULL)) != -1) {
 		switch (argch) {