From: DL6ER <[email protected]> When `tcp_request()` answers from stale cache data, it closes the connection right away, so that the child process can refresh the name and exit. A query the client has already sent on that connection is never read, and its answer is lost.
glibc runs into this: after a truncated UDP reply, it sends the A and AAAA queries together on one TCP connection, gets EOF after the first answer and fails the lookup without retrying the second query. In a test with both answers expired, 7 of 20 such lookups lost the second answer. Close the connection only if the client has not sent anything else yet. Otherwise, refresh the name and go on answering the queries on the connection, as for any other forwarded query. Signed-off-by: DL6ER <[email protected]> --- Hi Simon, with --use-stale-cache, tcp_request() closes the connection right after a stale answer, so a query the client has already queued on it is lost. glibc's A+AAAA retry over TCP after a truncated UDP reply runs into this and fails the lookup. The patch keeps the connection open only when more data is already waiting (MSG_PEEK), refreshes the name and then goes on answering. An idle connection is still closed right away, as before. Reproducer, loopback only: an upstream dnsmasq on port 5454 with --host- record=stale.test,192.0.2.1,2001:db8::1 --local-ttl=1, and the dnsmasq under test with --server=127.0.0.1#5454 --use-stale-cache. Query A and AAAA once over UDP, wait 1.5 s, then send both queries in one write on a TCP connection. Both answers arrived in 13 of 20 trials before and in 20 of 20 with the patch. Downstream report: https://github.com/pi-hole/FTL/issues/3167 Best regards, Dominik src/forward.c | 30 +++++++++++++++++++++++------- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/src/forward.c b/src/forward.c index f736c30..dae9a4f 100644 --- a/src/forward.c +++ b/src/forward.c @@ -2406,7 +2406,7 @@ void tcp_request(int confd, time_t now, struct iovec *bigbuff, unsigned char *pheader; unsigned int mark = 0; int have_mark = 0; - int first, last, filtered, do_stale = 0; + int first, last, filtered, do_stale = 0, closed = 0; struct iovec out_iov[2]; bigbuff->iov_base = NULL; @@ -2742,8 +2742,16 @@ void tcp_request(int confd, time_t now, struct iovec *bigbuff, } } + /* Fresh data is in the cache now. Go on with the queries the client + had already sent, if the connection is still open. */ if (do_stale) - break; + { + if (closed) + break; + do_stale = 0; + daemon->log_source_addr = &peer_addr; + continue; + } /* In case of local answer or no connections made. */ if (m == 0) @@ -2792,19 +2800,27 @@ void tcp_request(int confd, time_t now, struct iovec *bigbuff, /* If we answered with stale data, this process will now try and get fresh data into the cache and cannot therefore accept new queries. Close the incoming connection to signal that to the client. Then set do_stale and loop round - once more to try and get fresh data, after which we exit. */ + once more to try and get fresh data, after which we exit. A client which + has already sent its next query is waiting for the answer and would lose + it, so keep the connection open then and answer it after the refresh. */ if (stale) { - shutdown(confd, SHUT_RDWR); - close(confd); + char c; + + if (recv(confd, &c, 1, MSG_PEEK | MSG_DONTWAIT) != 1) + { + shutdown(confd, SHUT_RDWR); + close(confd); + closed = 1; + } do_stale = 1; /* Don't mark the query with the source when refreshing stale data. */ daemon->log_source_addr = NULL; } } -/* If we ran once to get fresh data, confd is already closed. */ - if (!do_stale) +/* If we ran once to get fresh data, confd may be closed already. */ + if (!closed) { shutdown(confd, SHUT_RDWR); close(confd); -- 2.43.0 _______________________________________________ Dnsmasq-discuss mailing list [email protected] https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss
