Do not unlink an offline message until it has been correctly enqueued.

While there, simplify the offline_enqueue() function by doing all the
sanity checks in the forked process,  and remove all fatal(): on error,
the offline message is left untouched in the directory.  Also, get rid
of the path_starts_with() check since all paths to offline messages are
now constructed internally.

ok gilles@ chl@

1 files changed, 53 insertions, 67 deletions

diff --git a/usr.sbin/smtpd/smtpd.c b/usr.sbin/smtpd/smtpd.c
index 7a3b9f2eb63..86fdd0e61f2 100644
--- a/usr.sbin/smtpd/smtpd.c
+++ b/usr.sbin/smtpd/smtpd.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: smtpd.c,v 1.137 2011/11/14 19:23:41 chl Exp $	*/
+/*	$OpenBSD: smtpd.c,v 1.138 2011/11/16 11:18:54 eric Exp $	*/
 
 /*
  * Copyright (c) 2008 Gilles Chehade <gilles@openbsd.org>
@@ -56,7 +56,6 @@ static void parent_send_config_ruleset(int);
 static void parent_sig_handler(int, short, void *);
 static void forkmda(struct imsgev *, u_int32_t, struct deliver *);
 static int parent_forward_open(char *);
-static int path_starts_with(char *, char *);
 static void fork_peers(void);
 static struct child *child_lookup(pid_t);
 static struct child *child_add(pid_t, int, int);
@@ -378,7 +377,10 @@ parent_sig_handler(int sig, short event, void *p)
 			case CHILD_ENQUEUE_OFFLINE:
 				if (fail)
 					log_warnx("smtpd: couldn't enqueue offline "
-					    "message; smtpctl %s", cause);
+					    "message %s; smtpctl %s", child->path, cause);
+				else
+					unlink(child->path);
+				free(child->path);
 				offline_done();
 				break;
 
@@ -897,7 +899,10 @@ offline_scan(int fd, short ev, void *arg)
 		if (d->d_type != DT_REG)
 			continue;
 
-		offline_add(d->d_name);
+		if (offline_add(d->d_name)) {
+			log_warnx("smtpd: could not add offline message %s", d->d_name);
+			continue;
+		}
 
 		if ((n++) == OFFLINE_READMAX) {
 			evtimer_set(&offline_ev, offline_scan, dir);
@@ -915,59 +920,31 @@ offline_scan(int fd, short ev, void *arg)
 static int
 offline_enqueue(char *name)
 {
-	char			path[MAXPATHLEN];
+	char		 t[MAXPATHLEN], *path;
 	struct user_backend *ub;
 	struct user	 u;
 	struct stat	 sb;
 	pid_t		 pid;
+	struct child	*child;
 
-	strlcpy(path, PATH_SPOOL PATH_OFFLINE "/", sizeof path);
-	if (strlcat(path, name, sizeof path) >= sizeof path)
-		fatalx("smtpd: path name too long");
-
-	if (! path_starts_with(path, PATH_SPOOL PATH_OFFLINE))
-		fatalx("smtpd: path outside offline dir");
-
-	log_debug("smtpd: enqueueing offline message %s", path);
-
-	if (lstat(path, &sb) == -1) {
-		if (errno == ENOENT) {
-			log_warn("smtpd: lstat: %s", path);
-			return (0);
-		}
-		fatal("lstat");
+	if (!bsnprintf(t, sizeof t, "%s/%s", PATH_SPOOL PATH_OFFLINE, name)) {
+		log_warnx("smtpd: path name too long");
+		return (-1);
 	}
 
-	if (chflags(path, 0) == -1) {
-		if (errno == ENOENT) {
-			log_warn("smtpd: chflags: %s", path);
-			return (0);
-		}
-		fatal("chflags");
+	if ((path = strdup(t)) == NULL) {
+		log_warn("smtpd: strdup");
+		return (-1);
 	}
 
-	ub = user_backend_lookup(USER_GETPWNAM);
-	bzero(&u, sizeof (u));
-	errno = 0;
-	if (! ub->getbyuid(&u, sb.st_uid)) {
-		log_warn("smtpd: getpwuid for uid %d failed",
-		    sb.st_uid);
-		unlink(path);
-		return (0);
-	}
+	log_debug("smtpd: enqueueing offline message %s", path);
 
-	if (! S_ISREG(sb.st_mode)) {
-		log_warnx("smtpd: file %s (uid %d) not regular, removing", path, sb.st_uid);
-		if (S_ISDIR(sb.st_mode))
-			rmdir(path);
-		else
-			unlink(path);
-		return (0);
+	if ((pid = fork()) == -1) {
+		log_warn("smtpd: fork");
+		free(path);
+		return (-1);
 	}
 
-	if ((pid = fork()) == -1)
-		fatal("offline_enqueue: fork");
-
 	if (pid == 0) {
 		char	*envp[2], *p, *tmp;
 		FILE	*fp;
@@ -976,19 +953,39 @@ offline_enqueue(char *name)
 
 		bzero(&args, sizeof(args));
 
+		if (lstat(path, &sb) == -1) {
+			log_warn("smtpd: lstat: %s", path);
+			_exit(1);
+		}
+
+		if (chflags(path, 0) == -1) {
+			log_warn("smtpd: chflags: %s", path);
+			_exit(1);
+		}
+
+		ub = user_backend_lookup(USER_GETPWNAM);
+		bzero(&u, sizeof (u));
+		errno = 0;
+		if (! ub->getbyuid(&u, sb.st_uid)) {
+			log_warnx("smtpd: getpwuid for uid %d failed",
+			    sb.st_uid);
+			_exit(1);
+		}
+
+		if (! S_ISREG(sb.st_mode)) {
+			log_warnx("smtpd: file %s (uid %d) not regular",
+			    path, sb.st_uid);
+			_exit(1);
+		}
+
 		if (setgroups(1, &u.gid) ||
 		    setresgid(u.gid, u.gid, u.gid) ||
 		    setresuid(u.uid, u.uid, u.uid) ||
-		    closefrom(STDERR_FILENO + 1) == -1) {
-			unlink(path);
+		    closefrom(STDERR_FILENO + 1) == -1)
 			_exit(1);
-		}
 
-		if ((fp = fopen(path, "r")) == NULL) {
-			unlink(path);
+		if ((fp = fopen(path, "r")) == NULL)
 			_exit(1);
-		}
-		unlink(path);
 
 		if (chdir(u.directory) == -1 && chdir("/") == -1)
 			_exit(1);
@@ -1022,9 +1019,10 @@ offline_enqueue(char *name)
 	}
 
 	offline_running++;
-	child_add(pid, CHILD_ENQUEUE_OFFLINE, -1);
+	child = child_add(pid, CHILD_ENQUEUE_OFFLINE, -1);
+	child->path = path;
 
-	return (1);
+	return (0);
 }
 
 static int
@@ -1043,7 +1041,7 @@ offline_add(char *path)
 	memmove(q->path, path, strlen(path) + 1);
 	TAILQ_INSERT_TAIL(&offline_q, q, entry);
 
-	return (1);
+	return (0);
 }
 
 static void
@@ -1096,18 +1094,6 @@ parent_forward_open(char *username)
 }
 
 int
-path_starts_with(char *file, char *prefix)
-{
-	char	 rprefix[MAXPATHLEN];
-	char	 rfile[MAXPATHLEN];
-
-	if (realpath(file, rfile) == NULL || realpath(prefix, rprefix) == NULL)
-		return (-1);
-
-	return (strncmp(rfile, rprefix, strlen(rprefix)) == 0);
-}
-
-int
 child_cmp(struct child *c1, struct child *c2)
 {
 	if (c1->pid < c2->pid)