Re: [Dnsmasq-discuss] Patch to cache SRV records - updated version (#3)
Magic, thank you. The fix, to a high level of confidence, is http://thekelleys.org.uk/gitweb/?p=dnsmasq.git;a=commit;h=a90f09db4cc635941a32b973b57e58c662569625 and the problem involved querying for a SRV record for _bookmarkdavs._tcp.p48-bookmarks.icloud.com which returns NXDOMAIN, as it should, but drops a little logic bomb into the cache datastructures which will detonate some unpredictable time later, when the cache entry is freed. Any non-existing SRV record will have the same effect. I've also seen a tight-loop problem in my testing, which I don't think is this problem. There's another fix, http://thekelleys.org.uk/gitweb/?p=dnsmasq.git;a=commit;h=2896e2485e44c04e73a0b7c9f7cbc9c8515d0800 which I think fixes that, but I'm not 100% sure I can think myself from the error to the tight loop, so I'm doing some more testing. Thanks for your contribution with this, and please keep running the development code. It really helps for people to do that. Cheers, Simon. On 08/01/2019 23:51, Mufasa wrote: > The requested core dump files have been sent. I updated my CFLAGS to use -g > instead of -O2 and executed the ulimit command immediately before dnsmasq is > launched (same script) in the fresh container. > > Because of the default behavior of Ubuntu’s Docker image, core dump files > were not being generated nor could I set the core dump output location to a > directory instead of the non-functional apport configuration default, so I > made these additional changes to my environment to generate the files: > > "docker —run”, the command to launch the container, got new parameters > "--privileged --ulimit core=-1” per recommendations I found on generating > core dumps in a container. This was in addition to running the ulimit > command in the shell script I start dnsmasq from. > > The launch script now does this to set the core dump location: > echo '/tmp/core.%h.%e.%t' > /proc/sys/kernel/core_pattern > > -Daniel > >> On Jan 8, 2019, at 3:14 AM, Simon Kelley wrote: >> >> Thanks for the feedback, and for tracking the bleeding-edge code. The >> following should help get useful information. >> >> 1) Replace -O2 with -g in your compilation flags >> 2) run the command "ulimit -c unlimited" from the shell you start >> dnsmasq from. >> >> When it goes bang, send me the core dump and the dnsmasq binary. >> > > ___ > Dnsmasq-discuss mailing list > Dnsmasq-discuss@lists.thekelleys.org.uk > http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss > ___ Dnsmasq-discuss mailing list Dnsmasq-discuss@lists.thekelleys.org.uk http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
Re: [Dnsmasq-discuss] Patch to cache SRV records - updated version (#3)
The requested core dump files have been sent. I updated my CFLAGS to use -g instead of -O2 and executed the ulimit command immediately before dnsmasq is launched (same script) in the fresh container. Because of the default behavior of Ubuntu’s Docker image, core dump files were not being generated nor could I set the core dump output location to a directory instead of the non-functional apport configuration default, so I made these additional changes to my environment to generate the files: "docker —run”, the command to launch the container, got new parameters "--privileged --ulimit core=-1” per recommendations I found on generating core dumps in a container. This was in addition to running the ulimit command in the shell script I start dnsmasq from. The launch script now does this to set the core dump location: echo '/tmp/core.%h.%e.%t' > /proc/sys/kernel/core_pattern -Daniel > On Jan 8, 2019, at 3:14 AM, Simon Kelley wrote: > > Thanks for the feedback, and for tracking the bleeding-edge code. The > following should help get useful information. > > 1) Replace -O2 with -g in your compilation flags > 2) run the command "ulimit -c unlimited" from the shell you start > dnsmasq from. > > When it goes bang, send me the core dump and the dnsmasq binary. > ___ Dnsmasq-discuss mailing list Dnsmasq-discuss@lists.thekelleys.org.uk http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
Re: [Dnsmasq-discuss] Patch to cache SRV records - updated version (#3)
On 08/01/2019 03:46, Mufasa wrote: > On 01/07/2019 08:32 AM, Simon Kelley wrote: >>/I've worked through the patch, and been inspired to clean up a few >>/>/long-standing nasty bits. This has the consequence that the mechanisms >>/>/which were added to enable storage of DNSKEY and DS RRtypes during the >>/>/the DNSSEC campaign are now much more general, and I've used them to >>/>/implement SRV caching. The new code is therefore all mine, as are any >>/>/bugs, but the net effect is the same as Jeremy's (I hope). />//>//>/I >>didn't implement a config switch to disable caching of SRV records, >>/>/because I can't conceive of a situation where such would be necessary. >>/>//>//>/Code is in the git repo now, and we're eating the new dog food here. >>/>/Please test away./ > > New to the list but just wanted to report my experience with the “new > dog food”. > > I use dnsmasq in Docker and have a script that will fully build my > configuration from the official Ubuntu docker image and the dnsmasq git > repository HEAD. Its been working well for almost a year now. I only > once had to rollback a commit when it broke compilation and after some > waiting, a new commit fixed the issue. > > After updating to commit 5b99eae59d59a8e34a7e512059b98bbd803312f2 today, > I’m finding that it dies with a "Segmentation fault (core dumped)" in > about 30 minutes or less. > > I compile it with CFLAGS='-Wall -W -O2 -DNO_IPV6 and launch it with > /usr/local/sbin/dnsmasq -d --log-facility=/var/log/dnsmasq.log > > If you have any advice on capturing more information about the segfault, > let me know. > Thanks for the feedback, and for tracking the bleeding-edge code. The following should help get useful information. 1) Replace -O2 with -g in your compilation flags 2) run the command "ulimit -c unlimited" from the shell you start dnsmasq from. When it goes bang, send me the core dump and the dnsmasq binary. Cheers, Simon. > -Daniel > > ___ > Dnsmasq-discuss mailing list > Dnsmasq-discuss@lists.thekelleys.org.uk > http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss > ___ Dnsmasq-discuss mailing list Dnsmasq-discuss@lists.thekelleys.org.uk http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
Re: [Dnsmasq-discuss] Patch to cache SRV records - updated version (#3)
On 01/07/2019 08:32 AM, Simon Kelley wrote: > I've worked through the patch, and been inspired to clean up a few > long-standing nasty bits. This has the consequence that the mechanisms > which were added to enable storage of DNSKEY and DS RRtypes during the > the DNSSEC campaign are now much more general, and I've used them to > implement SRV caching. The new code is therefore all mine, as are any > bugs, but the net effect is the same as Jeremy's (I hope). > > > I didn't implement a config switch to disable caching of SRV records, > because I can't conceive of a situation where such would be necessary. > > > Code is in the git repo now, and we're eating the new dog food here. > Please test away. New to the list but just wanted to report my experience with the “new dog food”. I use dnsmasq in Docker and have a script that will fully build my configuration from the official Ubuntu docker image and the dnsmasq git repository HEAD. Its been working well for almost a year now. I only once had to rollback a commit when it broke compilation and after some waiting, a new commit fixed the issue. After updating to commit 5b99eae59d59a8e34a7e512059b98bbd803312f2 today, I’m finding that it dies with a "Segmentation fault (core dumped)" in about 30 minutes or less. I compile it with CFLAGS='-Wall -W -O2 -DNO_IPV6 and launch it with /usr/local/sbin/dnsmasq -d --log-facility=/var/log/dnsmasq.log If you have any advice on capturing more information about the segfault, let me know. -Daniel___ Dnsmasq-discuss mailing list Dnsmasq-discuss@lists.thekelleys.org.uk http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
Re: [Dnsmasq-discuss] Patch to cache SRV records - updated version (#3)
On 01/07/2019 08:32 AM, Simon Kelley wrote: > I've worked through the patch, and been inspired to clean up a few > long-standing nasty bits. This has the consequence that the mechanisms > which were added to enable storage of DNSKEY and DS RRtypes during the > the DNSSEC campaign are now much more general, and I've used them to > implement SRV caching. The new code is therefore all mine, as are any > bugs, but the net effect is the same as Jeremy's (I hope). > > > I didn't implement a config switch to disable caching of SRV records, > because I can't conceive of a situation where such would be necessary. > > > Code is in the git repo now, and we're eating the new dog food here. > Please test away. Thanks a *lot* Simon. I'll start testing this new version asap. Jeremy. signature.asc Description: OpenPGP digital signature ___ Dnsmasq-discuss mailing list Dnsmasq-discuss@lists.thekelleys.org.uk http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
Re: [Dnsmasq-discuss] Patch to cache SRV records - updated version (#3)
I've worked through the patch, and been inspired to clean up a few long-standing nasty bits. This has the consequence that the mechanisms which were added to enable storage of DNSKEY and DS RRtypes during the the DNSSEC campaign are now much more general, and I've used them to implement SRV caching. The new code is therefore all mine, as are any bugs, but the net effect is the same as Jeremy's (I hope). I didn't implement a config switch to disable caching of SRV records, because I can't conceive of a situation where such would be necessary. Code is in the git repo now, and we're eating the new dog food here. Please test away. Cheers, Simon. On 20/12/2018 23:20, Jeremy Allison wrote: > On 12/20/2018 03:11 PM, Simon Kelley wrote: >> This is worth having for the removal of the archaic 16-bit limit on the >> flags field in a cache record alone. I've been meaning to tackle that >> for some time. >> >> This time of year either frees up lots of time for coding, or yields >> none at all, and for me it's the later, but I will go through this and >> merge it in the new year. > > Oh thanks for the comments. I'm currently running this version > under valgrind (with a minor test hack to force negative > SRV records to explitly expire after 60 seconds to ensure > I'm going through the cache expire code paths) and it seems > robust so far. > > There are some people in the ChromeOS Teams also looking > at this, although as it's holiday time here in the US it > might not get fully examined until Jan. If anyone finds > any more errors with it under test I'll let you know > and update it. > > Cheers & thanks and Merry Christmas / Happy Holidays ! > > Jeremy. > signature.asc Description: OpenPGP digital signature ___ Dnsmasq-discuss mailing list Dnsmasq-discuss@lists.thekelleys.org.uk http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
Re: [Dnsmasq-discuss] Patch to cache SRV records - updated version (#3)
This is worth having for the removal of the archaic 16-bit limit on the flags field in a cache record alone. I've been meaning to tackle that for some time. This time of year either frees up lots of time for coding, or yields none at all, and for me it's the later, but I will go through this and merge it in the new year. Cheers, Simon. On 20/12/2018 21:38, Jeremy Allison wrote: > On 12/20/2018 12:20 PM, Jeremy Allison wrote: >> On Thu, 20 Dec 2018 11:53:11 -0800 >> >> Jeremy Allison wrote: >> >>> I know dnsmasq doesn't cache SRV records by design: >>> >>> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=579536;msg=9 >>> >>> However, when used with Samba code (winbindd) and >>> other Windows-integrating technology (MIT krb5) >>> there are a lot of SRV record queries that make >>> the whole integration stack slow if SRV records >>> are not cached. >>> >>> With that in mind, here is a patch to cache >>> SRV records positively and negatively inside >>> dnsmasq. >>> >>> I'm sending here it so that people who might need >>> this functionality have a central place to find >>> it (it might end up being used in ChromeOS, depending >>> on test results / review). >> >> Sigh. Found a bug when testing. free_mx_srv_record() >> wasn't checking for NULL pointers on free(), >> which can be the case for negative cache >> records. > > Third time is the charm :-). Remember to NULL > out free'd and uninitialized pointers and > structrures, and remove the F_SRV flag on deleting > the cache entry. > > Hopefully this is the last iteration of this. > I can't see any more issues to address, but > I'm still testing :-). > > Jeremy. > > > ___ > Dnsmasq-discuss mailing list > Dnsmasq-discuss@lists.thekelleys.org.uk > http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss > signature.asc Description: OpenPGP digital signature ___ Dnsmasq-discuss mailing list Dnsmasq-discuss@lists.thekelleys.org.uk http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
Re: [Dnsmasq-discuss] Patch to cache SRV records - updated version (#3)
I think this should be added to the code maybe with an option in the config file to turn on the caching of these records. > -Original Message- > From: Dnsmasq-discuss > On Behalf Of Jeremy Allison > Sent: Thursday, December 20, 2018 4:39 PM > To: dnsmasq-discuss@lists.thekelleys.org.uk > Subject: Re: [Dnsmasq-discuss] Patch to cache SRV records - updated version > (#3) > > On 12/20/2018 12:20 PM, Jeremy Allison wrote: > > On Thu, 20 Dec 2018 11:53:11 -0800 > > > > Jeremy Allison wrote: > > > >> I know dnsmasq doesn't cache SRV records by design: > >> > >> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=579536;msg=9 > >> > >> However, when used with Samba code (winbindd) and > >> other Windows-integrating technology (MIT krb5) > >> there are a lot of SRV record queries that make > >> the whole integration stack slow if SRV records > >> are not cached. > >> > >> With that in mind, here is a patch to cache > >> SRV records positively and negatively inside > >> dnsmasq. > >> > >> I'm sending here it so that people who might need > >> this functionality have a central place to find > >> it (it might end up being used in ChromeOS, depending > >> on test results / review). > > > > Sigh. Found a bug when testing. free_mx_srv_record() > > wasn't checking for NULL pointers on free(), > > which can be the case for negative cache > > records. > > Third time is the charm :-). Remember to NULL > out free'd and uninitialized pointers and > structrures, and remove the F_SRV flag on deleting > the cache entry. > > Hopefully this is the last iteration of this. > I can't see any more issues to address, but > I'm still testing :-). > > Jeremy. ___ Dnsmasq-discuss mailing list Dnsmasq-discuss@lists.thekelleys.org.uk http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
Re: [Dnsmasq-discuss] Patch to cache SRV records - updated version (#3)
On 12/20/2018 12:20 PM, Jeremy Allison wrote:
> On Thu, 20 Dec 2018 11:53:11 -0800
>
> Jeremy Allison wrote:
>
>> I know dnsmasq doesn't cache SRV records by design:
>>
>> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=579536;msg=9
>>
>> However, when used with Samba code (winbindd) and
>> other Windows-integrating technology (MIT krb5)
>> there are a lot of SRV record queries that make
>> the whole integration stack slow if SRV records
>> are not cached.
>>
>> With that in mind, here is a patch to cache
>> SRV records positively and negatively inside
>> dnsmasq.
>>
>> I'm sending here it so that people who might need
>> this functionality have a central place to find
>> it (it might end up being used in ChromeOS, depending
>> on test results / review).
>
> Sigh. Found a bug when testing. free_mx_srv_record()
> wasn't checking for NULL pointers on free(),
> which can be the case for negative cache
> records.
Third time is the charm :-). Remember to NULL
out free'd and uninitialized pointers and
structrures, and remove the F_SRV flag on deleting
the cache entry.
Hopefully this is the last iteration of this.
I can't see any more issues to address, but
I'm still testing :-).
Jeremy.
From d9758e1b340ceb7a46c965a53dc040193d80956d Mon Sep 17 00:00:00 2001
From: Jeremy Allison
Date: Tue, 18 Dec 2018 10:44:08 -0800
Subject: [PATCH 1/6] Widen 'flags' field in cache to unsigned int from
unsigned short.
Ensure we still only cache unsigned short bits (0x).
Signed-off-by: Jeremy Allison
---
src/cache.c | 23 ++-
src/dnsmasq.h | 6 --
src/rfc1035.c | 2 +-
3 files changed, 19 insertions(+), 12 deletions(-)
diff --git a/src/cache.c b/src/cache.c
index 8662f54..cd72a0c 100644
--- a/src/cache.c
+++ b/src/cache.c
@@ -27,7 +27,7 @@ static int bignames_left, hash_size;
static void make_non_terminals(struct crec *source);
static struct crec *really_insert(char *name, struct all_addr *addr,
- time_t now, unsigned long ttl, unsigned
short flags);
+ time_t now, unsigned long ttl, unsigned int
flags);
/* type->string mapping: this is also used by the name-hash function as a
mixing table. */
static const struct {
@@ -330,7 +330,7 @@ static int is_expired(time_t now, struct crec *crecp)
return 1;
}
-static struct crec *cache_scan_free(char *name, struct all_addr *addr, time_t
now, unsigned short flags,
+static struct crec *cache_scan_free(char *name, struct all_addr *addr, time_t
now, unsigned int flags,
struct crec **target_crec, unsigned int
*target_uid)
{
/* Scan and remove old entries.
@@ -465,7 +465,7 @@ void cache_start_insert(void)
}
struct crec *cache_insert(char *name, struct all_addr *addr,
- time_t now, unsigned long ttl, unsigned short flags)
+ time_t now, unsigned long ttl, unsigned int flags)
{
/* Don't log DNSSEC records here, done elsewhere */
if (flags & (F_IPV4 | F_IPV6 | F_CNAME))
@@ -483,7 +483,7 @@ struct crec *cache_insert(char *name, struct all_addr *addr,
static struct crec *really_insert(char *name, struct all_addr *addr,
- time_t now, unsigned long ttl, unsigned
short flags)
+ time_t now, unsigned long ttl, unsigned int
flags)
{
struct crec *new, *target_crec = NULL;
union bigname *big_name = NULL;
@@ -495,6 +495,11 @@ static struct crec *really_insert(char *name, struct
all_addr *addr,
if (insert_error)
return NULL;
+ /*
+ * Ensure we only cache unsigned short bits.
+ */
+ flags &= DNSMASQ_CACHE_MASK;
+
/* First remove any expired entries and entries for the name/address we
are currently inserting. */
if ((new = cache_scan_free(name, addr, now, flags, _crec,
_uid)))
@@ -656,7 +661,7 @@ void cache_end_insert(void)
{
char *name = cache_get_name(new_chain);
ssize_t m = strlen(name);
- unsigned short flags = new_chain->flags;
+ unsigned int flags = new_chain->flags;
#ifdef HAVE_DNSSEC
u16 class = new_chain->uid;
#endif
@@ -717,7 +722,7 @@ int cache_recv_insert(time_t now, int fd)
struct all_addr addr;
unsigned long ttl;
time_t ttd;
- unsigned short flags;
+ unsigned int flags;
struct crec *crecp = NULL;
cache_start_insert();
@@ -856,7 +861,7 @@ struct crec *cache_find_by_name(struct crec *crecp, char
*name, time_t now, unsi
/* first search, look for relevant entries and push to top of list
also free anything which has expired */
struct crec *next, **up, **insert = NULL, **chainp =
- unsigned short ins_flags = 0;
+ unsigned int ins_flags = 0;
for (up = hash_bucket(name), crecp = *up; crecp; crecp = next)
{
@@ -1140,7 +1145,7 @@ int read_hostsfile(char *filename, unsigned int index,
int cache_size, struct cr
