Bug 1268 - SIGSEGV after log_reopen
SIGSEGV after log_reopen
Status: RESOLVED FIXED
Product: unbound
Classification: Unclassified
Component: server
1.6.2
x86_64 Linux
: P5 normal
Assigned To: unbound team
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2017-05-20 01:04 CEST by samozrejmost
Modified: 2017-05-23 23:37 CEST (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description samozrejmost 2017-05-20 01:04:23 CEST
This only happens in rare situations, but I'm reporting it anyway, as this crashes the server.

How to reproduce:
 - start the unbound daemon
 - change the ownership of the logfile so that the unbound process cannot write to it
 - issue 'unbound-control log_reopen' command
 - do something which would make the unbound process write to the logfile
 - SIGSEGV

What *I think* happens is that after the 'unbound-control log_reopen' command the code runs the log_init() function where:

	if(logfile && logfile != stderr)
		fclose(logfile);

'logfile' is then undefined, but not NULL. Then in the same function later:

	f = fopen(filename, "a");
	if(!f) {
		lock_quick_unlock(&log_lock);
		log_err("Could not open logfile %s: %s", filename, 
			strerror(errno));
		return;
	}

fopen() returns NULL (Permission denied), so then f=NULL, logfile is undefined. Any subsequent writes to the logfile (fprint(logfile,...)) then follow the undefined pointer 'logfile'.

Hope this helps!

PS: I came across this when I made a logrotate script which didn't handle the write rights properly.
Comment 1 Wouter Wijngaards 2017-05-22 09:24:04 CEST
Hi Samozremost,

Thanks for the report!  I tried to fix this by moving an =NULL before the fclose() event happens.  So that the other function has no issues, and encounters a NULL straight away.


Index: util/log.c
===================================================================
--- util/log.c	(revision 4176)
+++ util/log.c	(working copy)
@@ -103,8 +103,12 @@
 			use_syslog?"syslog":(filename&&filename[0]?filename:"stderr"));
 		lock_quick_lock(&log_lock);
 	}
-	if(logfile && logfile != stderr)
-		fclose(logfile);
+	if(logfile && logfile != stderr) {
+		FILE* cl = logfile;
+		logfile = NULL; /* set to NULL before it is closed, so that
+			other threads have a valid logfile or NULL */
+		fclose(cl);
+	}
 #ifdef HAVE_SYSLOG_H
 	if(logging_to_syslog) {
 		closelog();
Comment 2 samozrejmost 2017-05-23 23:37:55 CEST
I applied the patch and it works for me (didn't test for more than 1 thread).

Thank you for your work!

martin