On 05/10/2023 16:56, Damian Sawicki via Dnsmasq-discuss wrote:
Hello dnsmasq experts,
There seems to be a memory leak in v2.88. The reproduction steps are
as follows: insert an SRV record with TTL=0 in an upstream DNS server
and query dnsmasq for this record. I inserted a record with name
"dnsmasq-reproduction.entirelynew.com." and 20 almost identical
entries of the form "0 1 587 mail.example.com." and queried dnsmasq at
12 qps: after an hour, the memory consumption increased by about 32
MB.
AFAIU, the leaking memory is allocated in rfc1035.c in
if (!(addr.srv.target = blockdata_alloc(name, addr.srv.targetlen)))
(line 825 in v2.88). When the following insertion fails (and it fails
when ttl == 0 and daemon->min_cache_ttl == 0 in cache_insert)
newc = cache_insert(name, &addr, C_IN, now, attl, flags | F_FORWARD |
secflag);
(line 867 in v2.88), the memory is never freed. I haven't reproduced a
leak in this scenario, but lines 829-830 also don't include
deallocation.
if (!extract_name(header, qlen, &tmp, name, 1, 0))
return 2;
The relevant code hasn’t changed much between 2.81 and 2.89, so the
entire range might potentially be affected.
I’ve noticed the relevant part is currently being rewritten. Until a
new version is ready, the patch (not tested!) pasted below should
solve the problem. The default behaviour of Unbound is to serve stale
records with TTL=0 (see
https://unbound.docs.nlnetlabs.nl/en/latest/topics/core/serve-stale.html),
so this leak may affect many users.
I would be grateful if you could possibly let me know when the release
of v2.90 is planned. If something is not clear, or I could be of any
assistance regarding the leak, please don't hesitate to let me know!
Kind regards,
Damian Sawicki,
That all makes sense.
You're right that this code being re-written: it's been generalised to
cache any RR-type. That hasn't fixed the memory leak :( just generalized
it. Now you can leak memory with TTL=0 records of almost any type!
I shall port your fix to the new code forthwith.
The 2.90 release is planned just as soon as I can tidy up all the loose
ends. It's not hanging on any big upgrades and insoluble bugs, just on
my finding time.
I think your patch misses one thing: you need to check that the address
being inserted is a SRV record: it's a big union, and may be something
completely different, so you can't rely on addr.srv.target without checking
if (!newc && (flags & F_SRV) && addr.srv.target)
blockdata_free(addr.srv.target);
Cheers,
Simon.
-----
The patch is identical for v2.88 and v2.89.
diff --git a/src/rfc1035.c b/src/rfc1035.c
index 5c0df56..e85fe2e 100644
--- a/src/rfc1035.c
+++ b/src/rfc1035.c
@@ -826,8 +826,10 @@ int extract_addresses(struct dns_header *header,
size_t qlen, char *name, time_t
return 0;
/* we overwrote the original name, so get it back here. */
- if (!extract_name(header, qlen, &tmp, name, 1, 0))
+ if (!extract_name(header, qlen, &tmp, name, 1, 0)){
+ blockdata_free(addr.srv.target);
return 2;
+ }
}
else if (flags & (F_IPV4 | F_IPV6))
{
@@ -865,6 +867,8 @@ int extract_addresses(struct dns_header *header,
size_t qlen, char *name, time_t
if (insert)
{
newc = cache_insert(name, &addr, C_IN, now, attl,
flags | F_FORWARD | secflag);
+ if (!newc && addr.srv.target)
+ blockdata_free(addr.srv.target);
if (newc && cpp)
{
next_uid(newc);
_______________________________________________
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
