exiwhat: Ensure the SIGUSR1 signal handler is safe.
authorTony Finch <dot@dotat.at>
Tue, 7 Jun 2011 15:48:44 +0000 (16:48 +0100)
committerTony Finch <dot@dotat.at>
Tue, 7 Jun 2011 19:03:58 +0000 (20:03 +0100)
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
src/src/exim.c
src/src/exiwhat.src
src/src/functions.h
src/src/globals.c
src/src/globals.h
src/src/log.c
src/src/macros.h
src/src/readconf.c

index 8ca5d85af54ef9d2b6b583daad387a5a87819e4a..3af14c39eaa018edf3bd4673c1ac8d16e5428f9c 100644 (file)
@@ -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
 -----------------
index 371bc1025c5c584292aa32bf0c6004c9f945ddbd..34555560259bd79f4b0b4524abf704a8224adf16 100644 (file)
@@ -140,6 +140,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);
 }
 
 
@@ -348,35 +404,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  *
 *************************************************/
index 5f81507863ff7c1401a38a9f9e597b1500584212..e53a46edddab6460dcd3884a33d6fef12a75472f 100644 (file)
@@ -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
index 079e8ad142278cf3fc9200e8582510fdedc5cef6..9e06a5746fc26d790ffd4bfcef45a5a5203e68b4 100644 (file)
@@ -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
index c7e6c2081cf6e8ce3a1a3894b8839af89f0391ce..e29b544b506e0fce7c81303aef0af9c1aa51176f 100644 (file)
@@ -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;
index 3a1e53771ea511191590929b18bf88312dc0f7e7..c403b8b16ededee3809b94c7266ed77141a35b63 100644 (file)
@@ -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 */
index a2c1d44169d7e8b21b40e521544c1cdb9559d14a..231db6581db9c95bb0e7018b378eec83753bca31 100644 (file)
@@ -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
index 610221fd96f01f52764939dfab242e02d0d91602..e3129c61fc36bec7d7ba0f92c5b37999a7091854 100644 (file)
@@ -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 */
index 7aa44cf8c6b6fcf1fde3911d01bda811c8a06002..4e85a872ed191754119240ad709a2eb92548ec69 100644 (file)
@@ -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. */