Remove any process log handling from log.c. Use very simple code
to write to it in the signal handler, so it is obviously safe by
inspection.
Calculate the process_log_path eagerly, instead of trying to do so
inside the signal handler. This avoids possibly-unsafe string
handling code.
Be more careful about creating the process log file in a safe
way. This mainly avoids calling log_create() except in a subprocess,
since log_create might call the dangerous directory_create().
---
doc/doc-txt/ChangeLog | 7 ++
src/src/exim.c | 90 ++++++++++++------
src/src/functions.h | 3 +-
src/src/globals.c | 1 +
src/src/globals.h | 1 +
src/src/log.c | 251 ++++++++++++++++++++++---------------------------
src/src/macros.h | 1 -
src/src/readconf.c | 5 +
8 files changed, 188 insertions(+), 171 deletions(-)
diff --git a/doc/doc-txt/ChangeLog b/doc/doc-txt/ChangeLog
index ef82a0c..b0a68b4 100644
--- a/doc/doc-txt/ChangeLog
+++ b/doc/doc-txt/ChangeLog
@@ -14,6 +14,13 @@ TF/02 Log LMTP confirmation messages in the same way as SMTP,
TF/03 Include the error message when we fail to unlink a spool file.
+TF/04 Fix signal safety bugs in the implementation of exiwhat. It
+ sends a SIGUSR1 to all exim processes to make them write their
+ status to the process log. Exim writes to the log inside the
+ signal handler, and the code to implement this previously called
+ some library functions which are not safe for signal handlers.
+ Because of this exiwhat could make Exim lock up or crash.
+
Exim version 4.73
-----------------
diff --git a/src/src/exim.c b/src/src/exim.c
index 4c9867f..86cedc7 100644
--- a/src/src/exim.c
+++ b/src/src/exim.c
@@ -139,6 +139,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(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 *
*************************************************/
@@ -147,6 +179,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
*/
@@ -154,9 +188,33 @@ Returns: nothing
static void
usr1_handler(int sig)
{
-sig = sig; /* Keep picky compilers happy */
-log_write(0, LOG_PROCESS, "%s", process_info);
+int fd;
+
+sig = sig; /* Keep picky compilers happy */
os_restarting_signal(SIGUSR1, 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);
}
@@ -345,34 +403,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(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/functions.h b/src/src/functions.h
index 2408872..fda0b11 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 void 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 6653d62..ad2de06 100644
--- a/src/src/globals.c
+++ b/src/src/globals.c
@@ -824,6 +824,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 db7a79b..edbf858 100644
--- a/src/src/globals.h
+++ b/src/src/globals.h
@@ -526,6 +526,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 79ea4cc..c0076c6 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,59 @@ 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. There doesn't seem much point in testing for setgid
+and setuid errors. */
+
+if (pid == 0)
+ {
+ (void)setgid(exim_gid);
+ (void)setuid(exim_uid);
+ _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 +271,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,87 +284,71 @@ 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 in file_path.
When opening the panic log, if %D is present, we remove the datestamp from the
generated name; if it is at the start, remove a following non-alphameric
character as well; otherwise, remove a preceding non-alphameric 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 + Ustrlen(tod_stamp(tod_log_datestamp));
+ 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 + Ustrlen(tod_stamp(tod_log_datestamp));
- 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 */
@@ -354,38 +382,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;
- pid_t pid = fork();
-
- /* In the subprocess, change uid/gid and do the creation. Return 0 from the
- subprocess on success. There doesn't seem much point in testing for setgid
- and setuid errors. */
-
- if (pid == 0)
- {
- (void)setgid(exim_gid);
- (void)setuid(exim_uid);
- _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. */
@@ -513,10 +515,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
@@ -555,7 +553,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
@@ -714,7 +711,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++;
@@ -732,7 +728,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");
@@ -748,28 +744,23 @@ 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));
+while(*ptr) ptr++;
-/* Omit the timestamp in the process log because it is not signal-safe.*/
-if ((flags & LOG_PROCESS) == 0)
+if ((log_extra_selector & LX_pid) != 0)
{
- sprintf(CS ptr, "%s ", tod_stamp(tod_log));
- while(*ptr) ptr++;
-
- if ((log_extra_selector & LX_pid) != 0)
- {
- sprintf(CS ptr, "[%d] ", (int)getpid());
- while (*ptr) ptr++;
- }
+ sprintf(CS ptr, "[%d] ", (int)getpid());
+ while (*ptr) ptr++;
+ }
- if (really_exim && message_id[0] != 0)
- {
- sprintf(CS ptr, "%s ", message_id);
- while(*ptr) ptr++;
- }
+if (really_exim && message_id[0] != 0)
+ {
+ sprintf(CS ptr, "%s ", message_id);
+ while(*ptr) ptr++;
}
if ((flags & LOG_CONFIG) != 0) ptr = log_config_info(ptr, flags);
@@ -1020,24 +1011,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 3f24025..66ae68f 100644
--- a/src/src/macros.h
+++ b/src/src/macros.h
@@ -699,7 +699,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 118ccf5..542bc31 100644
--- a/src/src/readconf.c
+++ b/src/src/readconf.c
@@ -3105,6 +3105,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. */
--
1.7.3.GIT
