Index | Thread | Search

From:
Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject:
smtpd: convert to imsg_get_fd()
To:
tech@openbsd.org
Date:
Thu, 18 Jan 2024 16:48:42 +0100

Download raw body.

Thread
smtpd is the last program in base that needs to be converted to use
imsg_get_fd(). It is also a little bit the reason for this conversion
since with imsg_get_fd() the owner of the fd is will known and imsg_free()
will soon start to close unclaimed fds. This prevents unexpected fd leaks.

Now smtpd does a lot of fd passing but most of it is simple.
In log_imsg() imsg.fd can no longer be inspected. I hope this is OK.
This inspection in debug code violates the ownership principle.
In my opinion logging the fd is not very helpful since fds are reused
quickly.

Please test if you use smtpd in more than the base config.
-- 
:wq Claudio

Index: control.c
===================================================================
RCS file: /cvs/src/usr.sbin/smtpd/control.c,v
diff -u -p -r1.130 control.c
--- control.c	31 May 2023 16:51:46 -0000	1.130
+++ control.c	13 Dec 2023 13:19:05 -0000
@@ -107,7 +107,8 @@ control_imsg(struct mproc *p, struct ims
 		c = tree_get(&ctl_conns, imsg->hdr.peerid);
 		if (c == NULL)
 			return;
-		m_compose(&c->mproc, IMSG_CTL_OK, 0, 0, imsg->fd, NULL, 0);
+		m_compose(&c->mproc, IMSG_CTL_OK, 0, 0, imsg_get_fd(imsg),
+		    NULL, 0);
 		return;
 
 	case IMSG_STAT_INCREMENT:
Index: enqueue.c
===================================================================
RCS file: /cvs/src/usr.sbin/smtpd/enqueue.c,v
diff -u -p -r1.121 enqueue.c
--- enqueue.c	31 May 2023 16:51:46 -0000	1.121
+++ enqueue.c	16 Jan 2024 13:31:25 -0000
@@ -808,7 +808,7 @@ open_connection(void)
 			errx(1, "unexpected imsg reply type");
 		}
 
-		fd = imsg.fd;
+		fd = imsg_get_fd(&imsg);
 		imsg_free(&imsg);
 
 		break;
Index: lka.c
===================================================================
RCS file: /cvs/src/usr.sbin/smtpd/lka.c,v
diff -u -p -r1.247 lka.c
--- lka.c	14 Jun 2021 17:58:15 -0000	1.247
+++ lka.c	16 Jan 2024 13:19:04 -0000
@@ -47,7 +47,7 @@ static void
 lka_imsg(struct mproc *p, struct imsg *imsg)
 {
 	struct table		*table;
-	int			 ret;
+	int			 ret, fd;
 	struct sockaddr_storage	 ss;
 	struct userinfo		 userinfo;
 	struct addrname		 addrname;
@@ -305,7 +305,7 @@ lka_imsg(struct mproc *p, struct imsg *i
 		return;
 
 	case IMSG_LKA_OPEN_FORWARD:
-		lka_session_forward_reply(imsg->data, imsg->fd);
+		lka_session_forward_reply(imsg->data, imsg_get_fd(imsg));
 		return;
 
 	case IMSG_LKA_AUTHENTICATE:
@@ -351,7 +351,7 @@ lka_imsg(struct mproc *p, struct imsg *i
 		m_add_string(p, procname);
 		m_close(p);
 
-		lka_proc_forked(procname, subsystems, imsg->fd);
+		lka_proc_forked(procname, subsystems, imsg_get_fd(imsg));
 		return;
 
 	case IMSG_LKA_PROCESSOR_ERRFD:
@@ -359,8 +359,9 @@ lka_imsg(struct mproc *p, struct imsg *i
 		m_get_string(&m, &procname);
 		m_end(&m);
 
-		lka_proc_errfd(procname, imsg->fd);
-		shutdown(imsg->fd, SHUT_WR);
+		fd = imsg_get_fd(imsg);
+		lka_proc_errfd(procname, fd);
+		shutdown(fd, SHUT_WR);
 		return;
 
 	case IMSG_REPORT_SMTP_LINK_CONNECT:
Index: mda.c
===================================================================
RCS file: /cvs/src/usr.sbin/smtpd/mda.c,v
diff -u -p -r1.146 mda.c
--- mda.c	31 May 2023 16:51:46 -0000	1.146
+++ mda.c	16 Jan 2024 13:21:59 -0000
@@ -113,7 +113,7 @@ mda_imsg(struct mproc *p, struct imsg *i
 	uint64_t		 reqid;
 	size_t			 sz;
 	char			 out[256], buf[LINE_MAX];
-	int			 n;
+	int			 n, fd;
 	enum lka_resp_status	status;
 	enum mda_resp_status	mda_status;
 	int			mda_sysexit;
@@ -196,7 +196,8 @@ mda_imsg(struct mproc *p, struct imsg *i
 		s = tree_xget(&sessions, reqid);
 		e = s->evp;
 
-		if (imsg->fd == -1) {
+		fd = imsg_get_fd(imsg);
+		if (fd == -1) {
 			log_debug("debug: mda: cannot get message fd");
 			mda_queue_tempfail(e->id,
 			    "Cannot get message fd",
@@ -208,11 +209,11 @@ mda_imsg(struct mproc *p, struct imsg *i
 
 		log_debug("debug: mda: got message fd %d "
 		    "for session %016"PRIx64 " evpid %016"PRIx64,
-		    imsg->fd, s->id, e->id);
+		    fd, s->id, e->id);
 
-		if ((s->datafp = fdopen(imsg->fd, "r")) == NULL) {
+		if ((s->datafp = fdopen(fd, "r")) == NULL) {
 			log_warn("warn: mda: fdopen");
-			close(imsg->fd);
+			close(fd);
 			mda_queue_tempfail(e->id, "fdopen failed",
 			    ESC_OTHER_MAIL_SYSTEM_STATUS);
 			mda_log(e, "TempFail", "fdopen failed");
@@ -283,7 +284,8 @@ mda_imsg(struct mproc *p, struct imsg *i
 
 		s = tree_xget(&sessions, reqid);
 		e = s->evp;
-		if (imsg->fd == -1) {
+		fd = imsg_get_fd(imsg);
+		if (fd == -1) {
 			log_warn("warn: mda: fail to retrieve mda fd");
 			mda_queue_tempfail(e->id, "Cannot get mda fd",
 			    ESC_OTHER_MAIL_SYSTEM_STATUS);
@@ -294,10 +296,10 @@ mda_imsg(struct mproc *p, struct imsg *i
 
 		log_debug("debug: mda: got mda fd %d "
 		    "for session %016"PRIx64 " evpid %016"PRIx64,
-		    imsg->fd, s->id, s->evp->id);
+		    fd, s->id, s->evp->id);
 
-		io_set_nonblocking(imsg->fd);
-		io_set_fd(s->io, imsg->fd);
+		io_set_nonblocking(fd);
+		io_set_fd(s->io, fd);
 		io_set_write(s->io);
 		return;
 
@@ -315,8 +317,9 @@ mda_imsg(struct mproc *p, struct imsg *i
 		 * Grab last line of mda stdout/stderr if available.
 		 */
 		out[0] = '\0';
-		if (imsg->fd != -1)
-			mda_getlastline(imsg->fd, out, sizeof(out));
+		fd = imsg_get_fd(imsg);
+		if (fd != -1)
+			mda_getlastline(fd, out, sizeof(out));
 
 		/*
 		 * Choose between parent's description of error and
Index: mproc.c
===================================================================
RCS file: /cvs/src/usr.sbin/smtpd/mproc.c,v
diff -u -p -r1.39 mproc.c
--- mproc.c	14 Jun 2021 17:58:15 -0000	1.39
+++ mproc.c	16 Jan 2024 13:22:43 -0000
@@ -223,7 +223,7 @@ void
 m_forward(struct mproc *p, struct imsg *imsg)
 {
 	imsg_compose(&p->imsgbuf, imsg->hdr.type, imsg->hdr.peerid,
-	    imsg->hdr.pid, imsg->fd, imsg->data,
+	    imsg->hdr.pid, imsg_get_fd(imsg), imsg->data,
 	    imsg->hdr.len - sizeof(imsg->hdr));
 
 	if (imsg->hdr.type != IMSG_STAT_DECREMENT &&
Index: mta_session.c
===================================================================
RCS file: /cvs/src/usr.sbin/smtpd/mta_session.c,v
diff -u -p -r1.150 mta_session.c
--- mta_session.c	3 Jan 2024 08:11:15 -0000	1.150
+++ mta_session.c	16 Jan 2024 13:25:06 -0000
@@ -269,7 +269,7 @@ mta_session_imsg(struct mproc *p, struct
 	struct msg		 m;
 	uint64_t		 reqid;
 	const char		*name;
-	int			 status;
+	int			 status, fd;
 	struct stat		 sb;
 	
 	switch (imsg->hdr.type) {
@@ -279,14 +279,15 @@ mta_session_imsg(struct mproc *p, struct
 		m_get_id(&m, &reqid);
 		m_end(&m);
 
+		fd = imsg_get_fd(imsg);
 		s = mta_tree_pop(&wait_fd, reqid);
 		if (s == NULL) {
-			if (imsg->fd != -1)
-				close(imsg->fd);
+			if (fd != -1)
+				close(fd);
 			return;
 		}
 
-		if (imsg->fd == -1) {
+		if (fd == -1) {
 			log_debug("debug: mta: failed to obtain msg fd");
 			mta_flush_task(s, IMSG_MTA_DELIVERY_TEMPFAIL,
 			    "Could not get message fd", 0, 0);
@@ -295,12 +296,12 @@ mta_session_imsg(struct mproc *p, struct
 		}
 
 		if ((s->ext & MTA_EXT_SIZE) && s->ext_size != 0) {
-			if (fstat(imsg->fd, &sb) == -1) {
+			if (fstat(fd, &sb) == -1) {
 				log_debug("debug: mta: failed to stat msg fd");
 				mta_flush_task(s, IMSG_MTA_DELIVERY_TEMPFAIL,
 				    "Could not stat message fd", 0, 0);
 				mta_enter_state(s, MTA_READY);
-				close(imsg->fd);
+				close(fd);
 				return;
 			}
 			if (sb.st_size > (off_t)s->ext_size) {
@@ -308,12 +309,12 @@ mta_session_imsg(struct mproc *p, struct
 				mta_flush_task(s, IMSG_MTA_DELIVERY_PERMFAIL,
 				    "message too large for peer", 0, 0);
 				mta_enter_state(s, MTA_READY);
-				close(imsg->fd);
+				close(fd);
 				return;
 			}
 		}
 		
-		s->datafp = fdopen(imsg->fd, "r");
+		s->datafp = fdopen(fd, "r");
 		if (s->datafp == NULL)
 			fatal("mta: fdopen");
 
Index: queue.c
===================================================================
RCS file: /cvs/src/usr.sbin/smtpd/queue.c,v
diff -u -p -r1.195 queue.c
--- queue.c	31 May 2023 16:51:46 -0000	1.195
+++ queue.c	13 Dec 2023 13:21:18 -0000
@@ -126,7 +126,7 @@ queue_imsg(struct mproc *p, struct imsg 
 		return;
 
 	case IMSG_QUEUE_SMTP_SESSION:
-		bounce_fd(imsg->fd);
+		bounce_fd(imsg_get_fd(imsg));
 		return;
 
 	case IMSG_LKA_ENVELOPE_SUBMIT:
Index: queue_proc.c
===================================================================
RCS file: /cvs/src/usr.sbin/smtpd/queue_proc.c,v
diff -u -p -r1.9 queue_proc.c
--- queue_proc.c	14 Jun 2021 17:58:16 -0000	1.9
+++ queue_proc.c	13 Dec 2023 13:18:20 -0000
@@ -170,7 +170,7 @@ queue_proc_message_fd_r(uint32_t msgid)
 	queue_proc_call();
 	queue_proc_end();
 
-	return (imsg.fd);
+	return (imsg_get_fd(&imsg));
 }
 
 static int
Index: smtp_session.c
===================================================================
RCS file: /cvs/src/usr.sbin/smtpd/smtp_session.c,v
diff -u -p -r1.439 smtp_session.c
--- smtp_session.c	3 Jan 2024 08:11:15 -0000	1.439
+++ smtp_session.c	16 Jan 2024 13:27:47 -0000
@@ -702,7 +702,7 @@ smtp_session_imsg(struct mproc *p, struc
 	const char			*line, *helo;
 	uint64_t			 reqid, evpid;
 	uint32_t			 msgid;
-	int				 status, success;
+	int				 status, success, fd;
 	int                              filter_response;
 	const char                      *filter_param;
 	uint8_t                          i;
@@ -802,19 +802,20 @@ smtp_session_imsg(struct mproc *p, struc
 		m_get_int(&m, &success);
 		m_end(&m);
 
+		fd = imsg_get_fd(imsg);
 		s = tree_xpop(&wait_queue_fd, reqid);
-		if (!success || imsg->fd == -1) {
-			if (imsg->fd != -1)
-				close(imsg->fd);
+		if (!success || fd == -1) {
+			if (fd != -1)
+				close(fd);
 			smtp_reply(s, "421 %s Temporary Error",
 			    esc_code(ESC_STATUS_TEMPFAIL, ESC_OTHER_MAIL_SYSTEM_STATUS));
 			smtp_enter_state(s, STATE_QUIT);
 			return;
 		}
 
-		log_debug("smtp: %p: fd %d from queue", s, imsg->fd);
+		log_debug("smtp: %p: fd %d from queue", s, fd);
 
-		if (smtp_message_fd(s->tx, imsg->fd)) {
+		if (smtp_message_fd(s->tx, fd)) {
 			if (!SESSION_DATA_FILTERED(s))
 				smtp_message_begin(s->tx);
 			else
@@ -828,19 +829,20 @@ smtp_session_imsg(struct mproc *p, struc
 		m_get_int(&m, &success);
 		m_end(&m);
 
+		fd = imsg_get_fd(imsg);
 		s = tree_xpop(&wait_filter_fd, reqid);
-		if (!success || imsg->fd == -1) {
-			if (imsg->fd != -1)
-				close(imsg->fd);
+		if (!success || fd == -1) {
+			if (fd != -1)
+				close(fd);
 			smtp_reply(s, "421 %s Temporary Error",
 			    esc_code(ESC_STATUS_TEMPFAIL, ESC_OTHER_MAIL_SYSTEM_STATUS));
 			smtp_enter_state(s, STATE_QUIT);
 			return;
 		}
 
-		log_debug("smtp: %p: fd %d from lka", s, imsg->fd);
+		log_debug("smtp: %p: fd %d from lka", s, fd);
 
-		smtp_filter_fd(s->tx, imsg->fd);
+		smtp_filter_fd(s->tx, fd);
 		smtp_message_begin(s->tx);
 		return;
 
Index: smtpd.c
===================================================================
RCS file: /cvs/src/usr.sbin/smtpd/smtpd.c,v
diff -u -p -r1.346 smtpd.c
--- smtpd.c	18 Jun 2023 17:28:42 -0000	1.346
+++ smtpd.c	16 Jan 2024 13:30:58 -0000
@@ -917,7 +917,8 @@ setup_proc(void)
 			env->sc_queue_key = strdup(imsg.data);
 			break;
 		case IMSG_SETUP_PEER:
-			setup_peer(imsg.hdr.peerid, imsg.hdr.pid, imsg.fd);
+			setup_peer(imsg.hdr.peerid, imsg.hdr.pid,
+			    imsg_get_fd(&imsg));
 			break;
 		case IMSG_SETUP_DONE:
 			setup = 0;
@@ -1866,19 +1867,11 @@ log_imsg(int to, int from, struct imsg *
 	if (to == PROC_CONTROL && imsg->hdr.type == IMSG_STAT_SET)
 		return;
 
-	if (imsg->fd != -1)
-		log_trace(TRACE_IMSG, "imsg: %s <- %s: %s (len=%zu, fd=%d)",
-		    proc_name(to),
-		    proc_name(from),
-		    imsg_to_str(imsg->hdr.type),
-		    imsg->hdr.len - IMSG_HEADER_SIZE,
-		    imsg->fd);
-	else
-		log_trace(TRACE_IMSG, "imsg: %s <- %s: %s (len=%zu)",
-		    proc_name(to),
-		    proc_name(from),
-		    imsg_to_str(imsg->hdr.type),
-		    imsg->hdr.len - IMSG_HEADER_SIZE);
+	log_trace(TRACE_IMSG, "imsg: %s <- %s: %s (len=%zu)",
+	    proc_name(to),
+	    proc_name(from),
+	    imsg_to_str(imsg->hdr.type),
+	    imsg->hdr.len - IMSG_HEADER_SIZE);
 }
 
 const char *