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.

9 files changed, 195 insertions, 170 deletions

diff --git a/doc/doc-txt/ChangeLog b/doc/doc-txt/ChangeLog
index 8ca5d85af..3af14c39e 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 371bc1025..345555602 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 5f8150786..e53a46edd 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 079e8ad14..9e06a5746 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 c7e6c2081..e29b544b5 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 3a1e53771..c403b8b16 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 a2c1d4416..231db6581 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 610221fd9..e3129c61f 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 7aa44cf8c..4e85a872e 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. */