[Mimedefang] Privilege escalation via PID file manipulation

Dianne Skoll dfs at roaringpenguin.com
Thu Aug 31 16:42:54 EDT 2017


Hi,

This is a much more extensive patch, but I believe it does finally
close the hole if you keep your PID files in a root-owned directory.

Please test this; I plan on releasing 2.81 tomorrow.

Regards,

Dianne.


diff --git a/Changelog b/Changelog
index da1a867..b9f09fa 100644
--- a/Changelog
+++ b/Changelog
@@ -2,6 +2,18 @@ WARNING: Before upgrading MIMEDefang, please search this file for
 *** NOTE INCOMPATIBILITY ** to see if anything has changed that might
 affect your filter.
 
+2017-08-31 Dianne Skoll <dfs at roaringpenguin.com>
+
+	* Make mimedefang and mimedefang-multiplexor write their PID files
+	as root to avoid an unprivileged user tampering with the pidfiles.
+
+	*** NOTE INCOMPATIBILITY ***
+
+	You should move your PID files out of the MIMEDefang spool directory
+	and into a standard root-owned directory like /var/run.  Use the -o
+	option to create lock files in the spool directory.  The sample
+	init scripts have been updated to reflect this.
+
 2017-07-24 Dianne Skoll <dfs at roaringpenguin.com>
 
 	* MIMEDefang 2.80 RELEASED
diff --git a/examples/init-script.in b/examples/init-script.in
index 346efca..8da803f 100755
--- a/examples/init-script.in
+++ b/examples/init-script.in
@@ -10,8 +10,10 @@
 RETVAL=0
 prog='mimedefang'
 SPOOLDIR='@SPOOLDIR@'
-PID="$SPOOLDIR/$prog.pid"
-MXPID="$SPOOLDIR/$prog-multiplexor.pid"
+PID="/var/run/$prog.pid"
+MXPID="/var/run/$prog-multiplexor.pid"
+LOCK="$SPOOLDIR/$prog.lock"
+MXLOCK="$SPOOLDIR/$prog-multiplexor.lock"
 
 # These lines keep SpamAssassin happy.  Not needed if you
 # aren't using SpamAssassin.
@@ -229,7 +231,7 @@ start_it() {
     else
 	EMBEDFLAG=""
     fi
-    $PROGDIR/$prog-multiplexor -p $MXPID \
+    $PROGDIR/$prog-multiplexor -p $MXPID -o $MXLOCK \
 	$EMBEDFLAG \
 	`[ -n "$SPOOLDIR" ] && echo "-z $SPOOLDIR"` \
 	`[ -n "$FILTER" ] && echo "-f $FILTER"` \
@@ -269,7 +271,7 @@ start_it() {
     # Start mimedefang
     printf "%-60s" "Starting $prog: "
     rm -f $SOCKET > /dev/null 2>&1
-    $PROGDIR/$prog -P $PID -R $LOOPBACK_RESERVED_CONNECTIONS \
+    $PROGDIR/$prog -P $PID -o $LOCK -R $LOOPBACK_RESERVED_CONNECTIONS \
 	-m $MX_SOCKET \
 	`[ -n "$SPOOLDIR" ] && echo "-z $SPOOLDIR"` \
 	`[ -n "$MX_USER" ] && echo "-U $MX_USER"` \
diff --git a/mimedefang-multiplexor.8.in b/mimedefang-multiplexor.8.in
index 980b57d..3505e48 100644
--- a/mimedefang-multiplexor.8.in
+++ b/mimedefang-multiplexor.8.in
@@ -117,7 +117,18 @@ for the format of \fIsocket\fR.
 .TP
 .B \-p \fIfileName\fR
 Causes \fBmimedefang-multiplexor\fR to write its process-ID (after
-becoming a daemon) to the specified file.
+becoming a daemon) to the specified file.  The file will be owned
+by root.
+
+.TP
+.B \-o \fIfileName\fR
+Causes \fbmimedefang-multiplexor\fR to use \fIfileName\fR as a lock
+file to avoid multiple instances from running.  If you supply
+\fB\-p\fR but not \fB\-o\fR, then \fbmimedefang-multiplexor\fR
+constructs a lock file by appending ".lock" to the pid file.  However,
+this is less secure than having a root-owned pid file in a root-owned
+directory and a lock file writable by the user named by the \fB\-U\fR
+option.  (The lock file must be writable by the \fB\-U\fR user.)
 
 .TP
 .B \-f \fIfilter_path\fR
diff --git a/mimedefang-multiplexor.c b/mimedefang-multiplexor.c
index 13b77b9..2e44f12 100644
--- a/mimedefang-multiplexor.c
+++ b/mimedefang-multiplexor.c
@@ -56,6 +56,12 @@
 static void limit_mem_usage(unsigned long rss, unsigned long as);
 #endif
 
+static char *pidfile = NULL;
+static char *lockfile = NULL;
+
+/* Number of file descriptors to close when forking */
+#define CLOSEFDS 256
+
 /* Weird case, but hey... */
 #if defined(HAVE_WAIT3) && !defined(HAVE_SETRLIMIT)
 #include <sys/resource.h>
@@ -346,6 +352,7 @@ static int get_hourly_history_totals(int cmd, time_t now, int hours, int *total,
 
 #define NUM_FREE_SLAVES    (SlaveCount[STATE_IDLE] + SlaveCount[STATE_STOPPED])
 #define NUM_RUNNING_SLAVES (SlaveCount[STATE_IDLE] + SlaveCount[STATE_BUSY] + SlaveCount[STATE_KILLED])
+#define REPORT_FAILURE(msg) do { if (kidpipe[1] >= 0) { write(kidpipe[1], "E" msg, strlen(msg)+1); } else { fprintf(stderr, "%s\n", msg); } } while(0)
 
 /**********************************************************************
 * %FUNCTION: state_name
@@ -496,6 +503,7 @@ usage(void)
     fprintf(stderr, "  -v                -- Print version and exit\n");
     fprintf(stderr, "  -t filename       -- Log statistics to filename\n");
     fprintf(stderr, "  -p filename       -- Write process-ID in filename\n");
+    fprintf(stderr, "  -o file           -- Use specified file as a lock file\n");
     fprintf(stderr, "  -T                -- Log statistics to syslog\n");
     fprintf(stderr, "  -u                -- Flush stats file after each write\n");
     fprintf(stderr, "  -Z                -- Accept and process status updates from busy slaves\n");
@@ -565,10 +573,13 @@ main(int argc, char *argv[], char **env)
     int sock, unpriv_sock;
     int c;
     int n;
-    char *pidfile = NULL;
+    int pidfile_fd = -1;
+    int lockfile_fd = -1;
     char *user = NULL;
     char *options;
     int facility = LOG_MAIL;
+    int kidpipe[2] = {-1, -1};
+    char kidmsg[256];
 
     time_t now;
 
@@ -593,13 +604,13 @@ main(int argc, char *argv[], char **env)
     if (getuid() != geteuid()) {
 	fprintf(stderr, "ERROR: %s is NOT intended to run suid! Exiting.\n",
 		argv[0]);
-	exit(1);
+	exit(EXIT_FAILURE);
     }
 
     if (getgid() != getegid()) {
 	fprintf(stderr, "ERROR: %s is NOT intended to run sgid! Exiting.\n",
 		argv[0]);
-	exit(1);
+	exit(EXIT_FAILURE);
     }
 
     Settings.minSlaves     = 0;
@@ -636,9 +647,9 @@ main(int argc, char *argv[], char **env)
     Settings.debugSlaveScheduling = 0;
 
 #ifndef HAVE_SETRLIMIT
-    options = "GAa:Tt:um:x:y:r:i:b:c:s:hdlf:p:w:F:W:U:S:q:Q:I:DEO:X:Y:N:vZP:z:";
+    options = "GAa:Tt:um:x:y:r:i:b:c:s:hdlf:p:o:w:F:W:U:S:q:Q:I:DEO:X:Y:N:vZP:z:";
 #else
-    options = "GAa:Tt:um:x:y:r:i:b:c:s:hdlf:p:w:F:W:U:S:q:Q:L:R:M:I:DEO:X:Y:N:vZP:z:";
+    options = "GAa:Tt:um:x:y:r:i:b:c:s:hdlf:p:o:w:F:W:U:S:q:Q:L:R:M:I:DEO:X:Y:N:vZP:z:";
 #endif
     while((c = getopt(argc, argv, options)) != -1) {
 	switch(c) {
@@ -669,7 +680,7 @@ main(int argc, char *argv[], char **env)
 	    break;
 	case 'v':
 	    printf("mimedefang-multiplexor version %s\n", VERSION);
-	    exit(0);
+	    exit(EXIT_SUCCESS);
 
 	case 'E':
 	    #ifdef EMBED_PERL
@@ -830,6 +841,16 @@ main(int argc, char *argv[], char **env)
 		exit(EXIT_FAILURE);
 	    }
 	    break;
+	case 'o':
+	    /* Use this as our lock file */
+	    if (lockfile != NULL) free(lockfile);
+
+	    lockfile = strdup(optarg);
+	    if (!lockfile) {
+		fprintf(stderr, "%s: Out of memory\n", argv[0]);
+		exit(EXIT_FAILURE);
+	    }
+	    break;
 	case 'f':
 	    /* Filter program */
 	    if (optarg[0] != '/') {
@@ -919,6 +940,17 @@ main(int argc, char *argv[], char **env)
 	    strcat((char *) Settings.sockName, "/mimedefang-multiplexor.sock");
     }
 
+    /* Open the pidfile as root.  We'll write the pid later on in the grandchild */
+    if (pidfile) {
+	pidfile_fd = open(pidfile, O_RDWR|O_CREAT, 0666);
+	if (pidfile_fd < 0) {
+	    syslog(LOG_ERR, "Could not open PID file %s: %m", pidfile);
+	    exit(EXIT_FAILURE);
+	}
+	/* It needs to be world-readable */
+	fchmod(pidfile_fd, 0644);
+    }
+
     /* Drop privileges */
     if (user) {
 	pw = getpwnam(user);
@@ -964,6 +996,63 @@ main(int argc, char *argv[], char **env)
 	Settings.maxRecipokPerDomain = 0;
     }
 
+    /* Daemonize */
+    if (!nodaemon) {
+	/* Set up a pipe so child can report back when it's happy */
+	if (pipe(kidpipe) < 0) {
+	    perror("pipe");
+	    exit(EXIT_FAILURE);
+	}
+	i = fork();
+	if (i < 0) {
+	    fprintf(stderr, "%s: fork() failed\n", argv[0]);
+	    exit(EXIT_FAILURE);
+	} else if (i != 0) {
+	    /* parent */
+	    /* Wait for a message from kid */
+	    close(kidpipe[1]);
+	    i = read(kidpipe[0], kidmsg, sizeof(kidmsg) - 1);
+	    if (i < 0) {
+		fprintf(stderr, "Error reading message from child: %s\n",
+			strerror(errno));
+		exit(EXIT_FAILURE);
+	    }
+	    /* Zero-terminate the string */
+	    kidmsg[i] = 0;
+	    if (i == 1 && kidmsg[0] == 'X') {
+		/* Child indicated successful startup */
+		exit(EXIT_SUCCESS);
+	    }
+	    if (i > 1 && kidmsg[0] == 'E') {
+		/* Child indicated error */
+		fprintf(stderr, "Error from child: %s\n", kidmsg+1);
+		exit(EXIT_FAILURE);
+	    }
+	    /* Unknown status from child */
+	    fprintf(stderr, "Unknown reply from child: %s\n", kidmsg);
+	    exit(EXIT_FAILURE);
+	}
+	setsid();
+	signal(SIGHUP, SIG_IGN);
+	i = fork();
+	if (i < 0) {
+	    fprintf(stderr, "%s: fork() failed\n", argv[0]);
+	    exit(EXIT_FAILURE);
+	} else if (i != 0) {
+	    exit(EXIT_SUCCESS);
+	}
+
+    }
+
+    /* Do the locking */
+    if (pidfile || lockfile) {
+	if ( (lockfile_fd = write_and_lock_pidfile(pidfile, lockfile, pidfile_fd)) < 0) {
+	    REPORT_FAILURE("Cannot lock lockfile: Is another copy running?");
+	    exit(EXIT_FAILURE);
+	}
+	pidfile_fd = -1;
+    }
+
     /* Initialize history buckets */
     init_history();
 
@@ -983,16 +1072,18 @@ main(int argc, char *argv[], char **env)
     AllSlaves = calloc(Settings.maxSlaves, sizeof(Slave));
 
     if (!AllSlaves) {
-	fprintf(stderr, "%s: Unable to allocate memory for slaves\n",
-		argv[0]);
+	REPORT_FAILURE("Unable to allocate memory for slaves");
+	if (pidfile) unlink(pidfile);
+	if (lockfile) unlink(lockfile);
 	exit(EXIT_FAILURE);
     }
 
     /* Make an event selector */
     es = Event_CreateSelector();
     if (!es) {
-	fprintf(stderr, "%s: Could not create event selector: %s\n",
-		argv[0], strerror(errno));
+	REPORT_FAILURE("Could not create event selector");
+	if (pidfile) unlink(pidfile);
+	if (lockfile) unlink(lockfile);
 	exit(EXIT_FAILURE);
     }
 
@@ -1051,21 +1142,21 @@ main(int argc, char *argv[], char **env)
 
     if (sock < 0) {
 	if (sock == -2) {
-	    fprintf(stderr, "%s: Argument to -s option must be a\nUNIX-domain socket, not a TCP socket.\n", argv[0]);
+	    REPORT_FAILURE("Argument to -s option must be a UNIX-domain socket, not a TCP socket.");
 	} else {
-	    fprintf(stderr, "%s: Unable to create listening socket on path %s\n",
-		    argv[0],
-		    Settings.sockName);
+	    REPORT_FAILURE("Unable to create listening socket.");
 	}
+	if (pidfile) unlink(pidfile);
+	if (lockfile) unlink(lockfile);
 	exit(EXIT_FAILURE);
     }
     set_cloexec(sock);
 
     /* Set up an accept loop */
     if (!EventTcp_CreateAcceptor(es, sock, handleAccept)) {
-	fprintf(stderr, "%s: Could not make accept() handler: %s\n",
-		argv[0],
-		strerror(errno));
+	REPORT_FAILURE("Could not make accept() handler");
+	if (pidfile) unlink(pidfile);
+	if (lockfile) unlink(lockfile);
 	exit(EXIT_FAILURE);
     }
 
@@ -1077,16 +1168,16 @@ main(int argc, char *argv[], char **env)
 						Settings.listenBacklog, 0);
 	umask(file_umask);
 	if (unpriv_sock < 0) {
-	    fprintf(stderr, "%s: Unable to create listening socket on path %s\n",
-		    argv[0],
-		    Settings.unprivSockName);
+	    REPORT_FAILURE("Unable to create unprivileged listening socket");
+	    if (pidfile) unlink(pidfile);
+	    if (lockfile) unlink(lockfile);
 	    exit(EXIT_FAILURE);
 	}
 	set_cloexec(unpriv_sock);
 	if (!EventTcp_CreateAcceptor(es, unpriv_sock, handleUnprivAccept)) {
-	    fprintf(stderr, "%s: Could not make accept() handler: %s\n",
-		    argv[0],
-		    strerror(errno));
+	    REPORT_FAILURE("Could not make accept() handler");
+	    if (pidfile) unlink(pidfile);
+	    if (lockfile) unlink(lockfile);
 	    exit(EXIT_FAILURE);
 	}
     }
@@ -1096,7 +1187,9 @@ main(int argc, char *argv[], char **env)
 
     /* Set up pipe for signal handler for slave termination notification*/
     if (pipe(Pipe) < 0) {
-	perror("pipe");
+	REPORT_FAILURE("pipe() call failed");
+	if (pidfile) unlink(pidfile);
+	if (lockfile) unlink(lockfile);
 	exit(EXIT_FAILURE);
     }
 
@@ -1106,42 +1199,28 @@ main(int argc, char *argv[], char **env)
     /* Create event handler for pipe */
     if (!Event_AddHandler(es, Pipe[0],
 			  EVENT_FLAG_READABLE, handlePipe, NULL)) {
-	fprintf(stderr, "%s: Could not make pipe handler: %s\n",
-		argv[0],
-		strerror(errno));
+	REPORT_FAILURE("Could not make pipe handler");
+	if (pidfile) unlink(pidfile);
+	if (lockfile) unlink(lockfile);
 	exit(EXIT_FAILURE);
     }
 
-    /* Daemonize */
-    if (!nodaemon) {
-	i = fork();
-	if (i < 0) {
-	    fprintf(stderr, "%s: fork() failed\n", argv[0]);
-	    exit(EXIT_FAILURE);
-	} else if (i != 0) {
-	    /* parent */
-	    exit(EXIT_SUCCESS);
-	}
-	setsid();
-	signal(SIGHUP, SIG_IGN);
-	i = fork();
-	if (i < 0) {
-	    fprintf(stderr, "%s: fork() failed\n", argv[0]);
-	    exit(EXIT_FAILURE);
-	} else if (i != 0) {
-	    exit(EXIT_SUCCESS);
+    /* Close files */
+    for (i=0; i<CLOSEFDS; i++) {
+	/* Don't close stdin/stdout/stderr if we are not a daemon */
+	if (nodaemon && i < 3) {
+	    continue;
 	}
-    }
-
-    for (i=0; i<256; i++) {
-	if (i == unpriv_sock || i == sock || i == Pipe[0] || i == Pipe[1]) continue;
+	if (i == kidpipe[0] || i == kidpipe[1] || i == lockfile_fd || i == unpriv_sock || i == sock || i == Pipe[0] || i == Pipe[1]) continue;
 	(void) close(i);
     }
 
-    /* Direct stdin/stdout/stderr to /dev/null */
-    open("/dev/null", O_RDWR);
-    open("/dev/null", O_RDWR);
-    open("/dev/null", O_RDWR);
+    /* Direct stdin/stdout/stderr to /dev/null if we are a daemon */
+    if (!nodaemon) {
+	open("/dev/null", O_RDWR);
+	open("/dev/null", O_RDWR);
+	open("/dev/null", O_RDWR);
+    }
 
     /* Syslog if required */
     if (Settings.syslog_label) {
@@ -1153,20 +1232,14 @@ main(int argc, char *argv[], char **env)
     /* Keep track of our pid */
     ParentPid = getpid();
 
-    /* Write pid */
-    if (pidfile) {
-	if (write_and_lock_pidfile(pidfile) < 0) {
-	    exit(1);
-	}
-	free(pidfile);
-    }
-
     /* Set up SIGHUP handler */
     act.sa_handler = hupHandler;
     sigemptyset(&act.sa_mask);
     act.sa_flags = SA_RESTART;
     if (sigaction(SIGHUP, &act, NULL) < 0) {
-	syslog(LOG_ERR, "sigaction failed - exiting: %m");
+	REPORT_FAILURE("sigaction failed - exiting.");
+	if (pidfile) unlink(pidfile);
+	if (lockfile) unlink(lockfile);
 	exit(EXIT_FAILURE);
     }
 
@@ -1175,7 +1248,9 @@ main(int argc, char *argv[], char **env)
     sigemptyset(&act.sa_mask);
     act.sa_flags = SA_RESTART;
     if (sigaction(SIGINT, &act, NULL) < 0) {
-	syslog(LOG_ERR, "sigaction failed - exiting: %m");
+	REPORT_FAILURE("sigaction failed - exiting.");
+	if (pidfile) unlink(pidfile);
+	if (lockfile) unlink(lockfile);
 	exit(EXIT_FAILURE);
     }
 
@@ -1207,10 +1282,12 @@ main(int argc, char *argv[], char **env)
 
     /* Set signal handler for SIGCHLD */
     if (set_sigchld_handler() < 0) {
-	syslog(LOG_ERR, "sigaction failed - exiting: %m");
+	REPORT_FAILURE("sigaction failed - exiting.");
 #ifdef EMBED_PERL
 	if (Settings.useEmbeddedPerl) term_embedded_interpreter();
 #endif
+	if (pidfile) unlink(pidfile);
+	if (lockfile) unlink(lockfile);
 	exit(EXIT_FAILURE);
     }
 
@@ -1267,6 +1344,10 @@ main(int argc, char *argv[], char **env)
 	}
     }
 
+    /* Tell the waiting parent that everything is fine */
+    write(kidpipe[1], "X", 1);
+    close(kidpipe[1]);
+
     /* And loop... */
     while(1) {
 	if (Event_HandleEvent(es) < 0) {
@@ -2599,7 +2680,7 @@ activateSlave(Slave *s, char const *reason)
     if (Settings.useEmbeddedPerl) {
 	run_embedded_filter();
 	term_embedded_interpreter();
-	exit(0);
+	exit(EXIT_SUCCESS);
     }
 #endif
     if (Settings.wantStatusReports) {
@@ -3174,9 +3255,15 @@ sigterm(int sig)
     /* Only the parent process should handle SIGTERM */
     if (ParentPid != getpid()) {
 	syslog(LOG_WARNING, "Child process received SIGTERM before signal disposition could be reset!  Exiting!");
-	exit(1);
+	exit(EXIT_FAILURE);
     }
 
+    if (pidfile) {
+	unlink(pidfile);
+    }
+    if (lockfile) {
+	unlink(lockfile);
+    }
     if (DOLOG) {
 	if (sig) {
 	    syslog(LOG_INFO, "Received SIGTERM: Stopping slaves and terminating");
@@ -3217,7 +3304,7 @@ sigterm(int sig)
 #ifdef EMBED_PERL
 	    if (Settings.useEmbeddedPerl) term_embedded_interpreter();
 #endif
-	    exit(1);
+	    exit(EXIT_FAILURE);
 	}
 	if (j != 9) {
 	    sleep(1);
@@ -3247,7 +3334,7 @@ sigterm(int sig)
 #ifdef EMBED_PERL
 	    if (Settings.useEmbeddedPerl) term_embedded_interpreter();
 #endif
-	    exit(1);
+	    exit(EXIT_FAILURE);
 	}
 	if (j != 9) {
 	    sleep(1);
diff --git a/mimedefang.8.in b/mimedefang.8.in
index 3151098..e0f45ca 100644
--- a/mimedefang.8.in
+++ b/mimedefang.8.in
@@ -139,7 +139,18 @@ the directory containing the failed message using syslog.
 .TP
 .B \-P \fIfileName\fR
 Causes \fBmimedefang\fR to write its process-ID (after
-becoming a daemon) to the specified file.
+becoming a daemon) to the specified file.  The file will be
+owned by root.
+
+.TP
+.B \-o \fIfileName\fR
+Causes \fbmimedefang\fR to use \fIfileName\fR as a lock file to avoid
+multiple instances from running.  If you supply \fB\-P\fR but not
+\fB\-o\fR, then \fbmimedefang\fR constructs a lock file by appending
+".lock" to the pid file.  However, this is less secure than having a
+root-owned pid file in a root-owned directory and a lock file writable
+by the user named by the \fB\-U\fR option.  (The lock file must be
+writable by the \fB\-U\fR user.)
 
 .TP
 .B \-R \fInum\fR
diff --git a/mimedefang.c b/mimedefang.c
index 7e74137..b311838 100644
--- a/mimedefang.c
+++ b/mimedefang.c
@@ -109,6 +109,8 @@ extern int find_syslog_facility(char const *facility_name);
 #define MD_SMFI_TRY(func, args) do { if (func args != MI_SUCCESS) syslog(LOG_WARNING, "%s: %s returned MI_FAILURE", data->qid, #func); } while (0)
 
 char *scan_body = NULL;
+static char *pidfile = NULL;
+static char *lockfile = NULL;
 
 #define KEY_FILE CONFDIR "/mimedefang-ip-key"
 
@@ -474,25 +476,6 @@ set_reply(SMFICTX *ctx,
 }
 
 /**********************************************************************
-*%FUNCTION: closefiles
-*%ARGUMENTS:
-* notthis - file descriptor that should not be closed
-*%RETURNS:
-* Nothing
-*%DESCRIPTION:
-* Hack -- closes a whole bunch of file descriptors.  Use after a fork()
-*         to conserve descriptors.
-***********************************************************************/
-static void
-closefiles(int notthis)
-{
-    int i;
-    for (i=0; i<CLOSEFDS; i++) {
-	if (i != notthis) (void) close(i);
-    }
-}
-
-/**********************************************************************
 *%FUNCTION: mfconnect
 *%ARGUMENTS:
 * ctx -- Sendmail filter mail context
@@ -2287,6 +2270,7 @@ usage(void)
     fprintf(stderr, "  -t                -- Do recipient checks before processing body\n");
     fprintf(stderr, "  -q                -- Allow new connections to be queued by multiplexor\n");
     fprintf(stderr, "  -P file           -- Write process-ID of daemon to specified file\n");
+    fprintf(stderr, "  -o file           -- Use specified file as a lock file\n");
     fprintf(stderr, "  -T                -- Log filter times to syslog\n");
     fprintf(stderr, "  -b n              -- Set listen() backlog to n\n");
     fprintf(stderr, "  -C                -- Try very hard to conserve file descriptors\n");
@@ -2306,7 +2290,8 @@ usage(void)
     exit(EXIT_FAILURE);
 }
 
-#define REPORT_FAILURE(msg, len) do { if (kidpipe[1] >= 0) { write(kidpipe[1], "E" msg, len+1); } else { fprintf(stderr, "%s\n", msg); } } while(0)
+#define REPORT_FAILURE(msg) do { if (kidpipe[1] >= 0) { write(kidpipe[1], "E" msg, strlen(msg)+1); } else { fprintf(stderr, "%s\n", msg); } } while(0)
+
 /**********************************************************************
 * %FUNCTION: main
 * %ARGUMENTS:
@@ -2322,7 +2307,6 @@ main(int argc, char **argv)
     int c;
     int mx_alive;
     pid_t i;
-    char *pidfile = NULL;
     struct passwd *pw = NULL;
     FILE *fp;
     int facility = LOG_MAIL;
@@ -2331,7 +2315,10 @@ main(int argc, char **argv)
     int got_p_option = 0;
     int kidpipe[2];
     char kidmsg[256];
-
+    int pidfile_fd = -1;
+    int lockfile_fd = -1;
+    int rc;
+    int j;
     mode_t socket_umask = 077;
     mode_t file_umask   = 077;
 
@@ -2388,7 +2375,7 @@ main(int argc, char **argv)
     }
 
     /* Process command line options */
-    while ((c = getopt(argc, argv, "GNCDHL:MP:R:S:TU:Xa:b:cdhkm:p:qrstvx:z:y")) != -1) {
+    while ((c = getopt(argc, argv, "GNCDHL:MP:o:R:S:TU:Xa:b:cdhkm:p:qrstvx:z:y")) != -1) {
 	switch (c) {
 	case 'y':
 	    setsymlist_ok = 1;
@@ -2521,6 +2508,16 @@ main(int argc, char **argv)
 		exit(EXIT_FAILURE);
 	    }
 	    break;
+	case 'o':
+	    /* Use this as our lock file */
+	    if (lockfile != NULL) free(lockfile);
+
+	    lockfile = strdup(optarg);
+	    if (!lockfile) {
+		fprintf(stderr, "%s: Out of memory\n", argv[0]);
+		exit(EXIT_FAILURE);
+	    }
+	    break;
 	case 'P':
 	    /* Write our pid to this file */
 	    if (pidfile != NULL) free(pidfile);
@@ -2611,6 +2608,17 @@ main(int argc, char **argv)
 	exit(EXIT_FAILURE);
     }
 
+    /* Open the pidfile as root.  We'll write the pid later on in the grandchild */
+    if (pidfile) {
+	pidfile_fd = open(pidfile, O_RDWR|O_CREAT, 0666);
+	if (pidfile_fd < 0) {
+	    syslog(LOG_ERR, "Could not open PID file %s: %m", pidfile);
+	    exit(EXIT_FAILURE);
+	}
+	/* It needs to be world-readable */
+	fchmod(pidfile_fd, 0644);
+    }
+
     /* Look up user */
     if (user) {
 	pw = getpwnam(user);
@@ -2655,6 +2663,8 @@ main(int argc, char **argv)
 	exit(EXIT_FAILURE);
     }
 
+    (void) closelog();
+
     /* Daemonize */
     if (!nodaemon) {
 	/* Set up a pipe so child can report back when it's happy */
@@ -2676,7 +2686,7 @@ main(int argc, char **argv)
 	    if (i < 0) {
 		fprintf(stderr, "Error reading message from child: %s\n",
 			strerror(errno));
-		exit(1);
+		exit(EXIT_FAILURE);
 	    }
 	    /* Zero-terminate the string */
 	    kidmsg[i] = 0;
@@ -2700,9 +2710,7 @@ main(int argc, char **argv)
 	signal(SIGHUP, SIG_IGN);
 	i = fork();
 	if (i < 0) {
-	    fprintf(stderr, "%s: fork() failed\n", argv[0]);
-	    /* Signal the waiting parent */
-	    write(kidpipe[1], "Efork() failed", 14);
+	    REPORT_FAILURE("fork() failed");
 	    exit(EXIT_FAILURE);
 	} else if (i != 0) {
 	    exit(EXIT_SUCCESS);
@@ -2713,23 +2721,32 @@ main(int argc, char **argv)
 	kidpipe[1] = -1;
     }
 
-    /* Write pid */
-    if (pidfile) {
-	if (write_and_lock_pidfile(pidfile) < 0) {
-	    /* Signal the waiting parent */
-	    REPORT_FAILURE("Cannot lock pidfile: Is another copy running?", 45);
-	    exit(1);
+    /* In the actual daemon */
+    for (j=0; j<CLOSEFDS; j++) {
+	/* If we are not a daemon, leave stdin/stdout/stderr open */
+	if (nodaemon && j < 3) {
+	    continue;
+	}
+	if (j != pidfile_fd && j != kidpipe[1]) {
+	    close(j);
 	}
-	free(pidfile);
     }
 
-    (void) closelog();
-    closefiles(kidpipe[1]);
+    /* Do the locking */
+    if (pidfile || lockfile) {
+	if ( (lockfile_fd = write_and_lock_pidfile(pidfile, lockfile, pidfile_fd)) < 0) {
+	    /* Signal the waiting parent */
+	    REPORT_FAILURE("Cannot lock lockfile: Is another copy running?");
+	    exit(EXIT_FAILURE);
+	}
+    }
 
     /* Direct stdin/stdout/stderr to /dev/null */
-    open("/dev/null", O_RDWR);
-    open("/dev/null", O_RDWR);
-    open("/dev/null", O_RDWR);
+    if (!nodaemon) {
+	open("/dev/null", O_RDWR);
+	open("/dev/null", O_RDWR);
+	open("/dev/null", O_RDWR);
+    }
 
     openlog("mimedefang", LOG_PID, facility);
 
@@ -2770,7 +2787,9 @@ main(int argc, char **argv)
 	syslog(LOG_INFO, "Multiplexor alive - entering main loop");
     } else {
 	/* Signal the waiting parent */
-	REPORT_FAILURE("Multiplexor socket did not appear.  Exiting.", 44);
+	REPORT_FAILURE("Multiplexor socket did not appear.  Exiting.");
+	if (pidfile) unlink(pidfile);
+	if (lockfile) unlink(lockfile);
 	exit(EXIT_FAILURE);
     }
 
@@ -2779,7 +2798,14 @@ main(int argc, char **argv)
 	write(kidpipe[1], "X", 1);
 	close(kidpipe[1]);
     }
-    return smfi_main();
+    rc = (int) smfi_main();
+    if (pidfile) {
+	unlink(pidfile);
+    }
+    if (lockfile) {
+	unlink(lockfile);
+    }
+    return rc;
 }
 
 /**********************************************************************
diff --git a/mimedefang.h b/mimedefang.h
index 381316d..59cf247 100644
--- a/mimedefang.h
+++ b/mimedefang.h
@@ -66,7 +66,7 @@ extern int make_listening_socket(char const *str, int backlog, int must_be_unix)
 extern void do_delay(char const *sleepstr);
 extern int is_localhost(struct sockaddr *);
 extern int remove_local_socket(char const *str);
-extern int write_and_lock_pidfile(char const *pidfile);
+extern int write_and_lock_pidfile(char const *pidfile, char *lockfile, int fd);
 #ifdef EMBED_PERL
 extern int make_embedded_interpreter(char const *progPath,
 				     char const *subFilter,
diff --git a/redhat/mimedefang-init.in b/redhat/mimedefang-init.in
index d3a514f..8f1aff0 100644
--- a/redhat/mimedefang-init.in
+++ b/redhat/mimedefang-init.in
@@ -31,7 +31,7 @@
 #              scans on incoming mail messages.
 # processname: mimedefang
 # config: @CONFDIR_EVAL@/mimedefang-filter
-# pidfile: @SPOOLDIR@/mimedefang.pid
+# pidfile: /var/run/mimedefang.pid
 
 ### BEGIN INIT INFO
 # Provides:          mimedefang
@@ -122,36 +122,37 @@ start() {
     echo -n "Starting $prog-multiplexor: "
     [ -e $MX_SOCKET ] && rm -f $MX_SOCKET
     # Tricky stuff below... "echo -E" won't work, hence the two-step.
-    daemon $PROGDIR/$prog-multiplexor -p @SPOOLDIR@/$prog-multiplexor.pid \
-	$([ -n "$FILTER" ] && echo "-f $FILTER") \
-	$([ -n "$SYSLOG_FACILITY" ] && echo "-S $SYSLOG_FACILITY") \
-	$([ -n "$SUBFILTER" ] && echo "-F $SUBFILTER") \
-	$([ -n "$MX_MINIMUM" ] && echo "-m $MX_MINIMUM") \
-	$([ -n "$MX_MAXIMUM" ] && echo "-x $MX_MAXIMUM") \
-	$([ -n "$MX_RECIPOK_PERDOMAIN_LIMIT" ] && echo "-y $MX_RECIPOK_PERDOMAIN_LIMIT") \
-	$([ -n "$MX_USER" ] && echo "-U $MX_USER") \
-	$([ -n "$MX_IDLE" ] && echo "-i $MX_IDLE") \
-	$([ -n "$MX_BUSY" ] && echo "-b $MX_BUSY") \
-	$([ -n "$MX_QUEUE_SIZE" ] && echo "-q $MX_QUEUE_SIZE") \
-	$([ -n "$MX_QUEUE_TIMEOUT" ] && echo "-Q $MX_QUEUE_TIMEOUT") \
-	$([ -n "$MX_REQUESTS" ] && echo "-r $MX_REQUESTS") \
-	$([ -n "$MX_MAP_SOCKET" ] && echo "-N $MX_MAP_SOCKET") \
-	$([ -n "$MX_SLAVE_DELAY" ] && echo "-w $MX_SLAVE_DELAY") \
-	$([ -n "$MX_MIN_SLAVE_DELAY" ] && echo "-W $MX_MIN_SLAVE_DELAY") \
-	$([ -n "$MX_LOG_SLAVE_STATUS_INTERVAL" ] && echo "-L $MX_LOG_SLAVE_STATUS_INTERVAL") \
-	$([ -n "$MX_MAX_RSS" ] && echo "-R $MX_MAX_RSS") \
-	$([ -n "$MX_MAX_AS" ] && echo "-M $MX_MAX_AS") \
-	$([ "$MX_EMBED_PERL" = "yes" ] && (echo -n "-"; echo "E")) \
-	$([ "$MX_LOG" = "yes" ] && echo "-l") \
-	$([ "$MX_STATS" = "yes" ] && echo "-t /var/log/mimedefang/stats") \
-	$([ "$MX_STATUS_UPDATES" = "yes" ] && echo "-Z") \
-	$([ "$MX_STATS" = "yes" -a "$MX_FLUSH_STATS" = "yes" ] && echo "-u") \
-	$([ -n "$MX_TICK_REQUEST" ] && echo "-X $MX_TICK_REQUEST") \
-	$([ -n "$MX_TICK_PARALLEL" ] && echo "-P $MX_TICK_PARALLEL") \
-	$([ "$MX_STATS_SYSLOG" = "yes" ] && echo "-T") \
-	$([ "$MD_ALLOW_GROUP_ACCESS" = "yes" ] && echo "-G") \
-	$([ -n "$MX_NOTIFIER" ] && echo "-O $MX_NOTIFIER") \
-	-s $MX_SOCKET
+    daemon $PROGDIR/$prog-multiplexor -p /var/run/$prog-multiplexor.pid \
+	   -o @SPOOLDIR@/$prog-multiplexor.lock \
+	   $([ -n "$FILTER" ] && echo "-f $FILTER") \
+	   $([ -n "$SYSLOG_FACILITY" ] && echo "-S $SYSLOG_FACILITY") \
+	   $([ -n "$SUBFILTER" ] && echo "-F $SUBFILTER") \
+	   $([ -n "$MX_MINIMUM" ] && echo "-m $MX_MINIMUM") \
+	   $([ -n "$MX_MAXIMUM" ] && echo "-x $MX_MAXIMUM") \
+	   $([ -n "$MX_RECIPOK_PERDOMAIN_LIMIT" ] && echo "-y $MX_RECIPOK_PERDOMAIN_LIMIT") \
+	   $([ -n "$MX_USER" ] && echo "-U $MX_USER") \
+	   $([ -n "$MX_IDLE" ] && echo "-i $MX_IDLE") \
+	   $([ -n "$MX_BUSY" ] && echo "-b $MX_BUSY") \
+	   $([ -n "$MX_QUEUE_SIZE" ] && echo "-q $MX_QUEUE_SIZE") \
+	   $([ -n "$MX_QUEUE_TIMEOUT" ] && echo "-Q $MX_QUEUE_TIMEOUT") \
+	   $([ -n "$MX_REQUESTS" ] && echo "-r $MX_REQUESTS") \
+	   $([ -n "$MX_MAP_SOCKET" ] && echo "-N $MX_MAP_SOCKET") \
+	   $([ -n "$MX_SLAVE_DELAY" ] && echo "-w $MX_SLAVE_DELAY") \
+	   $([ -n "$MX_MIN_SLAVE_DELAY" ] && echo "-W $MX_MIN_SLAVE_DELAY") \
+	   $([ -n "$MX_LOG_SLAVE_STATUS_INTERVAL" ] && echo "-L $MX_LOG_SLAVE_STATUS_INTERVAL") \
+	   $([ -n "$MX_MAX_RSS" ] && echo "-R $MX_MAX_RSS") \
+	   $([ -n "$MX_MAX_AS" ] && echo "-M $MX_MAX_AS") \
+	   $([ "$MX_EMBED_PERL" = "yes" ] && (echo -n "-"; echo "E")) \
+	   $([ "$MX_LOG" = "yes" ] && echo "-l") \
+	   $([ "$MX_STATS" = "yes" ] && echo "-t /var/log/mimedefang/stats") \
+	   $([ "$MX_STATUS_UPDATES" = "yes" ] && echo "-Z") \
+	   $([ "$MX_STATS" = "yes" -a "$MX_FLUSH_STATS" = "yes" ] && echo "-u") \
+	   $([ -n "$MX_TICK_REQUEST" ] && echo "-X $MX_TICK_REQUEST") \
+	   $([ -n "$MX_TICK_PARALLEL" ] && echo "-P $MX_TICK_PARALLEL") \
+	   $([ "$MX_STATS_SYSLOG" = "yes" ] && echo "-T") \
+	   $([ "$MD_ALLOW_GROUP_ACCESS" = "yes" ] && echo "-G") \
+	   $([ -n "$MX_NOTIFIER" ] && echo "-O $MX_NOTIFIER") \
+	   -s $MX_SOCKET
     echo
 
     # Start daemon
@@ -162,31 +163,27 @@ start() {
     # thread-creation will fail on a very busy server.
     ulimit -s 2048
 
-    daemon $PROGDIR/$prog -P @SPOOLDIR@/$prog.pid \
-	-m $MX_SOCKET \
-	$([ -n "$LOOPBACK_RESERVED_CONNECTIONS" ] && echo "-R $LOOPBACK_RESERVED_CONNECTIONS") \
-	$([ -n "$MX_USER" ] && echo "-U $MX_USER") \
-	$([ -n "$SYSLOG_FACILITY" ] && echo "-S $SYSLOG_FACILITY") \
-	$([ "$LOG_FILTER_TIME" = "yes" ] && echo "-T") \
-	$([ "$MX_RELAY_CHECK" = "yes" ] && echo "-r") \
-	$([ "$MX_HELO_CHECK" = "yes" ] && echo "-H") \
-	$([ "$MX_SENDER_CHECK" = "yes" ] && echo "-s") \
-	$([ "$MX_RECIPIENT_CHECK" = "yes" ] && echo "-t") \
-	$([ "$KEEP_FAILED_DIRECTORIES" = "yes" ] && echo "-k") \
-	$([ "$MD_ALLOW_GROUP_ACCESS" = "yes" ] && echo "-G") \
-	$([ -n "$MD_EXTRA" ] && echo "$MD_EXTRA") \
-	$([ "$ALLOW_NEW_CONNECTIONS_TO_QUEUE" = "yes" ] && echo "-q") \
-	-p $SOCKET
+    daemon $PROGDIR/$prog -P /var/run/$prog.pid \
+	   -o @SPOOLDIR@/$prog.lock \
+	   -m $MX_SOCKET \
+	   $([ -n "$LOOPBACK_RESERVED_CONNECTIONS" ] && echo "-R $LOOPBACK_RESERVED_CONNECTIONS") \
+	   $([ -n "$MX_USER" ] && echo "-U $MX_USER") \
+	   $([ -n "$SYSLOG_FACILITY" ] && echo "-S $SYSLOG_FACILITY") \
+	   $([ "$LOG_FILTER_TIME" = "yes" ] && echo "-T") \
+	   $([ "$MX_RELAY_CHECK" = "yes" ] && echo "-r") \
+	   $([ "$MX_HELO_CHECK" = "yes" ] && echo "-H") \
+	   $([ "$MX_SENDER_CHECK" = "yes" ] && echo "-s") \
+	   $([ "$MX_RECIPIENT_CHECK" = "yes" ] && echo "-t") \
+	   $([ "$KEEP_FAILED_DIRECTORIES" = "yes" ] && echo "-k") \
+	   $([ "$MD_ALLOW_GROUP_ACCESS" = "yes" ] && echo "-G") \
+	   $([ -n "$MD_EXTRA" ] && echo "$MD_EXTRA") \
+	   $([ "$ALLOW_NEW_CONNECTIONS_TO_QUEUE" = "yes" ] && echo "-q") \
+	   -p $SOCKET
     RETVAL=$?
     echo
 
     [ $RETVAL -eq 0 ] && touch /var/lock/subsys/$prog
 
-    # Red Hat gets upset if pid files are not under /var/run, so let's
-    # keep Red Hat happy...
-    sleep 1
-    [ -f @SPOOLDIR@/$prog.pid ] && cp -f @SPOOLDIR@/$prog.pid /var/run
-    [ -f @SPOOLDIR@/$prog-multiplexor.pid ] && cp -f @SPOOLDIR@/$prog-multiplexor.pid /var/run
     return $RETVAL
 }
 
@@ -219,7 +216,7 @@ stop() {
     echo
 
     [ -e $SOCKET ] && rm -f $SOCKET
-    [ -f @SPOOLDIR@/$prog.pid ] && rm -f @SPOOLDIR@/$prog.pid
+    [ -f /var/run/$prog.pid ] && rm -f /var/run/$prog.pid
     [ -f /var/run/$prog.pid ] && rm -f /var/run/$prog.pid
 
     # Stop daemon
@@ -232,8 +229,8 @@ stop() {
     if [ "$1" = "wait" ] ; then
 	printf "Waiting for daemons to exit"
 	WAITPID=""
-	test -f @SPOOLDIR@/$prog.pid && WAITPID=`cat @SPOOLDIR@/$prog.pid`
-	test -f @SPOOLDIR@/prog-multiplexor.pid && WAITPID="$WAITPID `cat @SPOOLDIR@/prog-multiplexor.pid`"
+	test -f /var/run/$prog.pid && WAITPID=`cat /var/run/$prog.pid`
+	test -f /var/run//prog-multiplexor.pid && WAITPID="$WAITPID `cat /var/run//prog-multiplexor.pid`"
 	n=0
 	while [ -n "$WAITPID" ] ; do
 	    W2=""
@@ -252,7 +249,7 @@ stop() {
 	echo ""
     fi
 
-    [ -f @SPOOLDIR@/$prog-multiplexor.pid ] && rm -f @SPOOLDIR@/$prog-multiplexor.pid
+    [ -f /var/run/$prog-multiplexor.pid ] && rm -f /var/run/$prog-multiplexor.pid
     [ -f /var/run/$prog-multiplexor.pid ] && rm -f /var/run/$prog-multiplexor.pid
 
     [ $RETVAL -eq 0 ] && rm -f /var/lock/subsys/$prog
@@ -306,8 +303,8 @@ case "$1" in
 		echo "Could not communicate with $prog-multiplexor"
 	    fi
 	else
-	    if [ -r @SPOOLDIR@/$prog-multiplexor.pid ] ; then
-		kill -INT `cat @SPOOLDIR@/$prog-multiplexor.pid`
+	    if [ -r /var/run/$prog-multiplexor.pid ] ; then
+		kill -INT `cat /var/run/$prog-multiplexor.pid`
 		RETVAL=$?
 		if [ $RETVAL = 0 ] ; then
 		    echo "Told $prog-multiplexor to force reread of filter rules."
diff --git a/utils.c b/utils.c
index 7d4f9c1..4e84059 100644
--- a/utils.c
+++ b/utils.c
@@ -1300,30 +1300,53 @@ free_debug(void *ctx, void *x, char const *fname, int line)
 #endif
 
 int
-write_and_lock_pidfile(char const *pidfile)
+write_and_lock_pidfile(char const *pidfile, char *lockfile, int pidfile_fd)
 {
-    int fd;
     struct flock fl;
     char buf[64];
+    int lockfile_fd;
+    size_t len;
+
+    if (!lockfile) {
+	if (!pidfile) {
+	    return -1;
+	}
+	len = strlen(pidfile) + 6;
+	/* If no lockfile was supplied, construct one based on pidfile */
+	lockfile = malloc(len);
+	if (!lockfile) {
+	    return -1;
+	}
+
+	snprintf(lockfile, len, "%s.lock", pidfile);
+    }
+
+    lockfile_fd = open(lockfile, O_RDWR|O_CREAT, 0666);
+    if (lockfile_fd < 0) {
+	return -1;
+    }
 
     fl.l_type = F_WRLCK;
     fl.l_whence = SEEK_SET;
     fl.l_start = 0;
     fl.l_len = 0;
 
-    fd = open(pidfile, O_RDWR|O_CREAT, 0666);
-    if (fd < 0) {
-	syslog(LOG_ERR, "Could not open PID file %s: %m", pidfile);
+    if (fcntl(lockfile_fd, F_SETLK, &fl) < 0) {
+	syslog(LOG_ERR, "Could not lock lockfile file %s: %m.  Is another copy running?", lockfile);
 	return -1;
     }
-    if (fcntl(fd, F_SETLK, &fl) < 0) {
-	syslog(LOG_ERR, "Could not lock PID file %s: %m.  Is another copy running?", pidfile);
-	return -1;
+    if (pidfile_fd >= 0) {
+	ftruncate(pidfile_fd, 0);
+	snprintf(buf, sizeof(buf), "%lu\n", (unsigned long) getpid());
+	write(pidfile_fd, buf, strlen(buf));
+
+	/* Close the pidfile fd; no longer needed */
+	if (close(pidfile_fd) < 0) {
+	    return -1;
+	}
     }
-    ftruncate(fd, 0);
-    snprintf(buf, sizeof(buf), "%lu\n", (unsigned long) getpid());
-    write(fd, buf, strlen(buf));
-    /* Do NOT close fd... it will close and lock will be released
+
+    /* Do NOT close lockfile_fd... it will close and lock will be released
        when we exit */
-    return 0;
+    return lockfile_fd;
 }
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 173 bytes
Desc: OpenPGP digital signature
URL: <https://lists.mimedefang.org/pipermail/mimedefang_lists.mimedefang.org/attachments/20170831/d31dab22/attachment.sig>


More information about the MIMEDefang mailing list