The recent fix to handle large answers in unwind (errata #5 for 6.8)

has the downside to always copy the maximum IMSG size (about 16k)
between the resolver and frontend process for DNS answers because
we had to keep it as simple as possible.
We can now rearange things in -current to be less wasteful. This copies
only the usually small DNS answer.
In the unusual case that a DNS answer is larger than the maximum IMSG size
fragment the message and send multiple IMSGs.

3 files changed, 81 insertions, 83 deletions

diff --git a/sbin/unwind/frontend.c b/sbin/unwind/frontend.c
index 8adbb07219b..2165b5c0d8c 100644
--- a/sbin/unwind/frontend.c
+++ b/sbin/unwind/frontend.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: frontend.c,v 1.56 2020/11/09 04:22:05 tb Exp $	*/
+/*	$OpenBSD: frontend.c,v 1.57 2020/12/11 16:36:03 florian Exp $	*/
 
 /*
  * Copyright (c) 2018 Florian Obser <florian@openbsd.org>
@@ -86,7 +86,8 @@ struct pending_query {
 	int				 fd;
 	int				 bogus;
 	int				 rcode_override;
-	ssize_t				 answer_len;
+	int				 answer_len;
+	int				 received;
 	uint8_t				*answer;
 };
 
@@ -424,10 +425,7 @@ frontend_dispatch_resolver(int fd, short event, void *bula)
 	struct imsgev			*iev = bula;
 	struct imsgbuf			*ibuf = &iev->ibuf;
 	struct imsg			 imsg;
-	struct query_imsg		*query_imsg;
-	struct answer_imsg		*answer_imsg;
 	int				 n, shut = 0, chg;
-	uint8_t				*p;
 
 	if (event & EV_READ) {
 		if ((n = imsg_read(ibuf)) == -1 && errno != EAGAIN)
@@ -449,45 +447,30 @@ frontend_dispatch_resolver(int fd, short event, void *bula)
 			break;
 
 		switch (imsg.hdr.type) {
-		case IMSG_ANSWER_HEADER:
-			if (IMSG_DATA_SIZE(imsg) != sizeof(*query_imsg))
-				fatalx("%s: IMSG_ANSWER_HEADER wrong length: "
-				    "%lu", __func__, IMSG_DATA_SIZE(imsg));
-			query_imsg = (struct query_imsg *)imsg.data;
-			if ((pq = find_pending_query(query_imsg->id)) ==
-			    NULL) {
-				log_warnx("cannot find pending query %llu",
-				    query_imsg->id);
-				break;
-			}
-			if (query_imsg->err) {
-				send_answer(pq);
-				pq = NULL;
-				break;
-			}
-			pq->bogus = query_imsg->bogus;
-			break;
-		case IMSG_ANSWER:
-			if (IMSG_DATA_SIZE(imsg) != sizeof(*answer_imsg))
+		case IMSG_ANSWER: {
+			struct answer_header	*answer_header;
+			int			 data_len;
+			uint8_t			*data;
+
+			if (IMSG_DATA_SIZE(imsg) < sizeof(*answer_header))
 				fatalx("%s: IMSG_ANSWER wrong length: "
 				    "%lu", __func__, IMSG_DATA_SIZE(imsg));
-			answer_imsg = (struct answer_imsg *)imsg.data;
-			if ((pq = find_pending_query(answer_imsg->id)) ==
+			answer_header = (struct answer_header *)imsg.data;
+			data = (uint8_t *)imsg.data + sizeof(*answer_header);
+			if (answer_header->answer_len > 65535)
+				fatalx("%s: IMSG_ANSWER answer too big: %d",
+				    __func__, answer_header->answer_len);
+			data_len = IMSG_DATA_SIZE(imsg) -
+			    sizeof(*answer_header);
+
+			if ((pq = find_pending_query(answer_header->id)) ==
 			    NULL) {
-				log_warnx("cannot find pending query %llu",
-				    answer_imsg->id);
+				log_warnx("%s: cannot find pending query %llu",
+				    __func__, answer_header->id);
 				break;
 			}
 
-			p = realloc(pq->answer, pq->answer_len +
-			    answer_imsg->len);
-
-			if (p != NULL) {
-				pq->answer = p;
-				memcpy(pq->answer + pq->answer_len,
-				    answer_imsg->answer, answer_imsg->len);
-				pq->answer_len += answer_imsg->len;
-			} else {
+			if (answer_header->srvfail) {
 				free(pq->answer);
 				pq->answer_len = 0;
 				pq->answer = NULL;
@@ -495,9 +478,32 @@ frontend_dispatch_resolver(int fd, short event, void *bula)
 				send_answer(pq);
 				break;
 			}
-			if (!answer_imsg->truncated)
+
+			if (pq->answer == NULL) {
+				pq->answer = malloc(answer_header->answer_len);
+				if (pq->answer == NULL) {
+					pq->answer_len = 0;
+					pq->rcode_override =
+					    LDNS_RCODE_SERVFAIL;
+					send_answer(pq);
+					break;
+				}
+				pq->answer_len = answer_header->answer_len;
+				pq->received = 0;
+				pq->bogus = answer_header->bogus;
+			}
+
+			if (pq->received + data_len > pq->answer_len)
+				fatalx("%s: IMSG_ANSWER answer too big: %d",
+				    __func__, data_len);
+
+			memcpy(pq->answer + pq->received, data, data_len);
+			pq->received += data_len;
+
+			if (pq->received == pq->answer_len)
 				send_answer(pq);
 			break;
+		}
 		case IMSG_CTL_RESOLVER_INFO:
 		case IMSG_CTL_AUTOCONF_RESOLVER_INFO:
 		case IMSG_CTL_MEM_INFO:
diff --git a/sbin/unwind/resolver.c b/sbin/unwind/resolver.c
index fb34dee87d6..06d106cea93 100644
--- a/sbin/unwind/resolver.c
+++ b/sbin/unwind/resolver.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: resolver.c,v 1.126 2020/11/05 16:22:59 florian Exp $	*/
+/*	$OpenBSD: resolver.c,v 1.127 2020/12/11 16:36:03 florian Exp $	*/
 
 /*
  * Copyright (c) 2018 Florian Obser <florian@openbsd.org>
@@ -884,7 +884,7 @@ resolve_done(struct uw_resolver *res, void *arg, int rcode,
 	sldns_buffer		*buf = NULL;
 	struct regional		*region = NULL;
 	struct query_imsg	*query_imsg;
-	struct answer_imsg	 answer_imsg;
+	struct answer_header	*answer_header;
 	struct running_query	*rq;
 	struct timespec		 tp, elapsed;
 	int64_t			 ms;
@@ -894,12 +894,19 @@ resolve_done(struct uw_resolver *res, void *arg, int rcode,
 	char			 rcode_buf[16];
 	char			 qclass_buf[16];
 	char			 qtype_buf[16];
-	uint8_t			*p;
+	uint8_t			*p, *data;
+	uint8_t			 answer_imsg[MAX_IMSGSIZE - IMSG_HEADER_SIZE];
 
 	clock_gettime(CLOCK_MONOTONIC, &tp);
 
 	query_imsg = (struct query_imsg *)arg;
 
+	answer_header = (struct answer_header *)answer_imsg;
+	data = answer_imsg + sizeof(*answer_header);
+	answer_header->id = query_imsg->id;
+	answer_header->srvfail = 0;
+	answer_header->answer_len = answer_len;
+
 	timespecsub(&tp, &query_imsg->tp, &elapsed);
 
 	ms = elapsed.tv_sec * 1000 + elapsed.tv_nsec / 1000000;
@@ -1004,43 +1011,33 @@ resolve_done(struct uw_resolver *res, void *arg, int rcode,
 	if (result->rcode == LDNS_RCODE_SERVFAIL)
 		goto servfail;
 
-	query_imsg->err = 0;
-
 	if (sec == SECURE)
 		res->state = VALIDATING;
 
 	if (res->state == VALIDATING && sec == BOGUS) {
-		query_imsg->bogus = !force_acceptbogus;
-		if (query_imsg->bogus && why_bogus != NULL)
+		answer_header->bogus = !force_acceptbogus;
+		if (answer_header->bogus && why_bogus != NULL)
 			log_warnx("%s", why_bogus);
 	} else
-		query_imsg->bogus = 0;
+		answer_header->bogus = 0;
 
-	if (resolver_imsg_compose_frontend(IMSG_ANSWER_HEADER, 0, query_imsg,
-	    sizeof(*query_imsg)) == -1)
-		fatalx("IMSG_ANSWER_HEADER failed for \"%s %s %s\"",
-		    query_imsg->qname, qclass_buf, qtype_buf);
-
-	answer_imsg.id = query_imsg->id;
 	p = answer_packet;
-	while ((size_t)answer_len > MAX_ANSWER_SIZE) {
-		answer_imsg.truncated = 1;
-		answer_imsg.len = MAX_ANSWER_SIZE;
-		memcpy(&answer_imsg.answer, p, MAX_ANSWER_SIZE);
-		if (resolver_imsg_compose_frontend(IMSG_ANSWER, 0, &answer_imsg,
-		    sizeof(answer_imsg)) == -1)
+	do {
+		int len;
+
+		if ((size_t)answer_len > sizeof(answer_imsg) -
+		    sizeof(*answer_header))
+			len = sizeof(answer_imsg) - sizeof(*answer_header);
+		else
+			len = answer_len;
+		memcpy(data, p, len);
+		if (resolver_imsg_compose_frontend(IMSG_ANSWER, 0,
+		    &answer_imsg, sizeof(*answer_header) + len) == -1)
 			fatalx("IMSG_ANSWER failed for \"%s %s %s\"",
 			    query_imsg->qname, qclass_buf, qtype_buf);
-		p += MAX_ANSWER_SIZE;
-		answer_len -= MAX_ANSWER_SIZE;
-	}
-	answer_imsg.truncated = 0;
-	answer_imsg.len = answer_len;
-	memcpy(&answer_imsg.answer, p, answer_len);
-	if (resolver_imsg_compose_frontend(IMSG_ANSWER, 0, &answer_imsg,
-	    sizeof(answer_imsg)) == -1)
-		fatalx("IMSG_ANSWER failed for \"%s %s %s\"",
-		    query_imsg->qname, qclass_buf, qtype_buf);
+		answer_len -= len;
+		p += len;
+	} while (answer_len > 0);
 
 	TAILQ_REMOVE(&running_queries, rq, entry);
 	evtimer_del(&rq->timer_ev);
@@ -1052,9 +1049,9 @@ resolve_done(struct uw_resolver *res, void *arg, int rcode,
 	/* try_next_resolver() might free rq */
 	if (try_next_resolver(rq) != 0 && running_res == 0) {
 		/* we are the last one, send SERVFAIL */
-		query_imsg->err = -4; /* UB_SERVFAIL */
-		resolver_imsg_compose_frontend(IMSG_ANSWER_HEADER, 0,
-		    query_imsg, sizeof(*query_imsg));
+		answer_header->srvfail = 1;
+		resolver_imsg_compose_frontend(IMSG_ANSWER, 0,
+		    answer_imsg, sizeof(*answer_header));
 	}
  out:
 	free(query_imsg);
diff --git a/sbin/unwind/unwind.h b/sbin/unwind/unwind.h
index 9ebd8f47f87..659d94639e9 100644
--- a/sbin/unwind/unwind.h
+++ b/sbin/unwind/unwind.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: unwind.h,v 1.48 2020/11/05 16:22:59 florian Exp $	*/
+/*	$OpenBSD: unwind.h,v 1.49 2020/12/11 16:36:03 florian Exp $	*/
 
 /*
  * Copyright (c) 2018 Florian Obser <florian@openbsd.org>
@@ -116,7 +116,6 @@ enum imsg_type {
 	IMSG_SOCKET_IPC_FRONTEND,
 	IMSG_SOCKET_IPC_RESOLVER,
 	IMSG_QUERY,
-	IMSG_ANSWER_HEADER,
 	IMSG_ANSWER,
 	IMSG_CTL_RESOLVER_INFO,
 	IMSG_CTL_AUTOCONF_RESOLVER_INFO,
@@ -170,18 +169,14 @@ struct query_imsg {
 	char		 qname[NI_MAXHOST];
 	int		 t;
 	int		 c;
-	int		 err;
-	int		 bogus;
 	struct timespec	 tp;
 };
 
-struct answer_imsg {
-#define	MAX_ANSWER_SIZE	MAX_IMSGSIZE - IMSG_HEADER_SIZE - sizeof(uint64_t) - \
-			    2 * sizeof(int)
-	uint64_t	 id;
-	int		 truncated;
-	int		 len;
-	uint8_t		 answer[MAX_ANSWER_SIZE];
+struct answer_header {
+	uint64_t id;
+	int	 srvfail;
+	int	 bogus;
+	int	 answer_len;
 };
 
 extern uint32_t	 cmd_opts;