fix an issue observed this week-end while flooding ajacoutot@ :

we keep track of available fd's to prevent scheduling of messages if we
know that we are going to fail. however, since the envelope is not
removed from the scheduler, it will be rescheduled right away leading to
a busy loop in the scheduler. we know flag the mda/mta processes as BUSY
and do not schedule envelopes that target a BUSY process.

also, fix a potential bug that could lead to a use after free when doing
a batch/message/host traversal of schedulable envelopes.

while at it fix misuse of env->sc_opts as env->sc_flags, was not really
causing any issue as the misuse was constant ...

5 files changed, 77 insertions, 31 deletions

diff --git a/usr.sbin/smtpd/queue_fsqueue.c b/usr.sbin/smtpd/queue_fsqueue.c
index d7312ecae6d..f428965f4db 100644
--- a/usr.sbin/smtpd/queue_fsqueue.c
+++ b/usr.sbin/smtpd/queue_fsqueue.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: queue_fsqueue.c,v 1.37 2012/01/29 16:54:13 eric Exp $	*/
+/*	$OpenBSD: queue_fsqueue.c,v 1.38 2012/01/31 21:05:26 gilles Exp $	*/
 
 /*
  * Copyright (c) 2011 Gilles Chehade <gilles@openbsd.org>
@@ -243,6 +243,8 @@ fsqueue_envelope_delete(enum queue_kind qkind, struct envelope *ep)
 {
 	char pathname[MAXPATHLEN];
 
+	log_debug("#### %s: queue_envelope_delete: %016" PRIx64,
+	    __func__, ep->id);
 	fsqueue_envelope_path(qkind, ep->id, pathname, sizeof(pathname));
 
 	if (unlink(pathname) == -1) {
diff --git a/usr.sbin/smtpd/runner.c b/usr.sbin/smtpd/runner.c
index b02fc5e5f0e..75eb8a45bfd 100644
--- a/usr.sbin/smtpd/runner.c
+++ b/usr.sbin/smtpd/runner.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: runner.c,v 1.134 2012/01/28 16:50:02 gilles Exp $	*/
+/*	$OpenBSD: runner.c,v 1.135 2012/01/31 21:05:26 gilles Exp $	*/
 
 /*
  * Copyright (c) 2008 Gilles Chehade <gilles@openbsd.org>
@@ -77,6 +77,7 @@ runner_imsg(struct imsgev *iev, struct imsg *imsg)
 	case IMSG_QUEUE_DELIVERY_OK:
 		stat_decrement(STATS_RUNNER);
 		e = imsg->data;
+		log_debug("queue_delivery_ok: %016"PRIx64, e->id);
 		queue_envelope_delete(Q_QUEUE, e);
 		return;
 
@@ -85,6 +86,7 @@ runner_imsg(struct imsgev *iev, struct imsg *imsg)
 		e = imsg->data;
 		e->retry++;
 		queue_envelope_update(Q_QUEUE, e);
+		log_debug("queue_delivery_tempfail: %016"PRIx64, e->id);
 		scheduler->insert(e);
 		runner_reset_events();
 		return;
@@ -93,27 +95,34 @@ runner_imsg(struct imsgev *iev, struct imsg *imsg)
 		stat_decrement(STATS_RUNNER);
 		e = imsg->data;
 		if (e->type != D_BOUNCE && e->sender.user[0] != '\0') {
-			log_debug("PERMFAIL #2: %016" PRIx64, e->id);
 			bounce_record_message(e, &bounce);
+			log_debug("queue_delivery_permfail: %016"PRIx64,
+			    bounce.id);
 			scheduler->insert(&bounce);
 			runner_reset_events();
-
 		}
 		queue_envelope_delete(Q_QUEUE, e);
 		return;
 
 	case IMSG_MDA_SESS_NEW:
 		stat_decrement(STATS_MDA_SESSION);
+		if (env->sc_maxconn - stat_get(STATS_MDA_SESSION, STAT_ACTIVE))
+			env->sc_flags &= ~SMTPD_MDA_BUSY;
+		runner_reset_events();
 		return;
 
 	case IMSG_BATCH_DONE:
 		stat_decrement(STATS_MTA_SESSION);
+		if (env->sc_maxconn - stat_get(STATS_MTA_SESSION, STAT_ACTIVE))
+			env->sc_flags &= ~SMTPD_MTA_BUSY;
+		runner_reset_events();
 		return;
 
 	case IMSG_SMTP_ENQUEUE:
 		e = imsg->data;
 		if (imsg->fd < 0 || !bounce_session(imsg->fd, e)) {
 			queue_envelope_update(Q_QUEUE, e);
+			log_debug("smtp_enqueue: %016"PRIx64, e->id);
 			scheduler->insert(e);
 			runner_reset_events();
 			return;
@@ -121,20 +130,20 @@ runner_imsg(struct imsgev *iev, struct imsg *imsg)
 		return;
 
 	case IMSG_QUEUE_PAUSE_MDA:
-		env->sc_opts |= SMTPD_MDA_PAUSED;
+		env->sc_flags |= SMTPD_MDA_PAUSED;
 		return;
 
 	case IMSG_QUEUE_RESUME_MDA:
-		env->sc_opts &= ~SMTPD_MDA_PAUSED;
+		env->sc_flags &= ~SMTPD_MDA_PAUSED;
 		runner_reset_events();
 		return;
 
 	case IMSG_QUEUE_PAUSE_MTA:
-		env->sc_opts |= SMTPD_MTA_PAUSED;
+		env->sc_flags |= SMTPD_MTA_PAUSED;
 		return;
 
 	case IMSG_QUEUE_RESUME_MTA:
-		env->sc_opts &= ~SMTPD_MTA_PAUSED;
+		env->sc_flags &= ~SMTPD_MTA_PAUSED;
 		runner_reset_events();
 		return;
 
@@ -348,21 +357,27 @@ runner_process_envelope(u_int64_t evpid)
 	mta_av = env->sc_maxconn - stat_get(STATS_MTA_SESSION, STAT_ACTIVE);
 	mda_av = env->sc_maxconn - stat_get(STATS_MDA_SESSION, STAT_ACTIVE);
 	bnc_av = env->sc_maxconn - stat_get(STATS_RUNNER_BOUNCES, STAT_ACTIVE);
-	
+
 	if (! queue_envelope_load(Q_QUEUE, evpid, &envelope))
 		return 0;
 
 	if (envelope.type == D_MDA)
-		if (mda_av == 0)
+		if (mda_av == 0) {
+			env->sc_flags |= SMTPD_MDA_BUSY;
 			return 0;
+		}
 
 	if (envelope.type == D_MTA)
-		if (mta_av == 0)
+		if (mta_av == 0) {
+			env->sc_flags |= SMTPD_MTA_BUSY;
 			return 0;
+		}
 
 	if (envelope.type == D_BOUNCE)
-		if (bnc_av == 0)
+		if (bnc_av == 0) {
+			env->sc_flags |= SMTPD_BOUNCE_BUSY;
 			return 0;
+		}
 
 	if (runner_check_loop(&envelope)) {
 		struct envelope bounce;
@@ -370,12 +385,14 @@ runner_process_envelope(u_int64_t evpid)
 		envelope_set_errormsg(&envelope, "loop has been detected");
 		bounce_record_message(&envelope, &bounce);
 		scheduler->insert(&bounce);
+		scheduler->remove(NULL, evpid);
 		queue_envelope_delete(Q_QUEUE, &envelope);
 
 		runner_setup_events();
 		return 0;
 	}
 
+
 	return runner_process_batch(envelope.type, evpid);
 }
 
@@ -397,7 +414,7 @@ runner_process_batch(enum delivery_type type, u_int64_t evpid)
 			imsg_compose_event(env->sc_ievs[PROC_QUEUE],
 			    IMSG_SMTP_ENQUEUE, PROC_SMTP, 0, -1, &evp,
 			    sizeof evp);
-			scheduler->remove(evpid);
+			scheduler->remove(batch, evpid);
 		}
 		stat_increment(STATS_RUNNER);
 		stat_increment(STATS_RUNNER_BOUNCES);
@@ -413,7 +430,7 @@ runner_process_batch(enum delivery_type type, u_int64_t evpid)
 		imsg_compose_event(env->sc_ievs[PROC_QUEUE],
 		    IMSG_MDA_SESS_NEW, PROC_MDA, 0, fd, &evp,
 		    sizeof evp);
-		scheduler->remove(evpid);
+		scheduler->remove(batch, evpid);
 
 		stat_increment(STATS_RUNNER);
 		stat_increment(STATS_MDA_SESSION);
@@ -448,7 +465,7 @@ runner_process_batch(enum delivery_type type, u_int64_t evpid)
 			    IMSG_BATCH_APPEND, PROC_MTA, 0, -1, &evp,
 			    sizeof evp);
 
-			scheduler->remove(evpid);
+			scheduler->remove(batch, evpid);
 			stat_increment(STATS_RUNNER);
 		}
 
@@ -579,5 +596,5 @@ runner_remove_envelope(u_int64_t evpid)
 
 	evp.id = evpid;
 	queue_envelope_delete(Q_QUEUE, &evp);
-	scheduler->remove(evpid);
+	scheduler->remove(NULL, evpid);
 }
diff --git a/usr.sbin/smtpd/scheduler_ramqueue.c b/usr.sbin/smtpd/scheduler_ramqueue.c
index aeaeae3b954..c622ae4c576 100644
--- a/usr.sbin/smtpd/scheduler_ramqueue.c
+++ b/usr.sbin/smtpd/scheduler_ramqueue.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: scheduler_ramqueue.c,v 1.3 2012/01/30 10:02:55 chl Exp $	*/
+/*	$OpenBSD: scheduler_ramqueue.c,v 1.4 2012/01/31 21:05:26 gilles Exp $	*/
 
 /*
  * Copyright (c) 2012 Gilles Chehade <gilles@openbsd.org>
@@ -118,7 +118,7 @@ static void  scheduler_ramqueue_init(void);
 static int   scheduler_ramqueue_setup(time_t, time_t);
 static int   scheduler_ramqueue_next(u_int64_t *, time_t *);
 static void  scheduler_ramqueue_insert(struct envelope *);
-static void  scheduler_ramqueue_remove(u_int64_t);
+static void  scheduler_ramqueue_remove(void *, u_int64_t);
 static void *scheduler_ramqueue_host(char *);
 static void *scheduler_ramqueue_message(u_int32_t);
 static void *scheduler_ramqueue_batch(u_int64_t);
@@ -263,10 +263,10 @@ scheduler_ramqueue_next(u_int64_t *evpid, time_t *sched)
 	log_debug("scheduler_ramqueue: next");
 	TAILQ_FOREACH(rq_evp, &ramqueue.queue, queue_entry) {
 		if (rq_evp->rq_batch->type == D_MDA)
-			if (env->sc_opts & SMTPD_MDA_PAUSED)
+			if (env->sc_flags & (SMTPD_MDA_PAUSED|SMTPD_MDA_BUSY))
 				continue;
 		if (rq_evp->rq_batch->type == D_MTA)
-			if (env->sc_opts & SMTPD_MTA_PAUSED)
+			if (env->sc_flags & (SMTPD_MTA_PAUSED|SMTPD_MTA_BUSY))
 				continue;
 		if (evpid)
 			*evpid = rq_evp->evpid;
@@ -331,8 +331,9 @@ scheduler_ramqueue_insert(struct envelope *envelope)
 }
 
 static void
-scheduler_ramqueue_remove(u_int64_t evpid)
+scheduler_ramqueue_remove(void *hdl, u_int64_t evpid)
 {
+	struct ramqueue_iter *iter = hdl;
 	struct ramqueue_batch *rq_batch;
 	struct ramqueue_message *rq_msg;
 	struct ramqueue_envelope *rq_evp;
@@ -349,17 +350,33 @@ scheduler_ramqueue_remove(u_int64_t evpid)
 	TAILQ_REMOVE(&ramqueue.queue, rq_evp, queue_entry);
 	stat_decrement(STATS_RAMQUEUE_ENVELOPE);
 
+
 	/* check if we are the last of a batch */
-	if (TAILQ_FIRST(&rq_batch->envelope_queue) == NULL)
+	if (TAILQ_FIRST(&rq_batch->envelope_queue) == NULL) {
 		ramqueue_remove_batch(rq_host, rq_batch);
+		if (iter != NULL && iter->type == RAMQUEUE_ITER_BATCH) {
+			log_debug("scheduler_ramqueue_remove: batch removed");
+			iter->u.batch = NULL;
+		}
+	}
 
 	/* check if we are the last of a message */
-	if (RB_ROOT(&rq_msg->evptree) == NULL)
+	if (RB_ROOT(&rq_msg->evptree) == NULL) {
 		ramqueue_remove_message(rq_msg);
+		if (iter != NULL && iter->type == RAMQUEUE_ITER_MESSAGE) {
+			log_debug("scheduler_ramqueue_remove: message removed");
+			iter->u.message = NULL;
+		}
+	}
 
 	/* check if we are the last of a host */
-	if (TAILQ_FIRST(&rq_host->batch_queue) == NULL)
+	if (TAILQ_FIRST(&rq_host->batch_queue) == NULL) {
 		ramqueue_remove_host(rq_host);
+		if (iter != NULL && iter->type == RAMQUEUE_ITER_HOST) {
+			log_debug("scheduler_ramqueue_remove: host removed");
+			iter->u.host = NULL;
+		}
+	}
 
 	free(rq_evp);
 }
@@ -422,6 +439,7 @@ scheduler_ramqueue_message(u_int32_t msgid)
 	iter->u.message = rq_msg;
 
 	return iter;
+
 }
 
 static void *
@@ -453,6 +471,8 @@ scheduler_ramqueue_fetch(void *hdl, u_int64_t *evpid)
 
 	switch (iter->type) {
 	case RAMQUEUE_ITER_HOST: {
+		if (iter->u.host == NULL)
+			return 0;
 		rq_batch = TAILQ_FIRST(&iter->u.host->batch_queue);
 		if (rq_batch == NULL)
 			break;
@@ -464,6 +484,8 @@ scheduler_ramqueue_fetch(void *hdl, u_int64_t *evpid)
 	}
 
 	case RAMQUEUE_ITER_BATCH:
+		if (iter->u.batch == NULL)
+			return 0;
 		rq_evp = TAILQ_FIRST(&iter->u.batch->envelope_queue);
 		if (rq_evp == NULL)
 			break;
@@ -471,6 +493,8 @@ scheduler_ramqueue_fetch(void *hdl, u_int64_t *evpid)
 		return 1;
 
 	case RAMQUEUE_ITER_MESSAGE:
+		if (iter->u.message == NULL)
+			return 0;
 		rq_evp = RB_ROOT(&iter->u.message->evptree);
 		if (rq_evp == NULL)
 			break;
diff --git a/usr.sbin/smtpd/smtp.c b/usr.sbin/smtpd/smtp.c
index 85b81960f1f..2b5d7cf1884 100644
--- a/usr.sbin/smtpd/smtp.c
+++ b/usr.sbin/smtpd/smtp.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: smtp.c,v 1.100 2012/01/29 11:37:32 eric Exp $	*/
+/*	$OpenBSD: smtp.c,v 1.101 2012/01/31 21:05:26 gilles Exp $	*/
 
 /*
  * Copyright (c) 2008 Gilles Chehade <gilles@openbsd.org>
@@ -398,7 +398,7 @@ smtp_pause(void)
 	struct listener *l;
 
 	log_debug("smtp: pausing listening sockets");
-	env->sc_opts |= SMTPD_SMTP_PAUSED;
+	env->sc_flags |= SMTPD_SMTP_PAUSED;
 
 	TAILQ_FOREACH(l, env->sc_listeners, entry)
 		event_del(&l->ev);
@@ -410,7 +410,7 @@ smtp_resume(void)
 	struct listener *l;
 
 	log_debug("smtp: resuming listening sockets");
-	env->sc_opts &= ~SMTPD_SMTP_PAUSED;
+	env->sc_flags &= ~SMTPD_SMTP_PAUSED;
 
 	TAILQ_FOREACH(l, env->sc_listeners, entry)
 		event_add(&l->ev, NULL);
@@ -445,7 +445,7 @@ smtp_enqueue(uid_t *euid)
 	 * call to smtp_pause() because enqueue listener is not a real socket
 	 * and thus cannot be paused properly.
 	 */
-	if (env->sc_opts & SMTPD_SMTP_PAUSED)
+	if (env->sc_flags & SMTPD_SMTP_PAUSED)
 		return (-1);
 
 	if ((s = smtp_new(l)) == NULL)
@@ -504,7 +504,7 @@ smtp_new(struct listener *l)
 
 	log_debug("smtp: new client on listener: %p", l);
 
-	if (env->sc_opts & SMTPD_SMTP_PAUSED)
+	if (env->sc_flags & SMTPD_SMTP_PAUSED)
 		fatalx("smtp_new: unexpected client");
 
 	if ((s = calloc(1, sizeof(*s))) == NULL)
diff --git a/usr.sbin/smtpd/smtpd.h b/usr.sbin/smtpd/smtpd.h
index 151c81593e8..d7873d85ebd 100644
--- a/usr.sbin/smtpd/smtpd.h
+++ b/usr.sbin/smtpd/smtpd.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: smtpd.h,v 1.285 2012/01/29 11:37:32 eric Exp $	*/
+/*	$OpenBSD: smtpd.h,v 1.286 2012/01/31 21:05:26 gilles Exp $	*/
 
 /*
  * Copyright (c) 2008 Gilles Chehade <gilles@openbsd.org>
@@ -591,6 +591,9 @@ struct smtpd {
 #define SMTPD_MDA_PAUSED		       	 0x00000004
 #define SMTPD_MTA_PAUSED		       	 0x00000008
 #define SMTPD_SMTP_PAUSED		       	 0x00000010
+#define SMTPD_MDA_BUSY			       	 0x00000020
+#define SMTPD_MTA_BUSY			       	 0x00000040
+#define SMTPD_BOUNCE_BUSY      		       	 0x00000080
 	u_int32_t				 sc_flags;
 	struct timeval				 sc_qintval;
 	int					 sc_qexpire;
@@ -975,7 +978,7 @@ struct scheduler_backend {
 	int	(*next)(u_int64_t *, time_t *);
 
 	void	(*insert)(struct envelope *);
-	void	(*remove)(u_int64_t);
+	void	(*remove)(void *, u_int64_t);
 
 	void	*(*host)(char *);
 	void	*(*message)(u_int32_t);