Gitweb:
http://git.exim.org/exim.git/commitdiff/921b12ca0c361b9c543368edf057712afa02ca14
Commit: 921b12ca0c361b9c543368edf057712afa02ca14
Parent: 0ca0cf52fa9c635984937a3cc813d38fcdacd7ab
Author: Tony Finch <dot@???>
AuthorDate: Tue Jun 7 16:48:44 2011 +0100
Committer: Tony Finch <dot@???>
CommitDate: Tue Jun 7 20:03:58 2011 +0100
exiwhat: Ensure the SIGUSR1 signal handler is safe.
exiwhat sends a SIGUSR1 to all exim processes to make them write
their status to the process log. This is all done in the signal
handler, but the logging code makes a number of calls that are not
signal safe. These can all cause crashes or recursive locking in
libc.
Firstly, obtaining and formatting the timestamp is not safe.
Doing so is unnecessary since exiwhat strips off the timestamp.
This change removes timestamps from the process log.
Secondly, exim closes all the logs after writing the process
log. Closing syslog is not signal safe, and isn't necessary.
We now only close the process log after writing to it.
Thirdly, exim may calculate the process_log_path inside the signal
handler which involves some possibly-unsafe string handling code.
This change calculates the path when reading the configuration.
Fourthly, when exim creates the process log file it might have to
call the unsafe directory_create() though this is unlikely in
practice. After this change exim only calls log_create() in a
subprocess which is safe - it sometimes needs to do so anyway, if
it is running as root and needs to drop privileges.
The new code has no process log handling in log.c which eliminates
some awkward special cases. It uses very simple code to write to
the file in the signal handler, so it is obviously safe by inspection.
---
doc/doc-txt/ChangeLog | 15 +++
src/src/exim.c | 93 ++++++++++++-------
src/src/exiwhat.src | 2 +-
src/src/functions.h | 3 +-
src/src/globals.c | 1 +
src/src/globals.h | 1 +
src/src/log.c | 244 ++++++++++++++++++++++---------------------------
src/src/macros.h | 1 -
src/src/readconf.c | 5 +
9 files changed, 195 insertions(+), 170 deletions(-)
diff --git a/doc/doc-txt/ChangeLog b/doc/doc-txt/ChangeLog
index 8ca5d85..3af14c3 100644
--- a/doc/doc-txt/ChangeLog
+++ b/doc/doc-txt/ChangeLog
@@ -17,6 +17,21 @@ TK/01 DKIM Verification: Fix relaxed canon for empty headers w/o
TF/02 Fix a couple more cases where we did not log the error message
when unlink() failed. See also change 4.74-TF/03.
+TF/03 Make the exiwhat support code safe for signals. Previously Exim might
+ lock up or crash if it happened to be inside a call to libc when it
+ got a SIGUSR1 from exiwhat.
+
+ The SIGUSR1 handler appends the current process status to the process
+ log which is later printed by exiwhat. It used to use the general
+ purpose logging code to do this, but several functions it calls are
+ not safe for signals.
+
+ The new output code in the SIGUSR1 handler is specific to the process
+ log, and simple enough that it's easy to inspect for signal safety.
+ Removing some special cases also simplifies the general logging code.
+ Removing the spurious timestamps from the process log simplifies
+ exiwhat.
+
Exim version 4.76
-----------------
diff --git a/src/src/exim.c b/src/src/exim.c
index 371bc10..3455556 100644
--- a/src/src/exim.c
+++ b/src/src/exim.c
@@ -141,6 +141,38 @@ return yield;
/*************************************************
+* Set up processing details *
+*************************************************/
+
+/* Save a text string for dumping when SIGUSR1 is received.
+Do checks for overruns.
+
+Arguments: format and arguments, as for printf()
+Returns: nothing
+*/
+
+void
+set_process_info(const char *format, ...)
+{
+int len;
+va_list ap;
+sprintf(CS process_info, "%5d ", (int)getpid());
+len = Ustrlen(process_info);
+va_start(ap, format);
+if (!string_vformat(process_info + len, PROCESS_INFO_SIZE - len - 2, format, ap))
+ Ustrcpy(process_info + len, "**** string overflowed buffer ****");
+len = Ustrlen(process_info);
+process_info[len+0] = '\n';
+process_info[len+1] = '\0';
+process_info_len = len + 1;
+DEBUG(D_process_info) debug_printf("set_process_info: %s", process_info);
+va_end(ap);
+}
+
+
+
+
+/*************************************************
* Handler for SIGUSR1 *
*************************************************/
@@ -149,6 +181,8 @@ what it is currently doing. It will only be used if the OS is capable of
setting up a handler that causes automatic restarting of any system call
that is in progress at the time.
+This function takes care to be signal-safe.
+
Argument: the signal number (SIGUSR1)
Returns: nothing
*/
@@ -156,10 +190,32 @@ Returns: nothing
static void
usr1_handler(int sig)
{
-sig = sig; /* Keep picky compilers happy */
-log_write(0, LOG_PROCESS, "%s", process_info);
-log_close_all();
-os_restarting_signal(SIGUSR1, usr1_handler);
+int fd;
+
+os_restarting_signal(sig, usr1_handler);
+
+fd = Uopen(process_log_path, O_APPEND|O_WRONLY, LOG_MODE);
+if (fd < 0)
+ {
+ /* If we are already running as the Exim user, try to create it in the
+ current process (assuming spool_directory exists). Otherwise, if we are
+ root, do the creation in an exim:exim subprocess. */
+
+ int euid = geteuid();
+ if (euid == exim_uid)
+ fd = Uopen(process_log_path, O_CREAT|O_APPEND|O_WRONLY, LOG_MODE);
+ else if (euid == root_uid)
+ fd = log_create_as_exim(process_log_path);
+ }
+
+/* If we are neither exim nor root, or if we failed to create the log file,
+give up. There is not much useful we can do with errors, since we don't want
+to disrupt whatever is going on outside the signal handler. */
+
+if (fd < 0) return;
+
+(void)write(fd, process_info, process_info_len);
+(void)close(fd);
}
@@ -349,35 +405,6 @@ if (exim_tvcmp(&now_tv, then_tv) <= 0)
/*************************************************
-* Set up processing details *
-*************************************************/
-
-/* Save a text string for dumping when SIGUSR1 is received.
-Do checks for overruns.
-
-Arguments: format and arguments, as for printf()
-Returns: nothing
-*/
-
-void
-set_process_info(const char *format, ...)
-{
-int len;
-va_list ap;
-sprintf(CS process_info, "%5d ", (int)getpid());
-len = Ustrlen(process_info);
-va_start(ap, format);
-if (!string_vformat(process_info + len, PROCESS_INFO_SIZE - len, format, ap))
- Ustrcpy(process_info + len, "**** string overflowed buffer ****");
-DEBUG(D_process_info) debug_printf("set_process_info: %s\n", process_info);
-va_end(ap);
-}
-
-
-
-
-
-/*************************************************
* Call fopen() with umask 777 and adjust mode *
*************************************************/
diff --git a/src/src/exiwhat.src b/src/src/exiwhat.src
index 5f81507..e53a46e 100644
--- a/src/src/exiwhat.src
+++ b/src/src/exiwhat.src
@@ -130,7 +130,7 @@ fi
sleep 1
if [ ! -s ${log} ] ; then echo "No exim process data" ;
- else sed 's/^[0-9-]* [0-9:]* \([+-][0-9]* \)*\(\[[0-9]\+\] \)\?//' ${log} | sort -n | uniq ; fi
+ else sort -nu ${log} ; fi
# End of exiwhat
diff --git a/src/src/functions.h b/src/src/functions.h
index 079e8ad..9e06a57 100644
--- a/src/src/functions.h
+++ b/src/src/functions.h
@@ -151,7 +151,8 @@ extern void ip_keepalive(int, uschar *, BOOL);
extern int ip_recv(int, uschar *, int, int);
extern int ip_socket(int, int);
-extern uschar *local_part_quote(uschar *lpart);
+extern uschar *local_part_quote(uschar *);
+extern int log_create_as_exim(uschar *);
extern void log_close_all(void);
#ifdef WITH_CONTENT_SCAN
diff --git a/src/src/globals.c b/src/src/globals.c
index c7e6c20..e29b544 100644
--- a/src/src/globals.c
+++ b/src/src/globals.c
@@ -832,6 +832,7 @@ BOOL preserve_message_logs = FALSE;
uschar *primary_hostname = NULL;
BOOL print_topbitchars = FALSE;
uschar process_info[PROCESS_INFO_SIZE];
+int process_info_len = 0;
uschar *process_log_path = NULL;
BOOL prod_requires_admin = TRUE;
uschar *prvscheck_address = NULL;
diff --git a/src/src/globals.h b/src/src/globals.h
index 3a1e537..c403b8b 100644
--- a/src/src/globals.h
+++ b/src/src/globals.h
@@ -540,6 +540,7 @@ extern BOOL preserve_message_logs; /* Save msglog files */
extern uschar *primary_hostname; /* Primary name of this computer */
extern BOOL print_topbitchars; /* Topbit chars are printing chars */
extern uschar process_info[]; /* For SIGUSR1 output */
+extern int process_info_len;
extern uschar *process_log_path; /* Alternate path */
extern BOOL prod_requires_admin; /* TRUE if prodding requires admin */
extern uschar *prvscheck_address; /* Set during prvscheck expansion item */
diff --git a/src/src/log.c b/src/src/log.c
index a2c1d44..231db65 100644
--- a/src/src/log.c
+++ b/src/src/log.c
@@ -19,9 +19,9 @@ log files was originally contributed by Tony Sheen. */
#define LOG_MODE_FILE 1
#define LOG_MODE_SYSLOG 2
-enum { lt_main, lt_reject, lt_panic, lt_debug, lt_process };
+enum { lt_main, lt_reject, lt_panic, lt_debug };
-static uschar *log_names[] = { US"main", US"reject", US"panic", US"debug", US"process" };
+static uschar *log_names[] = { US"main", US"reject", US"panic", US"debug" };
@@ -182,7 +182,7 @@ Returns: a file descriptor, or < 0 on failure (errno set)
*/
static int
-create_log(uschar *name)
+log_create(uschar *name)
{
int fd = Uopen(name, O_CREAT|O_APPEND|O_WRONLY, LOG_MODE);
@@ -206,15 +206,66 @@ return fd;
+/*************************************************
+* Create a log file as the exim user *
+*************************************************/
+
+/* This function is called when we are root to spawn an exim:exim subprocess
+in which we can create a log file. It must be signal-safe since it is called
+by the usr1_handler().
+
+Arguments:
+ name the file name
+
+Returns: a file descriptor, or < 0 on failure (errno set)
+*/
+
+int
+log_create_as_exim(uschar *name)
+{
+pid_t pid = fork();
+int status = 1;
+int fd = -1;
+
+/* In the subprocess, change uid/gid and do the creation. Return 0 from the
+subprocess on success. If we don't check for setuid failures, then the file
+can be created as root, so vulnerabilities which cause setuid to fail mean
+that the Exim user can use symlinks to cause a file to be opened/created as
+root. We always open for append, so can't nuke existing content but it would
+still be Rather Bad. */
+
+if (pid == 0)
+ {
+ if (setgid(exim_gid) < 0)
+ die(US"exim: setgid for log-file creation failed, aborting",
+ US"Unexpected log failure, please try later");
+ if (setuid(exim_uid) < 0)
+ die(US"exim: setuid for log-file creation failed, aborting",
+ US"Unexpected log failure, please try later");
+ _exit((log_create(name) < 0)? 1 : 0);
+ }
+
+/* If we created a subprocess, wait for it. If it succeeded, try the open. */
+
+while (pid > 0 && waitpid(pid, &status, 0) != pid);
+if (status == 0) fd = Uopen(name, O_APPEND|O_WRONLY, LOG_MODE);
+
+/* If we failed to create a subprocess, we are in a bad way. We return
+with fd still < 0, and errno set, letting the caller handle the error. */
+
+return fd;
+}
+
+
+
/*************************************************
* Open a log file *
*************************************************/
-/* This function opens one of a number of logs, which all (except for the
-"process log") reside in the same directory, creating the directory if it does
-not exist. This may be called recursively on failure, in order to open the
-panic log.
+/* This function opens one of a number of logs, creating the log directory if
+it does not exist. This may be called recursively on failure, in order to open
+the panic log.
The directory is in the static variable file_path. This is static so that it
the work of sorting out the path is done just once per Exim process.
@@ -227,7 +278,7 @@ avoid races.
Arguments:
fd where to return the resulting file descriptor
- type lt_main, lt_reject, lt_panic, lt_debug or lt_process
+ type lt_main, lt_reject, lt_panic, or lt_debug
tag optional tag to include in the name (only hooked up for debug)
Returns: nothing
@@ -240,21 +291,8 @@ uid_t euid;
BOOL ok, ok2;
uschar buffer[LOG_NAME_SIZE];
-/* Sort out the file name. This depends on the type of log we are opening. The
-process "log" is written in the spool directory by default, but a path name can
-be specified in the configuration. */
-
-if (type == lt_process)
- {
- if (process_log_path == NULL)
- ok = string_format(buffer, sizeof(buffer), "%s/exim-process.info",
- spool_directory);
- else
- ok = string_format(buffer, sizeof(buffer), "%s", process_log_path);
- }
-
-/* The names of the other three logs are controlled by file_path. The panic log
-is written to the same directory as the main and reject logs, but its name does
+/* The names of the log files are controlled by file_path. The panic log is
+written to the same directory as the main and reject logs, but its name does
not have a datestamp. The use of datestamps is indicated by %D/%M in file_path.
When opening the panic log, if %D or %M is present, we remove the datestamp
from the generated name; if it is at the start, remove a following
@@ -262,66 +300,63 @@ non-alphanumeric character as well; otherwise, remove a preceding
non-alphanumeric character. This is definitely kludgy, but it sort of does what
people want, I hope. */
-else
+ok = string_format(buffer, sizeof(buffer), CS file_path, log_names[type]);
+
+/* Save the name of the mainlog for rollover processing. Without a datestamp,
+it gets statted to see if it has been cycled. With a datestamp, the datestamp
+will be compared. The static slot for saving it is the same size as buffer,
+and the text has been checked above to fit, so this use of strcpy() is OK. */
+
+if (type == lt_main)
{
- ok = string_format(buffer, sizeof(buffer), CS file_path, log_names[type]);
+ Ustrcpy(mainlog_name, buffer);
+ mainlog_datestamp = mainlog_name + string_datestamp_offset;
+ }
- /* Save the name of the mainlog for rollover processing. Without a datestamp,
- it gets statted to see if it has been cycled. With a datestamp, the datestamp
- will be compared. The static slot for saving it is the same size as buffer,
- and the text has been checked above to fit, so this use of strcpy() is OK. */
+/* Ditto for the reject log */
- if (type == lt_main)
- {
- Ustrcpy(mainlog_name, buffer);
- mainlog_datestamp = mainlog_name + string_datestamp_offset;
- }
+else if (type == lt_reject)
+ {
+ Ustrcpy(rejectlog_name, buffer);
+ rejectlog_datestamp = rejectlog_name + string_datestamp_offset;
+ }
- /* Ditto for the reject log */
+/* and deal with the debug log (which keeps the datestamp, but does not
+update it) */
- else if (type == lt_reject)
+else if (type == lt_debug)
+ {
+ Ustrcpy(debuglog_name, buffer);
+ if (tag)
{
- Ustrcpy(rejectlog_name, buffer);
- rejectlog_datestamp = rejectlog_name + string_datestamp_offset;
+ /* this won't change the offset of the datestamp */
+ ok2 = string_format(buffer, sizeof(buffer), "%s%s",
+ debuglog_name, tag);
+ if (ok2)
+ Ustrcpy(debuglog_name, buffer);
}
+ }
- /* and deal with the debug log (which keeps the datestamp, but does not
- update it) */
+/* Remove any datestamp if this is the panic log. This is rare, so there's no
+need to optimize getting the datestamp length. We remove one non-alphanumeric
+char afterwards if at the start, otherwise one before. */
- else if (type == lt_debug)
+else if (string_datestamp_offset >= 0)
+ {
+ uschar *from = buffer + string_datestamp_offset;
+ uschar *to = from + string_datestamp_length;
+ if (from == buffer || from[-1] == '/')
{
- Ustrcpy(debuglog_name, buffer);
- if (tag)
- {
- /* this won't change the offset of the datestamp */
- ok2 = string_format(buffer, sizeof(buffer), "%s%s",
- debuglog_name, tag);
- if (ok2)
- Ustrcpy(debuglog_name, buffer);
- }
+ if (!isalnum(*to)) to++;
}
-
- /* Remove any datestamp if this is the panic log. This is rare, so there's no
- need to optimize getting the datestamp length. We remove one non-alphanumeric
- char afterwards if at the start, otherwise one before. */
-
- else if (string_datestamp_offset >= 0)
+ else
{
- uschar *from = buffer + string_datestamp_offset;
- uschar *to = from + string_datestamp_length;
- if (from == buffer || from[-1] == '/')
- {
- if (!isalnum(*to)) to++;
- }
- else
- {
- if (!isalnum(from[-1])) from--;
- }
+ if (!isalnum(from[-1])) from--;
+ }
- /* This strcpy is ok, because we know that to is a substring of from. */
+ /* This strcpy is ok, because we know that to is a substring of from. */
- Ustrcpy(from, to);
- }
+ Ustrcpy(from, to);
}
/* If the file name is too long, it is an unrecoverable disaster */
@@ -355,47 +390,12 @@ euid = geteuid();
/* If we are already running as the Exim user (even if that user is root),
we can go ahead and create in the current process. */
-if (euid == exim_uid) *fd = create_log(buffer);
+if (euid == exim_uid) *fd = log_create(buffer);
/* Otherwise, if we are root, do the creation in an exim:exim subprocess. If we
are neither exim nor root, creation is not attempted. */
-else if (euid == root_uid)
- {
- int status, rv;
- pid_t pid = fork();
-
- /* In the subprocess, change uid/gid and do the creation. Return 0 from the
- subprocess on success. If we don't check for setuid failures, then the file
- can be created as root, so vulnerabilities which cause setuid to fail mean
- that the Exim user can use symlinks to cause a file to be opened/created as
- root. We always open for append, so can't nuke existing content but it would
- still be Rather Bad. */
-
- if (pid == 0)
- {
- rv = setgid(exim_gid);
- if (rv)
- die(US"exim: setgid for log-file creation failed, aborting",
- US"Unexpected log failure, please try later");
- rv = setuid(exim_uid);
- if (rv)
- die(US"exim: setuid for log-file creation failed, aborting",
- US"Unexpected log failure, please try later");
- _exit((create_log(buffer) < 0)? 1 : 0);
- }
-
- /* If we created a subprocess, wait for it. If it succeeded retry the open. */
-
- if (pid > 0)
- {
- while (waitpid(pid, &status, 0) != pid);
- if (status == 0) *fd = Uopen(buffer, O_APPEND|O_WRONLY, LOG_MODE);
- }
-
- /* If we failed to create a subprocess, we are in a bad way. We fall through
- with *fd still < 0, and errno set, letting the code below handle the error. */
- }
+else if (euid == root_uid) *fd = log_create_as_exim(buffer);
/* If we now have an open file, set the close-on-exec flag and return. */
@@ -523,10 +523,6 @@ recognized:
log_file_path = "syslog" write to syslog
log_file_path = "syslog : xxx" write to syslog and to files (any order)
-The one exception to this is messages containing LOG_PROCESS. These are always
-written to exim-process.info in the spool directory. They aren't really log
-messages in the same sense as the others.
-
The message always gets '\n' added on the end of it, since more than one
process may be writing to the log at once and we don't want intermingling to
happen in the middle of lines. To be absolutely sure of this we write the data
@@ -565,7 +561,6 @@ Arguments:
LOG_REJECT write to reject log or syslog LOG_NOTICE
LOG_PANIC write to panic log or syslog LOG_ALERT
LOG_PANIC_DIE write to panic log or LOG_ALERT and then crash
- LOG_PROCESS write to process log (always a file)
format a printf() format
... arguments for format
@@ -724,7 +719,6 @@ DEBUG(D_any|D_v)
((flags & LOG_MAIN) != 0)? " MAIN" : "",
((flags & LOG_PANIC) != 0)? " PANIC" : "",
((flags & LOG_PANIC_DIE) == LOG_PANIC_DIE)? " DIE" : "",
- ((flags & LOG_PROCESS) != 0)? " PROCESS": "",
((flags & LOG_REJECT) != 0)? " REJECT" : "");
while(*ptr) ptr++;
@@ -742,7 +736,7 @@ DEBUG(D_any|D_v)
/* If no log file is specified, we are in a mess. */
-if ((flags & (LOG_MAIN|LOG_PANIC|LOG_REJECT|LOG_PROCESS)) == 0)
+if ((flags & (LOG_MAIN|LOG_PANIC|LOG_REJECT)) == 0)
log_write(0, LOG_MAIN|LOG_PANIC_DIE, "log_write called with no log "
"flags set");
@@ -758,8 +752,8 @@ if (disable_logging)
if (!write_rejectlog) flags &= ~LOG_REJECT;
-/* Create the main message in the log buffer, including the message
-id except for the process log and when called by a utility. */
+/* Create the main message in the log buffer. Do not include the message id
+when called by a utility. */
ptr = log_buffer;
sprintf(CS ptr, "%s ", tod_stamp(tod_log));
@@ -771,7 +765,7 @@ if ((log_extra_selector & LX_pid) != 0)
while (*ptr) ptr++;
}
-if (really_exim && (flags & LOG_PROCESS) == 0 && message_id[0] != 0)
+if (really_exim && message_id[0] != 0)
{
sprintf(CS ptr, "%s ", message_id);
while(*ptr) ptr++;
@@ -1025,24 +1019,6 @@ if ((flags & LOG_REJECT) != 0)
}
-/* Handle the process log file, where exim processes can be made to dump
-details of what they are doing by sending them a USR1 signal. Note that
-a message id is not automatically added above. This information is always
-written to a file - never to syslog. */
-
-if ((flags & LOG_PROCESS) != 0)
- {
- int processlogfd;
- open_log(&processlogfd, lt_process, NULL); /* No return on error */
- if ((rc = write(processlogfd, log_buffer, length)) != length)
- {
- log_write_failed(US"process log", length, rc);
- /* That function does not return */
- }
- (void)close(processlogfd);
- }
-
-
/* Handle the panic log, which is not kept open like the others. If it fails to
open, there will be a recursive call to log_write(). We detect this above and
attempt to write to the system log as a last-ditch try at telling somebody. In
diff --git a/src/src/macros.h b/src/src/macros.h
index 610221f..e3129c6 100644
--- a/src/src/macros.h
+++ b/src/src/macros.h
@@ -700,7 +700,6 @@ local_scan.h */
#define LOG_MAIN 1 /* Write to the main log */
#define LOG_PANIC 2 /* Write to the panic log */
#define LOG_PANIC_DIE 6 /* Write to the panic log and then die */
-#define LOG_PROCESS 8 /* Write to the process log */
#define LOG_REJECT 16 /* Write to the reject log, with headers */
#define LOG_SENDER 32 /* Add raw sender to the message */
#define LOG_RECIPIENTS 64 /* Add raw recipients to the message */
diff --git a/src/src/readconf.c b/src/src/readconf.c
index 7aa44cf..4e85a87 100644
--- a/src/src/readconf.c
+++ b/src/src/readconf.c
@@ -3113,6 +3113,11 @@ if (*pid_file_path != 0)
pid_file_path = s;
}
+/* Set default value of process_log_path */
+
+if (process_log_path == NULL || *process_log_path =='\0')
+ process_log_path = string_sprintf("%s/exim-process.info", spool_directory);
+
/* Compile the regex for matching a UUCP-style "From_" line in an incoming
message. */
