Hello all,

One eyesore for a long time for me is the recvfd pledge inside snmpe.c.
This one is only there for the creating of socketpairs between multiple
children. While strictly speaking not needed anymore since the removal
of the traphandler process, the structure of proc.c kind of enforces
its presence since proc_run() calls run() (snmpe_init()) right before
calling event_dispatch(), where the socketpair fds are send through
here. Since there's no callback after all socketpairs have been send
it's impossible to remove the recvfd pledge at a trustworthy point.

Looking at the other proc.c versions, I found that iked's version has
support for IMSG_CTL_PROCREADY, although with a slightly different
approach (a finalize function in PROC_PARENT after all children have
answered). The diff below pulls in iked's proc.c with the following
changes:
- snmpd doesn't have a control process, so remove those calls (this
  was already removed from snmpd's proc.c)
- Make proc_connect's conntected callback NULL-able. snmpd doesn't
  need it.
- store proc_run's run() and arg inside struct privsep_proc, and
  delay their calls until IMSG_CTL_PROCREADY has been received.
  This way we know that all socketpair's have been received.

Thoughts? OK?

martijn@

diff /usr/src
path + /usr/src
commit - 0377483fd63d89703211981e944e9e01c48a851d
blob - bbf7335275bd1a95037ce8a1ff46e55dded65db9
file + usr.sbin/snmpd/proc.c
--- usr.sbin/snmpd/proc.c
+++ usr.sbin/snmpd/proc.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: proc.c,v 1.39 2024/11/21 13:38:45 claudio Exp $	*/
+/*	$OpenBSD: proc.c,v 1.51 2024/11/21 13:35:20 claudio Exp $	*/
 
 /*
  * Copyright (c) 2010 - 2016 Reyk Floeter <reyk@openbsd.org>
@@ -17,13 +17,14 @@
  * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
  */
 
+#include <sys/types.h>
+#include <sys/queue.h>
 #include <sys/socket.h>
 #include <sys/wait.h>
 
 #include <fcntl.h>
 #include <stdio.h>
 #include <stdlib.h>
-#include <stdint.h>
 #include <unistd.h>
 #include <string.h>
 #include <errno.h>
@@ -36,6 +37,8 @@
 #include "log.h"
 #include "snmpd.h"
 
+enum privsep_procid privsep_process;
+
 void	 proc_exec(struct privsep *, struct privsep_proc *, unsigned int, int,
 	    char **);
 void	 proc_setup(struct privsep *, struct privsep_proc *, unsigned int);
@@ -139,14 +142,18 @@ proc_exec(struct privsep *ps, struct privsep_proc *pro
 }
 
 void
-proc_connect(struct privsep *ps)
+proc_connect(struct privsep *ps, void (*connected)(struct privsep *))
 {
 	struct imsgev		*iev;
 	unsigned int		 src, dst, inst;
 
 	/* Don't distribute any sockets if we are not really going to run. */
-	if (ps->ps_noaction)
+	if (ps->ps_noaction) {
+		if (connected != NULL)
+			connected(ps);
 		return;
+	}
+	ps->ps_connected = connected;
 
 	for (dst = 0; dst < PROC_MAX; dst++) {
 		/* We don't communicate with ourselves. */
@@ -157,7 +164,7 @@ proc_connect(struct privsep *ps)
 			iev = &ps->ps_ievs[dst][inst];
 			if (imsgbuf_init(&iev->ibuf,
 			    ps->ps_pp->pp_pipes[dst][inst]) == -1)
-				fatal("imsgbuf_init");
+				fatal("%s: imsgbuf_init", __func__);
 			imsgbuf_allow_fdpass(&iev->ibuf);
 			event_set(&iev->ev, iev->ibuf.fd, iev->events,
 			    iev->handler, iev->data);
@@ -174,6 +181,27 @@ proc_connect(struct privsep *ps)
 
 			proc_open(ps, src, dst);
 		}
+
+	/*
+	 * Finally, send a ready message to everyone:
+	 * When this message is processed by the receiver, it has
+	 * already processed all IMSG_CTL_PROCFD messages and all
+	 * pipes are ready.
+	 */
+	for (dst = 0; dst < PROC_MAX; dst++) {
+		if (dst == PROC_PARENT)
+			continue;
+		for (inst = 0; inst < ps->ps_instances[dst]; inst++) {
+			if (proc_compose_imsg(ps, dst, inst, IMSG_CTL_PROCREADY,
+			    -1, -1, NULL, 0) == -1)
+				fatal("%s: proc_compose_imsg", __func__);
+			ps->ps_connecting++;
+#if DEBUG
+			log_debug("%s: #%d %s %d", __func__,
+			    ps->ps_connecting, ps->ps_title[dst], inst + 1);
+#endif
+		}
+	}
 }
 
 void
@@ -268,7 +296,7 @@ proc_accept(struct privsep *ps, int fd, enum privsep_p
 
 	iev = &ps->ps_ievs[dst][n];
 	if (imsgbuf_init(&iev->ibuf, fd) == -1)
-		fatal("imsgbuf_init");
+		fatal("%s: imsgbuf_init", __func__);
 	imsgbuf_allow_fdpass(&iev->ibuf);
 	event_set(&iev->ev, iev->ibuf.fd, iev->events, iev->handler, iev->data);
 	event_add(&iev->ev, NULL);
@@ -553,11 +581,14 @@ proc_run(struct privsep *ps, struct privsep_proc *p,
 
 	proc_setup(ps, procs, nproc);
 	proc_accept(ps, PROC_PARENT_SOCK_FILENO, PROC_PARENT, 0);
-	DPRINTF("%s: %s %d/%d, pid %d", __func__, p->p_title,
+
+#if DEBUG
+	log_debug("%s: %s %d/%d, pid %d", __func__, p->p_title,
 	    ps->ps_instance + 1, ps->ps_instances[p->p_id], getpid());
+#endif
 
-	if (run != NULL)
-		run(ps, p, arg);
+	p->p_run = run;
+	p->p_arg = arg;
 
 	event_dispatch();
 
@@ -593,13 +624,12 @@ proc_dispatch(int fd, short event, void *arg)
 
 	if (event & EV_WRITE) {
 		if (imsgbuf_write(ibuf) == -1) {
-			if (errno == EPIPE) {
-				/* this pipe is dead, remove the handler */
+			if (errno == EPIPE) {	/* Connection closed. */
 				event_del(&iev->ev);
 				event_loopexit(NULL);
 				return;
-			}
-			fatal("%s: imsgbuf_write", __func__);
+			} else
+				fatal("imsgbuf_write");
 		}
 	}
 
@@ -639,6 +669,36 @@ proc_dispatch(int fd, short event, void *arg)
 			proc_accept(ps, imsg_get_fd(&imsg), pf.pf_procid,
 			    pf.pf_instance);
 			break;
+		case IMSG_CTL_PROCREADY:
+#if DEBUG
+			log_debug("%s: ready-%s: #%d %s %d -> %s %d", __func__,
+			    p->p_id == PROC_PARENT ? "req" : "ack",
+			    ps->ps_connecting, p->p_title, imsg.hdr.pid,
+			    title, ps->ps_instance + 1);
+#endif
+			if (p->p_id == PROC_PARENT) {
+				/* ack that we are ready */
+				if (proc_compose_imsg(ps, PROC_PARENT, 0,
+				    IMSG_CTL_PROCREADY, -1, -1, NULL, 0) == -1)
+					fatal("%s: proc_compose_imsg", __func__);
+				if (p->p_run != NULL) {
+					p->p_run(ps, p, p->p_arg);
+					p->p_run = NULL;
+				}
+			} else {
+				/* parent received ack */
+				if (ps->ps_connecting == 0)
+					fatalx("%s: wrong acks", __func__);
+				if (ps->ps_instance != 0)
+					fatalx("%s: wrong instance %d",
+					    __func__, ps->ps_instance);
+				if (--ps->ps_connecting == 0) {
+					log_debug("%s: all connected", __func__);
+					if (ps->ps_connected != NULL)
+						ps->ps_connected(ps);
+				}
+			}
+			break;
 		default:
 			fatalx("%s: %s %d got invalid imsg %d peerid %d "
 			    "from %s %d",
@@ -762,6 +822,14 @@ proc_composev(struct privsep *ps, enum privsep_procid 
 	return (proc_composev_imsg(ps, id, -1, type, -1, -1, iov, iovcnt));
 }
 
+int
+proc_forward_imsg(struct privsep *ps, struct imsg *imsg,
+    enum privsep_procid id, int n)
+{
+	return (proc_compose_imsg(ps, id, n, imsg->hdr.type,
+	    imsg->hdr.peerid, -1, imsg->data, IMSG_DATA_SIZE(imsg)));
+}
+
 struct imsgbuf *
 proc_ibuf(struct privsep *ps, enum privsep_procid id, int n)
 {
commit - 0377483fd63d89703211981e944e9e01c48a851d
blob - 60af191fb6f00ff295542f65ed9c733b66038830
file + usr.sbin/snmpd/snmpd.c
--- usr.sbin/snmpd/snmpd.c
+++ usr.sbin/snmpd/snmpd.c
@@ -46,8 +46,6 @@ static struct privsep_proc procs[] = {
 	{ "snmpe", PROC_SNMPE, snmpd_dispatch_snmpe, snmpe, snmpe_shutdown },
 };
 
-enum privsep_procid privsep_process;
-
 void
 snmpd_sig_handler(int sig, short event, void *arg)
 {
@@ -242,7 +240,7 @@ main(int argc, char *argv[])
 	signal_add(&ps->ps_evsigpipe, NULL);
 	signal_add(&ps->ps_evsigusr1, NULL);
 
-	proc_connect(ps);
+	proc_connect(ps, NULL);
 	snmpd_backend(env);
 
 	if (pledge("stdio dns sendfd proc exec id", NULL) == -1)
commit - 0377483fd63d89703211981e944e9e01c48a851d
blob - bc1b9ff7d6b8fab49046e4fe3bec9ba5fca31a35
file + usr.sbin/snmpd/snmpd.h
--- usr.sbin/snmpd/snmpd.h
+++ usr.sbin/snmpd/snmpd.h
@@ -90,6 +90,7 @@ enum imsg_type {
 	IMSG_NONE,
 	IMSG_CTL_VERBOSE,
 	IMSG_CTL_PROCFD,
+	IMSG_CTL_PROCREADY,
 	IMSG_TRAP_EXEC,
 	IMSG_AX_FD
 };
@@ -144,6 +145,8 @@ struct privsep {
 	struct event		 ps_evsigusr1;
 
 	void			*ps_env;
+	unsigned int		 ps_connecting;
+	void			(*ps_connected)(struct privsep *);
 };
 
 struct privsep_proc {
@@ -157,6 +160,11 @@ struct privsep_proc {
 	const char		*p_chroot;
 	struct privsep		*p_ps;
 	struct passwd		*p_pw;
+
+	/* private */
+	void			(*p_run)(struct privsep *,
+				    struct privsep_proc *, void *);
+	void			*p_arg;
 };
 
 struct privsep_fd {
@@ -488,7 +496,7 @@ enum privsep_procid
 void	 proc_init(struct privsep *, struct privsep_proc *, unsigned int, int,
 	    int, char **, enum privsep_procid);
 void	 proc_kill(struct privsep *);
-void	 proc_connect(struct privsep *);
+void	 proc_connect(struct privsep *, void (*connected)(struct privsep *));
 void	 proc_dispatch(int, short event, void *);
 void	 proc_run(struct privsep *, struct privsep_proc *,
 	    struct privsep_proc *, u_int,
@@ -507,6 +515,8 @@ int	 proc_composev_imsg(struct privsep *, enum privsep
 	    u_int16_t, u_int32_t, int, const struct iovec *, int);
 int	 proc_composev(struct privsep *, enum privsep_procid,
 	    uint16_t, const struct iovec *, int);
+int	 proc_forward_imsg(struct privsep *, struct imsg *,
+	    enum privsep_procid, int);
 struct imsgbuf *
 	 proc_ibuf(struct privsep *, enum privsep_procid, int);
 struct imsgev *
commit - 0377483fd63d89703211981e944e9e01c48a851d
blob - d444c8d9b8a47d7da9242e9372d1b88241dece5b
file + usr.sbin/snmpd/snmpe.c
--- usr.sbin/snmpd/snmpe.c
+++ usr.sbin/snmpd/snmpe.c
@@ -110,7 +110,7 @@ snmpe_init(struct privsep *ps, struct privsep_proc *p,
 	/* no filesystem visibility */
 	if (unveil("/", "") == -1)
 		fatal("unveil /");
-	if (pledge("stdio recvfd inet unix", NULL) == -1)
+	if (pledge("stdio inet unix", NULL) == -1)
 		fatal("pledge");
 
 	log_info("snmpe %s: ready",