fix a very annoying events masking issue which would cause a fatal() to be

hit under certain conditions; while tracking the bug I ran into other bugs
which were kind of related and could cause us to hit a fatal() too.

fix by me, but with lots of testing and investigation with jacekm@,
ok jacekm@

4 files changed, 88 insertions, 33 deletions

diff --git a/usr.sbin/smtpd/queue_shared.c b/usr.sbin/smtpd/queue_shared.c
index 7803b6dd75f..4c901e2d07a 100644
--- a/usr.sbin/smtpd/queue_shared.c
+++ b/usr.sbin/smtpd/queue_shared.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: queue_shared.c,v 1.5 2009/01/29 12:43:25 jacekm Exp $	*/
+/*	$OpenBSD: queue_shared.c,v 1.6 2009/01/30 16:37:52 gilles Exp $	*/
 
 /*
  * Copyright (c) 2008 Gilles Chehade <gilles@openbsd.org>
@@ -65,8 +65,11 @@ queue_create_layout_message(char *queuepath, char *message_id)
 		fatalx("queue_create_layout_message: snprintf");
 
 	if (mkdtemp(rootdir) == NULL) {
-		if (errno == ENOSPC)
+		if (errno == ENOSPC) {
+			log_debug("FAILED WITH ENOSPC");
+			bzero(message_id, MAX_ID_SIZE);
 			return 0;
+		}
 		fatal("queue_create_layout_message: mkdtemp");
 	}
 
@@ -79,7 +82,9 @@ queue_create_layout_message(char *queuepath, char *message_id)
 
 	if (mkdir(evpdir, 0700) == -1) {
 		if (errno == ENOSPC) {
+			log_debug("FAILED WITH ENOSPC");
 			rmdir(rootdir);
+			bzero(message_id, MAX_ID_SIZE);
 			return 0;
 		}
 		fatal("queue_create_layout_message: mkdir");
@@ -100,6 +105,9 @@ queue_delete_layout_message(char *queuepath, char *msgid)
 		fatalx("snprintf");
 
 	if (rename(rootdir, purgedir) == -1) {
+		log_debug("ID: %s", msgid);
+		log_debug("PATH: %s", rootdir);
+		log_debug("PURGE: %s", purgedir);
 		fatal("queue_delete_layout_message: rename");
 	}
 }
diff --git a/usr.sbin/smtpd/smtp.c b/usr.sbin/smtpd/smtp.c
index 2d10a710dd5..0f5f24c18fb 100644
--- a/usr.sbin/smtpd/smtp.c
+++ b/usr.sbin/smtpd/smtp.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: smtp.c,v 1.18 2009/01/29 21:59:15 jacekm Exp $	*/
+/*	$OpenBSD: smtp.c,v 1.19 2009/01/30 16:37:52 gilles Exp $	*/
 
 /*
  * Copyright (c) 2008 Gilles Chehade <gilles@openbsd.org>
@@ -173,8 +173,11 @@ smtp_dispatch_parent(int sig, short event, void *p)
 			key.s_msg.id = reply->session_id;
 
 			s = SPLAY_FIND(sessiontree, &env->sc_sessions, &key);
-			if (s == NULL) {
-				/* Session was removed while we were waiting for the message */
+			if (s == NULL)
+				fatal("smtp_dispatch_parent: session is gone");
+
+			if (s->s_flags & F_QUIT) {
+				session_destroy(s);
 				break;
 			}
 
@@ -243,8 +246,11 @@ smtp_dispatch_mfa(int sig, short event, void *p)
 			key.s_msg.id = ss->id;
 
 			s = SPLAY_FIND(sessiontree, &env->sc_sessions, &key);
-			if (s == NULL) {
-				/* Session was removed while we were waiting for the message */
+			if (s == NULL)
+				fatal("smtp_dispatch_mfa: session is gone");
+
+			if (s->s_flags & F_QUIT) {
+				session_destroy(s);
 				break;
 			}
 
@@ -306,15 +312,20 @@ smtp_dispatch_lka(int sig, short event, void *p)
 			key.s_id = s->s_id;
 
 			ss = SPLAY_FIND(sessiontree, &env->sc_sessions, &key);
-			if (ss == NULL) {
-				/* Session was removed while we were waiting
-				 * for the message */
+			if (ss == NULL)
+				fatal("smtp_dispatch_lka: session is gone");
+
+			if (ss->s_flags & F_QUIT) {
+				session_destroy(s);
 				break;
 			}
 
 			strlcpy(ss->s_hostname, s->s_hostname, MAXHOSTNAMELEN);
 			strlcpy(ss->s_msg.session_hostname, s->s_hostname,
 			    MAXHOSTNAMELEN);
+
+			session_pickup(s, NULL);
+
 			break;
 		}
 		default:
@@ -374,8 +385,11 @@ smtp_dispatch_queue(int sig, short event, void *p)
 			key.s_id = ss->id;
 
 			s = SPLAY_FIND(sessiontree, &env->sc_sessions, &key);
-			if (s == NULL) {
-				/* Session was removed while we were waiting for the message */
+			if (s == NULL)
+				fatal("smtp_dispatch_queue: session is gone");
+
+			if (s->s_flags & F_QUIT) {
+				session_destroy(s);
 				break;
 			}
 
@@ -396,12 +410,16 @@ smtp_dispatch_queue(int sig, short event, void *p)
 			key.s_id = ss->id;
 
 			s = SPLAY_FIND(sessiontree, &env->sc_sessions, &key);
-			if (s == NULL) {
-				/* Session was removed while we were waiting for the message */
+			if (s == NULL)
+				fatal("smtp_dispatch_queue: session is gone");
+
+			fd = imsg_get_fd(ibuf, &imsg);
+
+			if (s->s_flags & F_QUIT) {
+				session_destroy(s);
 				break;
 			}
 
-			fd = imsg_get_fd(ibuf, &imsg);
 			if (fd != -1) {
 				s->s_msg.datafp = fdopen(fd, "w");
 				if (s->s_msg.datafp == NULL) {
@@ -426,10 +444,14 @@ smtp_dispatch_queue(int sig, short event, void *p)
 			key.s_msg.id = ss->id;
 
 			s = SPLAY_FIND(sessiontree, &env->sc_sessions, &key);
-			if (s == NULL) {
-				/* Session was removed while we were waiting for the message */
+			if (s == NULL)
+				fatal("smtp_dispatch_queue: session is gone");
+
+			if (s->s_flags & F_QUIT) {
+				session_destroy(s);
 				break;
 			}
+
 			s->s_msg.status |= S_MESSAGE_TEMPFAILURE;
 			break;
 		}
@@ -446,13 +468,20 @@ smtp_dispatch_queue(int sig, short event, void *p)
 			key.s_msg.id = ss->id;
 
 			s = SPLAY_FIND(sessiontree, &env->sc_sessions, &key);
-			if (s == NULL) {
-				/* Session was removed while we were waiting for the message */
+			if (s == NULL)
+				fatal("smtp_dispatch_queue: session is gone");
+
+			if (imsg.hdr.type == IMSG_QUEUE_COMMIT_MESSAGE) {
+				s->s_msg.message_id[0] = '\0';
+				s->s_msg.message_uid[0] = '\0';
+			}
+
+			if (s->s_flags & F_QUIT) {
+				session_destroy(s);
 				break;
 			}
 
 			session_pickup(s, ss);
-
 			break;
 		}
 		default:
diff --git a/usr.sbin/smtpd/smtp_session.c b/usr.sbin/smtpd/smtp_session.c
index 935bed2b676..1340cc703e5 100644
--- a/usr.sbin/smtpd/smtp_session.c
+++ b/usr.sbin/smtpd/smtp_session.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: smtp_session.c,v 1.47 2009/01/29 21:59:15 jacekm Exp $	*/
+/*	$OpenBSD: smtp_session.c,v 1.48 2009/01/30 16:37:52 gilles Exp $	*/
 
 /*
  * Copyright (c) 2008 Gilles Chehade <gilles@openbsd.org>
@@ -196,6 +196,7 @@ session_rfc4954_auth_plain(struct session *s, char *arg, size_t nr)
 
 	imsg_compose(s->s_env->sc_ibufs[PROC_PARENT], IMSG_PARENT_AUTHENTICATE,
 	    0, 0, -1, &s->s_auth, sizeof(s->s_auth));
+	s->s_flags |= F_EVLOCKED;
 	bufferevent_disable(s->s_bev, EV_READ);
 
 	return 1;
@@ -246,6 +247,7 @@ session_rfc4954_auth_login(struct session *s, char *arg, size_t nr)
 
 	imsg_compose(s->s_env->sc_ibufs[PROC_PARENT], IMSG_PARENT_AUTHENTICATE,
 	    0, 0, -1, &s->s_auth, sizeof(s->s_auth));
+	s->s_flags |= F_EVLOCKED;
 	bufferevent_disable(s->s_bev, EV_READ);
 
 	return 1;
@@ -424,6 +426,7 @@ session_rfc5321_mail_handler(struct session *s, char *args)
 
 	imsg_compose(s->s_env->sc_ibufs[PROC_MFA], IMSG_MFA_MAIL,
 	    0, 0, -1, &s->s_msg, sizeof(s->s_msg));
+	s->s_flags |= F_EVLOCKED;
 	bufferevent_disable(s->s_bev, EV_READ);
 	return 1;
 }
@@ -472,6 +475,7 @@ session_rfc5321_rcpt_handler(struct session *s, char *args)
 
 	imsg_compose(s->s_env->sc_ibufs[PROC_MFA], IMSG_MFA_RCPT,
 	    0, 0, -1, &mr, sizeof(mr));
+	s->s_flags |= F_EVLOCKED;
 	bufferevent_disable(s->s_bev, EV_READ);
 	return 1;
 }
@@ -601,6 +605,7 @@ session_auth_pickup(struct session *s, char *arg, size_t nr)
 	if (s == NULL)
 		fatal("session_pickup: desynchronized");
 
+	s->s_flags &= ~F_EVLOCKED;
 	bufferevent_enable(s->s_bev, EV_READ);
 
 	switch (s->s_state) {
@@ -632,6 +637,7 @@ session_pickup(struct session *s, struct submit_status *ss)
 	if (s == NULL)
 		fatal("session_pickup: desynchronized");
 
+	s->s_flags &= ~F_EVLOCKED;
 	bufferevent_enable(s->s_bev, EV_READ);
 
 	if ((ss != NULL && ss->code == 421) ||
@@ -650,6 +656,7 @@ session_pickup(struct session *s, struct submit_status *ss)
 		break;
 
 	case S_TLS:
+		s->s_flags |= F_EVLOCKED;
 		bufferevent_disable(s->s_bev, EV_READ|EV_WRITE);
 		s->s_state = S_GREETED;
 		ssl_session_init(s);
@@ -669,13 +676,13 @@ session_pickup(struct session *s, struct submit_status *ss)
 		imsg_compose(s->s_env->sc_ibufs[PROC_QUEUE],
 		    IMSG_QUEUE_CREATE_MESSAGE, 0, 0, -1, &s->s_msg,
 		    sizeof(s->s_msg));
+		s->s_flags |= F_EVLOCKED;
 		bufferevent_disable(s->s_bev, EV_READ);
 		break;
 
 	case S_MAIL:
 
 		session_respond(s, "%d Sender ok", ss->code);
-
 		break;
 
 	case S_RCPTREQUEST:
@@ -693,9 +700,9 @@ session_pickup(struct session *s, struct submit_status *ss)
 		s->s_state = S_RCPT;
 		s->s_msg.rcptcount++;
 		s->s_msg.recipient = ss->u.path;
-		session_respond(s, "%d Recipient ok", ss->code);
 
 	case S_RCPT:
+		session_respond(s, "%d Recipient ok", ss->code);
 		break;
 
 	case S_DATAREQUEST:
@@ -703,6 +710,7 @@ session_pickup(struct session *s, struct submit_status *ss)
 		imsg_compose(s->s_env->sc_ibufs[PROC_QUEUE],
 		    IMSG_QUEUE_MESSAGE_FILE, 0, 0, -1, &s->s_msg,
 		    sizeof(s->s_msg));
+		s->s_flags |= F_EVLOCKED;
 		bufferevent_disable(s->s_bev, EV_READ);
 		break;
 
@@ -724,7 +732,6 @@ session_pickup(struct session *s, struct submit_status *ss)
 		break;
 
 	default:
-		log_debug("session_pickup: state value: %d", s->s_state);
 		fatal("session_pickup: unknown state");
 		break;
 	}
@@ -747,14 +754,16 @@ session_init(struct listener *l, struct session *s)
 
 	s_smtp.clients++;
 
+	if ((s->s_bev = bufferevent_new(s->s_fd, session_read, session_write,
+	    session_error, s)) == NULL)
+		fatalx("session_init: bufferevent_new failed");
+
 	strlcpy(s->s_hostname, "<unknown>", MAXHOSTNAMELEN);
 	strlcpy(s->s_msg.session_hostname, s->s_hostname, MAXHOSTNAMELEN);
 	imsg_compose(s->s_env->sc_ibufs[PROC_LKA], IMSG_LKA_HOST, 0, 0, -1, s,
 	    sizeof(struct session));
-
-	if ((s->s_bev = bufferevent_new(s->s_fd, session_read, session_write,
-	    session_error, s)) == NULL)
-		fatalx("session_init: bufferevent_new failed");
+	s->s_flags |= F_EVLOCKED;
+	bufferevent_disable(s->s_bev, EV_READ);
 
 	SPLAY_INSERT(sessiontree, &s->s_env->sc_sessions, s);
 
@@ -763,8 +772,6 @@ session_init(struct listener *l, struct session *s)
 		ssl_session_init(s);
 		return;
 	}
-
-	session_pickup(s, NULL);
 }
 
 void
@@ -930,6 +937,8 @@ session_cleanup(struct session *s)
 		    sizeof(s->s_msg));
 		s->s_msg.message_id[0] = '\0';
 		s->s_msg.message_uid[0] = '\0';
+		s->s_flags |= F_EVLOCKED;
+		bufferevent_disable(s->s_bev, EV_READ);
 	}
 }
 
@@ -938,7 +947,14 @@ session_error(struct bufferevent *bev, short event, void *p)
 {
 	struct session	*s = p;
 
-	session_destroy(s);
+	/* If events are locked, do not destroy session
+	 * but set F_QUIT flag so that we destroy it as
+	 * soon as the event lock is removed.
+	 */
+	if (s->s_flags & F_EVLOCKED)
+		s->s_flags |= F_QUIT;
+	else
+		session_destroy(s);
 }
 
 void
@@ -947,6 +963,7 @@ session_msg_submit(struct session *s)
 	imsg_compose(s->s_env->sc_ibufs[PROC_QUEUE],
 	    IMSG_QUEUE_COMMIT_MESSAGE, 0, 0, -1, &s->s_msg,
 	    sizeof(s->s_msg));
+	s->s_flags |= F_EVLOCKED;
 	bufferevent_disable(s->s_bev, EV_READ);
 	s->s_state = S_DONE;
 }
diff --git a/usr.sbin/smtpd/smtpd.h b/usr.sbin/smtpd/smtpd.h
index 9ae8818cf55..60b50aa413d 100644
--- a/usr.sbin/smtpd/smtpd.h
+++ b/usr.sbin/smtpd/smtpd.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: smtpd.h,v 1.62 2009/01/29 21:59:15 jacekm Exp $	*/
+/*	$OpenBSD: smtpd.h,v 1.63 2009/01/30 16:37:52 gilles Exp $	*/
 
 /*
  * Copyright (c) 2008 Gilles Chehade <gilles@openbsd.org>
@@ -579,7 +579,8 @@ enum session_flags {
 	F_8BITMIME	= 0x4,
 	F_SECURE	= 0x8,
 	F_AUTHENTICATED	= 0x10,
-	F_PEERHASTLS	= 0x20
+	F_PEERHASTLS	= 0x20,
+	F_EVLOCKED	= 0x40
 };
 
 struct session {