<div dir="auto">Hi Dominik,<div dir="auto"><br></div><div dir="auto">Nice to hear that it works and solves the issues. To me it seems this is a simple oversight which has gone undetected for years because like I described previously the triggering conditions are rather convoluted. It is really a corner case issue.</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 mån 7 okt. 2024 21:25Dominik Derigs via Dnsmasq-discuss <<a href="mailto:dnsmasq-discuss@lists.thekelleys.org.uk">dnsmasq-discuss@lists.thekelleys.org.uk</a>> skrev:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><u></u>
<div>
<p>Hi Erik,</p>
<p>the decision of the Pi-hole team to apply your patch without the
usual waiting for patches to go into dnsmasq main trunk first is
one way to show that we were (and still are) absolutely convinced
about the patch and consider the likeliness of having to revert it
at any point very low. On top, we actually had at least one but I
think even a small couple of bug reports which were resolved with
this patch right away.</p>
<p>There is nothing wrong with your patch (as far as my point of
view weights in here) but Simon Kelley is the sole maintainer of
dnsmasq and he decides which patch gets merged, when and, if.<br>
</p>
<p>Best,</p>
<p>Dominik<br>
</p>
<div>On 07.10.24 02:13, Erik Karlsson wrote:<br>
</div>
<blockquote type="cite">
<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 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 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 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 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 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>
<br>
<fieldset></fieldset>
<pre>_______________________________________________
Dnsmasq-discuss mailing list
<a href="mailto:Dnsmasq-discuss@lists.thekelleys.org.uk" target="_blank" rel="noreferrer">Dnsmasq-discuss@lists.thekelleys.org.uk</a>
<a href="https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss" target="_blank" rel="noreferrer">https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss</a>
</pre>
</blockquote>
</div>
_______________________________________________<br>
Dnsmasq-discuss mailing list<br>
<a href="mailto:Dnsmasq-discuss@lists.thekelleys.org.uk" target="_blank" rel="noreferrer">Dnsmasq-discuss@lists.thekelleys.org.uk</a><br>
<a href="https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss" rel="noreferrer noreferrer" target="_blank">https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss</a><br>
</blockquote></div>