From 3ae073ab82183075a93e71ed889e0ee4fdb780e6 Mon Sep 17 00:00:00 2001 From: Simon Kelley Date: Wed, 17 Feb 2021 23:56:32 +0000 Subject: [PATCH] Fix problem with DNS retries in 2.83/2.84. The new logic in 2.83/2.84 which merges distinct requests for the same domain causes problems with clients which do retries as distinct requests (differing IDs and/or source ports.) The retries just get piggy-backed on the first, failed, request. The logic is now changed so that distinct requests for repeated queries still get merged into a single ID/source port, but they now always trigger a re-try upstream. Thanks to Nicholas Mu for his analysis. (cherry picked from commit 141a26f979b4bc959d8e866a295e24f8cf456920) Simplify preceding fix. Remove distinction between retry with same QID/SP and retry for same query with different QID/SP. If the QID/SP are the same as an existing one, simply retry, if a new QID/SP is seen, add to the list to be replied to. (cherry picked from commit 305cb79c5754d5554729b18a2c06fe7ce699687a) --- src/forward.c | 105 ++++++++++++++++++++++---------------------------- 1 file changed, 45 insertions(+), 60 deletions(-) diff --git a/src/forward.c b/src/forward.c index 637f220..1e9c509 100644 --- a/src/forward.c +++ b/src/forward.c @@ -17,9 +17,6 @@ #include "dnsmasq.h" static struct frec *lookup_frec(unsigned short id, int fd, int family, void *hash); -static struct frec *lookup_frec_by_sender(unsigned short id, - union mysockaddr *addr, - void *hash); static struct frec *lookup_frec_by_query(void *hash, unsigned int flags); static unsigned short get_id(void); @@ -260,8 +257,48 @@ static int forward_query(int udpfd, union mysockaddr *udpaddr, fwd_flags |= FREC_DO_QUESTION; #endif - /* may be no servers available. */ - if (forward || (forward = lookup_frec_by_sender(ntohs(header->id), udpaddr, hash))) + /* Check for retry on existing query */ + if (!forward && (forward = lookup_frec_by_query(hash, fwd_flags))) + { + struct frec_src *src; + + for (src = &forward->frec_src; src; src = src->next) + if (src->orig_id == ntohs(header->id) && + sockaddr_isequal(&src->source, udpaddr)) + break; + + /* Existing query, but from new source, just add this + client to the list that will get the reply.*/ + if (!src) + { + /* Note whine_malloc() zeros memory. */ + if (!daemon->free_frec_src && + daemon->frec_src_count < daemon->ftabsize && + (daemon->free_frec_src = whine_malloc(sizeof(struct frec_src)))) + { + daemon->frec_src_count++; + daemon->free_frec_src->next = NULL; + } + + /* If we've been spammed with many duplicates, just drop the query. */ + if (!daemon->free_frec_src) + return 0; + + src = daemon->free_frec_src; + daemon->free_frec_src = src->next; + src->next = forward->frec_src.next; + forward->frec_src.next = src; + src->orig_id = ntohs(header->id); + src->source = *udpaddr; + src->dest = *dst_addr; + src->log_id = daemon->log_id; + src->iface = dst_iface; + src->fd = udpfd; + } + } + + /* retry existing query */ + if (forward) { /* If we didn't get an answer advertising a maximal packet in EDNS, fall back to 1280, which should work everywhere on IPv6. @@ -336,42 +373,8 @@ static int forward_query(int udpfd, union mysockaddr *udpaddr, } else { - /* Query from new source, but the same query may be in progress - from another source. If so, just add this client to the - list that will get the reply. - - Note that is the EDNS client subnet option is in use, we can't do this, - as the clients (and therefore query EDNS options) will be different - for each query. The EDNS subnet code has checks to avoid - attacks in this case. */ - if (!option_bool(OPT_CLIENT_SUBNET) && (forward = lookup_frec_by_query(hash, fwd_flags))) - { - if (!daemon->free_frec_src && - daemon->frec_src_count < daemon->ftabsize && - (daemon->free_frec_src = whine_malloc(sizeof(struct frec_src)))) - { - daemon->frec_src_count++; - daemon->free_frec_src->next = NULL; - } - - /* If we've been spammed with many duplicates, just drop the query. */ - if (daemon->free_frec_src) - { - struct frec_src *new = daemon->free_frec_src; - daemon->free_frec_src = new->next; - new->next = forward->frec_src.next; - forward->frec_src.next = new; - new->orig_id = ntohs(header->id); - new->source = *udpaddr; - new->dest = *dst_addr; - new->log_id = daemon->log_id; - new->iface = dst_iface; - new->fd = udpfd; - } - - return 1; - } - + /* new query */ + if (gotname) flags = search_servers(now, &addrp, gotname, daemon->namebuff, &type, &domain, &norebind); @@ -380,6 +383,7 @@ static int forward_query(int udpfd, union mysockaddr *udpaddr, #endif type &= ~SERV_DO_DNSSEC; + /* may be no servers available. */ if (daemon->servers && !flags) forward = get_new_frec(now, NULL, 0); /* table full - flags == 0, return REFUSED */ @@ -2249,25 +2253,6 @@ static struct frec *lookup_frec(unsigned short id, int fd, int family, void *has return NULL; } -static struct frec *lookup_frec_by_sender(unsigned short id, - union mysockaddr *addr, - void *hash) -{ - struct frec *f; - struct frec_src *src; - - for (f = daemon->frec_list; f; f = f->next) - if (f->sentto && - !(f->flags & (FREC_DNSKEY_QUERY | FREC_DS_QUERY)) && - memcmp(hash, f->hash, HASH_SIZE) == 0) - for (src = &f->frec_src; src; src = src->next) - if (src->orig_id == id && - sockaddr_isequal(&src->source, addr)) - return f; - - return NULL; -} - static struct frec *lookup_frec_by_query(void *hash, unsigned int flags) { struct frec *f; -- 2.26.2