<div dir="auto">Hi everyone,<div dir="auto"><br></div><div dir="auto">Does anyone have something to say about this patch? I sent it for the first time about 6 months ago but the discussion quickly died down and it seems to have been forgotten about since then.</div><div dir="auto"><br></div><div dir="auto">To elaborate a bit about the rationale behind it the issue was discovered by analyzing core dumps from live customer installations which I for obvious confidentiality/privacy reasons cannot share. The core dumps showed a use after free of crecp->name.namep in cache_scan_free for records derived from DHCP. The core dumps also showed that the value of dns_dirty was 1 which is indicative of the fact that lease_update_dns needs to be run but has not been run.</div><div dir="auto"><br></div><div dir="auto">For the issue to reproduce with default configuration I think you need at least two DHCP derived names which share the same domain (otherwise the issue is masked by the is_expired logic). One of them has to expire followed by a request for either the domain itself or something with a coinciding hash before any DHCP requests are handled. Additionally for an actual crash to happen the memory has to be unmapped as opposed to just returned to the free list. In most cases rather than crashing garbage data will be returned, something which may or may not go unnoticed. So, it is a bit of a corner case issue but a very real one as evidenced by the core dumps.</div><div dir="auto"><br></div><div dir="auto">From analysis of the code and especially other places where lease_prune is used it also seems to be evident that lease_update_dns is needed afterwards. The only place where this is not done is async_event and this is what the patch is addressing.</div><div dir="auto"><br></div><div dir="auto">In addition when use-stale-cache is enabled the issue seems to reproduce more easily, something which has been reported by the Pi-hole community:</div><div dir="auto"><br></div><div dir="auto"><a href="https://discourse.pi-hole.net/t/host-name-of-client-xxx-contains-at-least-one-invalid-character-at-position-0/69132" rel="noreferrer noreferrer noreferrer noreferrer" target="_blank">https://discourse.pi-hole.net/t/host-name-of-client-xxx-contains-at-least-one-invalid-character-at-position-0/69132</a><br></div><div dir="auto"><br></div><div dir="auto">The patch was subsequently applied to Pi-hole with seemingly favourable outcome:</div><div dir="auto"><br></div><div dir="auto"><a href="https://github.com/pi-hole/FTL/pull/1965" rel="noreferrer noreferrer noreferrer" target="_blank">https://github.com/pi-hole/FTL/pull/1965</a><br></div><div dir="auto"><br></div><div dir="auto">Best regards,</div><div dir="auto">Erik</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">Den tis 1 okt. 2024 11:01Erik Karlsson <<a href="mailto:erik.r.karlsson@gmail.com" rel="noreferrer noreferrer noreferrer noreferrer noreferrer noreferrer noreferrer noreferrer noreferrer noreferrer noreferrer noreferrer noreferrer noreferrer noreferrer noreferrer noreferrer noreferrer" target="_blank">erik.r.karlsson@gmail.com</a>> skrev:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">From: Erik Karlsson <<a href="mailto:erik.karlsson@iopsys.eu" rel="noreferrer noreferrer noreferrer noreferrer noreferrer noreferrer noreferrer noreferrer noreferrer noreferrer noreferrer noreferrer noreferrer noreferrer noreferrer noreferrer noreferrer noreferrer noreferrer" target="_blank">erik.karlsson@iopsys.eu</a>><br>
<br>
Not doing so can result in a use after free since the name for DHCP<br>
derived DNS records is represented as a pointer into the DHCP lease<br>
table. Update will only happen when necessary since lease_update_dns<br>
tests internally on dns_dirty and the force argument is zero.<br>
<br>
Signed-off-by: Erik Karlsson <<a href="mailto:erik.karlsson@iopsys.eu" rel="noreferrer noreferrer noreferrer noreferrer noreferrer noreferrer noreferrer noreferrer noreferrer noreferrer noreferrer noreferrer noreferrer noreferrer noreferrer noreferrer noreferrer noreferrer noreferrer" target="_blank">erik.karlsson@iopsys.eu</a>><br>
---<br>
src/dnsmasq.c | 1 +<br>
1 file changed, 1 insertion(+)<br>
<br>
diff --git a/src/dnsmasq.c b/src/dnsmasq.c<br>
index a9f26ae..1be3b82 100644<br>
--- a/src/dnsmasq.c<br>
+++ b/src/dnsmasq.c<br>
@@ -1518,6 +1518,7 @@ static void async_event(int pipe, time_t now)<br>
{<br>
lease_prune(NULL, now);<br>
lease_update_file(now);<br>
+ lease_update_dns(0);<br>
}<br>
#ifdef HAVE_DHCP6<br>
else if (daemon->doing_ra)<br>
-- <br>
2.46.2<br>
<br>
</blockquote></div>