Hey Frank, dear list-members, thanks for your proposed fix. The Pi-hole team adopted dnsmasq v2.81 early on and we're seeing reports for mysterious crashes scattered all over the dnsmasq code since the very first days of releasing our latest version. Crashes reports show several locations, e.g., in_zone(), iface_check() and even main() in a few places. TCP forks were always happening sometime (not necessarily immediately) before a crash. Running dnsmasq in debug mode (prevents forking) stopped any crashes.
So far, we haven't had the time to dig into this ourselves properly, however, your description perfectly matches with my preliminary conclusion for one week ago: > the dangling pointer is in **all** bug reports known so far in the daemon pointer (https://github.com/pi-hole/FTL/issues/805#issuecomment-643157367) The addition of TCP forks being able to add cache replies in dnsmasq v2.81 is what is causing this code (which has been like this for along time) to become problematic. I recently looked at the same code but missed the chance that the byte may be still in the pipe. This is a very good catch! I'm writing this mail to raise awareness that this appears to be a high severity bug as it can easily lead to anything between indeterminable behavior to fatal failures (typically the case). Even when the bug can only be triggered under certain conditions, it recently caused over 200 posts on the Pi-hole issue ticket system (Github). Crashes due to this have been reported independently by several (more than 20) individual users. Best Dominik On Wed, 2020-06-17 at 19:52 -0700, Frank wrote: > Hello, > > This patch fixes a buffer overflow in TCP requests. Since the read is > not actually being retried, the byte written by the child can be left > in the pipe. When that happens, cache_recv_insert() reads the length > of the name, which is now multiplied by 256 due to the extra 0 byte > (8 bit shift) and results in daemon->namebuff being overflowed. > Namebuff is immediately before the daemon struct in memory so it ends > up corrupting the beginning of the daemon struct. > > diff --git a/src/dnsmasq.c b/src/dnsmasq.c > index 6481de8..457dea3 100644 > --- a/src/dnsmasq.c > +++ b/src/dnsmasq.c > @@ -1887,7 +1887,7 @@ static void check_dns_listeners(time_t now) > single byte comes back up the pipe, which > is sent by the child after it has closed > the > netlink socket. */ > - retry_send(read(pipefd, &a, 1)); > + while (retry_send(read(pipefd, &a, 1))); > #endif > break; > } > @@ -1928,7 +1928,7 @@ static void check_dns_listeners(time_t now) > #ifdef HAVE_LINUX_NETWORK > /* See comment above re netlink socket. */ > close(daemon->netlinkfd); > - retry_send(write(pipefd, &a, 1)); > + while (retry_send(write(pipefd, &a, 1))); > #endif > } > > Frank > > > _______________________________________________ > Dnsmasq-discuss mailing list > Dnsmasqfirstname.lastname@example.org > http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss _______________________________________________ Dnsmasq-discuss mailing list Dnsmasqemail@example.com http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss