When looking what this change did, I have noticed mark_servers() cleanup
of local_domains is using serv->next after it has freed serv. Use
additional variable just like in cleanup_servers().
Patch attached.
On 11/21/22 23:22, Simon Kelley wrote:
Thanks for this. It was in my mind that vary large number of domains
would be --local=/domain/ or --address=/domain/, not forwarding to
servers.
I've applied something that looks very like your patch, but with
cosmetic code changes.
Cheers,
Simon.
On 20/11/2022 05:50, Ye Zhou wrote:
Hi all,
I'm attaching a patch to optimize a speed issue introduced in version
2.86.
I have two ISP upstreams and need to forward different sites to
different ISP's DNS providers. For example:
server=/meituan.com/114.114.114.114 <http://meituan.com/114.114.114.114>
... (lots of records)
server=/taobao.com/223.5.5.5 <http://taobao.com/223.5.5.5>
... (lots of records)
It works well before v2.86. Since v2.86 the configuration load time
becomes extremely long (more than 1 minutes to load all server
records). The time consuming part is inside the rewritten
domain-match.c. When adding a new server record, the code will
traverse all existing records so the configuration load becomes
quadratic time complexity. The issue still persists on v2.88rc3.
This patch will optimize the config load time by bypassing the time
consuming code block. During the config load mark_servers() will
never be called so does not need to waste time on the record
traversal and re-order part.
Before:
dnsmasq --test 77.50s user 0.30s system 99% cpu 1:17.84 total
After:
dnsmasq --test 0.16s user 0.02s system 99% cpu 0.188 total
https://gist.github.com/zhouye/adfd509f51645d314f53992331449c45
<https://gist.github.com/zhouye/adfd509f51645d314f53992331449c45>
_______________________________________________
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss
_______________________________________________
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss
--
Petr Menšík
Software Engineer, RHEL
Red Hat, https://www.redhat.com/
PGP: DFCF908DB7C87E8E529925BC4931CA5B6C9FC5CB
From 11fe9dd86740865ebd6911a3a2ccdaa6b943d043 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Petr=20Men=C5=A1=C3=ADk?= <pemen...@redhat.com>
Date: Fri, 25 Nov 2022 14:06:17 +0100
Subject: [PATCH] Avoid use-after-free when cleaning up local_domains
mark_servers() must use additional variable to hold next pointer. Just
like very similar loop in cleanup_servers() does.
---
src/domain-match.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/domain-match.c b/src/domain-match.c
index bef460a..52a0342 100644
--- a/src/domain-match.c
+++ b/src/domain-match.c
@@ -559,7 +559,7 @@ static int maybe_free_servers = 0;
/* Must be called before add_update_server() to set daemon->servers_tail */
void mark_servers(int flag)
{
- struct server *serv, **up;
+ struct server *serv, *next = NULL, **up;
maybe_free_servers = !!flag;
@@ -580,8 +580,9 @@ void mark_servers(int flag)
1) numerous and 2) not reloaded often. We just delete
and recreate. */
if (flag)
- for (serv = daemon->local_domains, up = &daemon->local_domains; serv; serv = serv->next)
+ for (serv = daemon->local_domains, up = &daemon->local_domains; serv; serv = next)
{
+ next = serv->next;
if (serv->flags & flag)
{
*up = serv->next;
--
2.38.1
_______________________________________________
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss
