From 7780a86b36a26a50ae51d1c52249f6550dfac7f6 Mon Sep 17 00:00:00 2001
From: Claudio Jeker <claudio@cvs.openbsd.org>
Date: Thu, 21 Nov 2024 13:25:02 +0000
Subject: Try to handle the dumpster fire called constraint.c a bit better.

The imsg handling in here is not quite right. It works but more by luck.
- use imsgbuf_read_one (similar code as found in a few other places)
  to read the one message we expect.
- do not call imsgbuf_flush() without a child running to read the data.
  With large enough requests imsgbuf_flush() may be locked forever since
  there is no reader on the other side of the pipe.

OK tb@
---
 usr.sbin/ntpd/constraint.c | 45 +++++++++++++++++++++++++++++++++------------
 1 file changed, 33 insertions(+), 12 deletions(-)

(limited to 'usr.sbin/ntpd')

diff --git a/usr.sbin/ntpd/constraint.c b/usr.sbin/ntpd/constraint.c
index b994c02cf6b..993541f9381 100644
--- a/usr.sbin/ntpd/constraint.c
+++ b/usr.sbin/ntpd/constraint.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: constraint.c,v 1.58 2024/11/21 13:17:57 claudio Exp $	*/
+/*	$OpenBSD: constraint.c,v 1.59 2024/11/21 13:25:01 claudio Exp $	*/
 
 /*
  * Copyright (c) 2015 Reyk Floeter <reyk@openbsd.org>
@@ -274,15 +274,37 @@ priv_constraint_msg(u_int32_t id, u_int8_t *data, size_t len, int argc,
 	if (imsg_compose(&cstr->ibuf, IMSG_CONSTRAINT_QUERY, id, 0, -1,
 	    data, len) == -1)
 		fatal("%s: imsg_compose", __func__);
-	if (imsgbuf_flush(&cstr->ibuf) == -1)
-		fatal("imsgbuf_flush");
-
 	/*
 	 * Fork child handlers and make sure to do any sensitive work in the
 	 * the (unprivileged) child.  The parent should not do any parsing,
 	 * certificate loading etc.
 	 */
 	cstr->pid = start_child(CONSTRAINT_PROC_NAME, pipes[1], argc, argv);
+
+	if (imsgbuf_flush(&cstr->ibuf) == -1)
+		fatal("imsgbuf_flush");
+}
+
+static int
+imsgbuf_read_one(struct imsgbuf *imsgbuf, struct imsg *imsg)
+{
+	while (1) {
+		switch (imsg_get(imsgbuf, imsg)) {
+		case -1:
+			return (-1);
+		case 0:
+			break;
+		default:
+			return (1);
+		}
+
+		switch (imsgbuf_read(imsgbuf)) {
+		case -1:
+			return (-1);
+		case 0:
+			return (0);
+		}
+	}
 }
 
 void
@@ -291,16 +313,16 @@ priv_constraint_readquery(struct constraint *cstr, struct ntp_addr_msg *am,
 {
 	struct ntp_addr		*h;
 	uint8_t			*dptr;
-	int			 n;
 	struct imsg		 imsg;
 	size_t			 mlen;
 
 	/* Read the message our parent left us. */
-	if (((n = imsgbuf_read(&cstr->ibuf)) == -1 && errno != EAGAIN) ||
-	    n == 0)
-		fatal("%s: imsgbuf_read", __func__);
-	if (((n = imsg_get(&cstr->ibuf, &imsg)) == -1) || n == 0)
-		fatal("%s: imsg_get", __func__);
+	switch (imsgbuf_read_one(&cstr->ibuf, &imsg)) {
+	case -1:
+		fatal("%s: imsgbuf_read_one", __func__);
+	case 0:
+		fatalx("%s: imsgbuf_read_one: connection closed", __func__);
+	}
 	if (imsg.hdr.type != IMSG_CONSTRAINT_QUERY)
 		fatalx("%s: invalid message type", __func__);
 
@@ -613,8 +635,7 @@ priv_constraint_dispatch(struct pollfd *pfd)
 	if (!(pfd->revents & POLLIN))
 		return (0);
 
-	if (((n = imsgbuf_read(&cstr->ibuf)) == -1 && errno != EAGAIN) ||
-	    n == 0) {
+	if (imsgbuf_read(&cstr->ibuf) != 1) {
 		/* there's a race between SIGCHLD delivery and reading imsg
 		   but if we've seen the reply, we're good */
 		priv_constraint_close(pfd->fd, cstr->state !=
-- 
cgit v1.2.3