[Dnsmasq-discuss] Memory leak for SRV records with TTL=0 in v2.88

[Damian Sawicki]

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

-----

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);