Anyone?
While I could volunteer all y'all for testing that seems a bit silly.
I'd rather have someone to read the diff because
- The tricky cases will be very rare to hit in normal operations,
DNS answers are just not that big.
- I'd like to hear that this is actually a good pattern to do this and
I'm not completely off the trail
Eventually I will put it in though, to make progress.
Thanks.
On Thu, Nov 19, 2020 at 03:59:03PM +0100, Florian Obser wrote:
> On Fri, Nov 13, 2020 at 05:53:53PM +0100, Florian Obser wrote:
> > The recent fix for handling large (about 16k) answers in unwind has
> > the downside that we are now always copying at least 16k per answer
> > between the resolver and frontend process.
> > That seems wasteful.
> >
> > This re-arranges things to only copy what its needed.
> >
> > Tests, OKs?
> >
>
> Anyone? So far I had one (1) test report.
>
> diff --git frontend.c frontend.c
> index 8adbb07219b..74715087c5f 100644
> --- frontend.c
> +++ frontend.c
> @@ -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 resolver.c resolver.c
> index fb34dee87d6..c3faba08faf 100644
> --- resolver.c
> +++ resolver.c
> @@ -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 unwind.h unwind.h
> index 9ebd8f47f87..161619cd34c 100644
> --- unwind.h
> +++ unwind.h
> @@ -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;
>
>
> --
> I'm not entirely sure you are real.
>
--
I'm not entirely sure you are real.
