Hi Dominik, 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.
Best regards, Erik Den mån 7 okt. 2024 21:25Dominik Derigs via Dnsmasq-discuss < dnsmasq-discuss@lists.thekelleys.org.uk> skrev: > Hi Erik, > > 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. > > 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. > > Best, > > Dominik > On 07.10.24 02:13, Erik Karlsson wrote: > > Hi everyone, > > 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. > > 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. > > 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. > > 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. > > 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: > > > https://discourse.pi-hole.net/t/host-name-of-client-xxx-contains-at-least-one-invalid-character-at-position-0/69132 > > The patch was subsequently applied to Pi-hole with seemingly favourable > outcome: > > https://github.com/pi-hole/FTL/pull/1965 > > Best regards, > Erik > > Den tis 1 okt. 2024 11:01Erik Karlsson <erik.r.karls...@gmail.com> skrev: > >> From: Erik Karlsson <erik.karls...@iopsys.eu> >> >> Not doing so can result in a use after free since the name for DHCP >> derived DNS records is represented as a pointer into the DHCP lease >> table. Update will only happen when necessary since lease_update_dns >> tests internally on dns_dirty and the force argument is zero. >> >> Signed-off-by: Erik Karlsson <erik.karls...@iopsys.eu> >> --- >> src/dnsmasq.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/src/dnsmasq.c b/src/dnsmasq.c >> index a9f26ae..1be3b82 100644 >> --- a/src/dnsmasq.c >> +++ b/src/dnsmasq.c >> @@ -1518,6 +1518,7 @@ static void async_event(int pipe, time_t now) >> { >> lease_prune(NULL, now); >> lease_update_file(now); >> + lease_update_dns(0); >> } >> #ifdef HAVE_DHCP6 >> else if (daemon->doing_ra) >> -- >> 2.46.2 >> >> > _______________________________________________ > Dnsmasq-discuss mailing > listdnsmasq-disc...@lists.thekelleys.org.ukhttps://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 >
_______________________________________________ Dnsmasq-discuss mailing list Dnsmasq-discuss@lists.thekelleys.org.uk https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss
