when operating in enqueue mode, it was easy to make smtpctl fatal() by

writing a small app that sent out of order imsg's. prevent this by use
of a state machine and read event masking.

issue spotted by jacekm@, temporary fix by me. there are ideas around
this, but we want to experiment them a bit and they are low priority.

1 files changed, 39 insertions, 13 deletions

diff --git a/usr.sbin/smtpd/control.c b/usr.sbin/smtpd/control.c
index 1472af8d7c1..90f4da1b3a4 100644
--- a/usr.sbin/smtpd/control.c
+++ b/usr.sbin/smtpd/control.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: control.c,v 1.18 2009/03/01 13:05:41 jacekm Exp $	*/
+/*	$OpenBSD: control.c,v 1.19 2009/03/08 20:39:49 gilles Exp $	*/
 
 /*
  * Copyright (c) 2008 Pierre-Yves Ritschard <pyr@openbsd.org>
@@ -318,38 +318,52 @@ control_dispatch_ext(int fd, short event, void *arg)
 		case IMSG_MFA_RCPT: {
 			struct message_recipient *mr;
 
+			if (c->state != CS_INIT && c->state != CS_RCPT)
+				goto badstate;
+
 			mr = imsg.data;
 			imsg_compose(env->sc_ibufs[PROC_MFA], IMSG_MFA_RCPT, 0, 0, -1,
 			    mr, sizeof(*mr));
-
+			event_del(&c->ibuf.ev);
 			break;
 		}
 		case IMSG_QUEUE_CREATE_MESSAGE: {
 			struct message *messagep;
 
+			if (c->state != CS_NONE && c->state != CS_DONE)
+				goto badstate;
+
 			messagep = imsg.data;
 			messagep->session_id = fd;
 			imsg_compose(env->sc_ibufs[PROC_QUEUE], IMSG_QUEUE_CREATE_MESSAGE, 0, 0, -1,
 			    messagep, sizeof(*messagep));
-
+			event_del(&c->ibuf.ev);
 			break;
 		}
 		case IMSG_QUEUE_MESSAGE_FILE: {
 			struct message *messagep;
 
+			if (c->state != CS_RCPT)
+				goto badstate;
+
 			messagep = imsg.data;
 			messagep->session_id = fd;
 			imsg_compose(env->sc_ibufs[PROC_QUEUE], IMSG_QUEUE_MESSAGE_FILE, 0, 0, -1,
 			    messagep, sizeof(*messagep));
+			event_del(&c->ibuf.ev);
 			break;
 		}
 		case IMSG_QUEUE_COMMIT_MESSAGE: {
 			struct message *messagep;
 
+			if (c->state != CS_FD)
+				goto badstate;
+
 			messagep = imsg.data;
 			messagep->session_id = fd;
 			imsg_compose(env->sc_ibufs[PROC_QUEUE], IMSG_QUEUE_COMMIT_MESSAGE, 0, 0, -1,
 			    messagep, sizeof(*messagep));
+			event_del(&c->ibuf.ev);
 			break;
 		}
 		case IMSG_STATS: {
@@ -363,7 +377,6 @@ control_dispatch_ext(int fd, short event, void *arg)
 			imsg_compose(env->sc_ibufs[PROC_QUEUE], IMSG_STATS, 0, 0, -1, &s, sizeof(s));
 			imsg_compose(env->sc_ibufs[PROC_RUNNER], IMSG_STATS, 0, 0, -1, &s, sizeof(s));
 			imsg_compose(env->sc_ibufs[PROC_SMTP], IMSG_STATS, 0, 0, -1, &s, sizeof(s));
-
 			break;
 		}
 		case IMSG_RUNNER_SCHEDULE: {
@@ -490,6 +503,8 @@ control_dispatch_ext(int fd, short event, void *arg)
 		}
 		imsg_free(&imsg);
 		continue;
+
+badstate:
 badcred:
 		imsg_compose(&c->ibuf, IMSG_CTL_FAIL, 0, 0, -1,
 		    NULL, 0);
@@ -656,15 +671,17 @@ control_dispatch_mfa(int sig, short event, void *p)
 			struct ctl_conn		*c;
 
 			ss = imsg.data;
-
-			if (ss->code == 250)
-				break;
-
 			if ((c = control_connbyfd(ss->id)) == NULL) {
 				log_warn("control_dispatch_queue: fd %lld: not found", ss->id);
 				return;
 			}
-			
+
+			event_add(&c->ibuf.ev, NULL);
+			if (ss->code == 250) {
+				c->state = CS_RCPT;
+				break;
+			}
+
 			imsg_compose(&c->ibuf, IMSG_CTL_FAIL, 0, 0, -1, NULL, 0);
 
 			break;
@@ -724,12 +741,14 @@ control_dispatch_queue(int sig, short event, void *p)
 				log_warn("control_dispatch_queue: fd %lld: not found", ss->id);
 				return;
 			}
+			event_add(&c->ibuf.ev, NULL);
 
 			if (ss->code != 250) {
 				imsg_compose(&c->ibuf, IMSG_CTL_FAIL, 0, 0, -1,
 				    NULL, 0);
 			}
 			else {
+				c->state = CS_INIT;
 				ss->msg.session_id = ss->id;
 				strlcpy(ss->msg.message_id, ss->u.msgid,
 				    sizeof(ss->msg.message_id));
@@ -748,7 +767,8 @@ control_dispatch_queue(int sig, short event, void *p)
 				log_warn("control_dispatch_queue: fd %lld: not found", ss->id);
 				return;
 			}
-
+			event_add(&c->ibuf.ev, NULL);
+			c->state = CS_RCPT;
 			imsg_compose(&c->ibuf, IMSG_CTL_OK, 0, 0, -1,
 			    NULL, 0);
 
@@ -765,11 +785,14 @@ control_dispatch_queue(int sig, short event, void *p)
 				    ss->id);
 				return;
 			}
+			event_add(&c->ibuf.ev, NULL);
 
 			fd = imsg_get_fd(ibuf, &imsg);
-			if (ss->code == 250)
+			if (ss->code == 250) {
+				c->state = CS_FD;
 				imsg_compose(&c->ibuf, IMSG_CTL_OK, 0, 0, fd,
 				    &ss->msg, sizeof(struct message));
+			}
 			else
 				imsg_compose(&c->ibuf, IMSG_CTL_FAIL, 0, 0, -1,
 				    &ss->msg, sizeof(struct message));
@@ -785,10 +808,13 @@ control_dispatch_queue(int sig, short event, void *p)
 				    ss->id);
 				return;
 			}
+			event_add(&c->ibuf.ev, NULL);
 
-			if (ss->code == 250)
+			if (ss->code == 250) {
+				c->state = CS_DONE;
 				imsg_compose(&c->ibuf, IMSG_CTL_OK, 0, 0, -1,
 				    &ss->msg, sizeof(struct message));
+			}
 			else
 				imsg_compose(&c->ibuf, IMSG_CTL_FAIL, 0, 0, -1,
 				    &ss->msg, sizeof(struct message));
@@ -967,7 +993,7 @@ session_socket_blockmode(int fd, enum blockmodes bm)
 
 	if (bm == BM_NONBLOCK)
 		flags |= O_NONBLOCK;
-	else
+
 		flags &= ~O_NONBLOCK;
 
 	if ((flags = fcntl(fd, F_SETFL, flags)) == -1)