Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed to handle case where only IPv6 nameservers are specified #56

Merged
merged 3 commits into from
Aug 15, 2014

Conversation

hloeung
Copy link
Contributor

@hloeung hloeung commented Aug 15, 2014

At present, mtr will abort when only IPv6 nameservers are specified in /etc/resolv.conf as reported in https://bugs.launchpad.net/ubuntu/+source/mtr/+bug/752583. This fixes that by including a count of all IPv6 nameservers.

@rewolff
Copy link
Collaborator

rewolff commented Aug 15, 2014

The underscores in the field name for the ns6count seem to indicate that this is a private field. Isn't there a more proper (portable) way to get at the ns6count field?

@hloeung
Copy link
Contributor Author

hloeung commented Aug 15, 2014

rewolff, thanks for getting back to me. It doesn't seem like there's a more proper/portable way. I've fixed it up so this should only apply if GLIBC is defined.

What do you think?

@rewolff
Copy link
Collaborator

rewolff commented Aug 15, 2014

I've done a bit more research.... We are messing with the internals of the _res variable, which is documented as having just one user-accessible field. And we're messing with a bunch of others.

The code is already not-portable, but seems to work. Can you remove the check for __glibc then I'll merge.

If you want a nice weekend project: Rewrite dns.c: Every lookup request: fork, call the system resolver, report the result, exit. This will cause a flurry of extra mtr-processes every time you start mtr. Modern machines can handle that.... (improvement once that works: If available, use threads. )

I'm not very good with git. Let me try if I can do the "remove glibc check" myself.

@rewolff
Copy link
Collaborator

rewolff commented Aug 15, 2014

The NSCOUNT define is missing for when ipv6 is not enabled/available.

@rewolff rewolff merged commit c02701d into traviscross:master Aug 15, 2014
@rewolff
Copy link
Collaborator

rewolff commented Aug 15, 2014

Fancy! I pulled from your repo, changed a few things, then pushed to github, and it immediately notices in my web-browser that the repo now contains your patch/pull request, so it auto-closes this pull........

@hloeung
Copy link
Contributor Author

hloeung commented Aug 15, 2014

haha fancy indeed! Thank you so much for spending the time reviewing,
fixing, and merging my change.

On 15 August 2014 19:50, rewolff notifications@github.com wrote:

Fancy! I pulled from your repo, changed a few things, then pushed to
github, and it immediately notices in my web-browser that the repo now
contains your patch/pull request, so it auto-closes this pull........


Reply to this email directly or view it on GitHub
#56 (comment).

@hloeung
Copy link
Contributor Author

hloeung commented Aug 15, 2014

rewolff, I think we should only use ncount6 for glibc. It seems that libc used by FreeBSD and possibly others doesn't have that member and IPv6 nameservers are included in ncount:

e.g. http://svnweb.freebsd.org/base/head/lib/libc/resolv/res_init.c?revision=269867&view=markup#l225 and http://svnweb.freebsd.org/base/head/lib/libc/resolv/res_init.c?revision=269867&view=markup#l870

@aquerubin
Copy link
Contributor

On Fri, 15 Aug 2014, rewolff wrote:

Merged #56.

This latest change causes mtr to look for resolver structs that don't
exist and as a result it segfaults. Ie. you cannot just add nscount and
nscount6. In the case where all resolvers in resolv.conf are IPv6 only,
then both nscount and nscount6 are exactly the same (for some OS). But it
doesn't mean you have twice as many nameservers to query. I run a number
of servers and workstations that use only IPv6 nameservers and they do not
exhibit this particular issue. So the question now is what is REALLY
causing the issue and for that I think we need more specific info on the
OS/version exhibiting the problem.

Antonio Querubin
e-mail: tony@lavanauts.org
xmpp: antonioquerubin@gmail.com

@aquerubin
Copy link
Contributor

I'm attaching a short program that I've been using to figure out how
nscount and nscount6 are used/initialized in various OS.

Antonio Querubin
e-mail: tony@lavanauts.org
xmpp: antonioquerubin@gmail.com

@rewolff
Copy link
Collaborator

rewolff commented Aug 15, 2014

@hloeung , Does that "only glibc" address Antonio's objections?
@aquerubin, What OS were you testing on? I don't see your "test program". And if I don't have a freeBSD or somesuch machine handy, what good does that program do me?

@aquerubin
Copy link
Contributor

On Fri, 15 Aug 2014, rewolff wrote:

@hloeung , Does that "only glibc" address Antonio's objections?

@aquerubin, What OS were you testing on?

Previous to this latest commit I had verified mtr was working with only
IPv6 nameservers on the following:

Linux Mint 17
Scientific Linux 6.5
CentOS 6.5
Ubuntu 12.04
Debian 6.0
OpenSUSE 12.2
FreeBSD 10.0

I don't see your "test program".

Looks like the github mail server filters attachments. I'll repost the
code inline below.

I don't mean to belabor this point but in case it's not clear, the idea is
to validate the following:

nscount = the number of listed resolvers in /etc/resolv.conf

nscount6 (if it exists) = the number of IPv6 resolvers in /etc/resolv.conf
or 0 depending on OS. Sometimes it's not always initialized correctly (or
this could be just a bug in the OS).

So nscount6 (if it exists) <= nscount

Attempting to querying nscount+nscount6 resolvers does not make sense when
there are only nscount resolvers in the first place.

The short program that follows below dumps out nscount, nscount6 (if it
exists on that OS), and the resolver IP addresses. Ie. if I can retrieve
ALL the addresses as known to the system resolver then I should be able to
query all of them and receive responses from each one. So we need to get
to the root of @hloeung's reported issue. If he can provide specific OS
info, I can try to duplicate the problem.

// Short program to debug resolver issues. -aq

#include <netinet/in.h>
#include <arpa/nameser.h>
#include <resolv.h>

#include <arpa/inet.h>

#include <string.h>
#include <stdio.h>
#include <stdlib.h>

#ifdef GLIBC
#define NSSOCKADDR6(i) (myres._u._ext.nsaddrs[i])
#else
#define NSSOCKADDR6(i) (&(myres._u._ext.ext->nsaddrs[i].sin6))

#ifndef __res_state_ext
/* __res_state_ext is missing on many (most?) BSD systems */
struct __res_state_ext {
union res_sockaddr_union nsaddrs[MAXNS];
struct sort_list {
int af;
union {
struct in_addr ina;
struct in6_addr in6a;
} addr, mask;
} sort_list[MAXRESOLVSORT];
char nsuffix[64];
char nsuffix2[64];
};
#endif
#endif

#ifdef res_ninit
#define MY_RES_INIT() res_ninit(&myres);
struct __res_state myres;
#else
#define MY_RES_INIT() res_init();
#define myres _res
#endif

int satop(struct sockaddr *sa, char *dst, socklen_t size);

int main(int argc, char **argv) {
struct sockaddr_in6 *sa6;
char addrstr[INET6_ADDRSTRLEN];
unsigned int i;
sa_family_t af;

#ifdef GLIBC
printf("GLIBC = %d\n", GLIBC);
#endif

MY_RES_INIT();

printf("res_state.nscount = %d\n", myres.nscount);
#ifdef __GLIBC

printf("_u._ext_nscount6 = %d\n", myres._u._ext.nscount6);
#endif

if (!myres.nscount) {
fprintf(stderr,"No nameservers defined.\n");
exit(-1);
}

for (i = 0; i < myres.nscount; i++) {
printf("resolver # %d, ", i);
af = myres.nsaddr_list[i].sin_family;
if (af == AF_INET) {
printf("IPv4 : ");
satop((struct sockaddr *) &myres.nsaddr_list[i], addrstr,
sizeof addrstr);
printf("%s\n", addrstr);
}
else {
sa6 = (struct sockaddr_in6 *) NSSOCKADDR6(i);
af = sa6->sin6_family;
if (af == AF_INET6) {
printf("IPv6 : ");
satop((struct sockaddr *) sa6, addrstr, sizeof addrstr);
printf("%s\n", addrstr);
}
}
}

return(0);
}

/* Convert a sockaddr to a printable address. */
int satop(struct sockaddr *sa, char *dst, socklen_t size) {
sa_family_t af;
struct sockaddr_in *sa4;
struct sockaddr_in6 *sa6;

af = sa->sa_family;

switch (af) {
case AF_INET:
sa4 = (struct sockaddr_in *) sa;
inet_ntop(af, (const void *) &(sa4->sin_addr), dst, size);
return(0);
case AF_INET6:
sa6 = (struct sockaddr_in6 *) sa;
inet_ntop(af, (const void *) &(sa6->sin6_addr), dst, size);
return(0);
default:
*dst = '\0';
return(-1);
}

}

And if I don't have a freeBSD or somesuch machine handy, what good does
that program do me?

Virtualization is your friend. It's amazingly easy to boot an ISO image
file into a virtual machine to create a quick sandbox for just about any
OS. The challenge is in determining which OS merit your time and
attention to play with :)

Antonio Querubin
e-mail: tony@lavanauts.org
xmpp: antonioquerubin@gmail.com

@hloeung
Copy link
Contributor Author

hloeung commented Aug 15, 2014

@rewolff yes that "only glibc" addresses Antonio's issue.

@aquerubin nscount only counts IPv6 addresses. Example program I used for testing:

| #include <stdio.h>
| #include <resolv.h>
|
| int main(void) {
| res_init();
| printf("%d\n", _res.nscount);
| return 0;
| }

My resolv.conf:

| nameserver ::1
| nameserver 2001:44b8:1::1
| nameserver 2001:44b8:2::2
| domain local
| options timeout:3 no-check-names

Output from program is shows nscount as 0 as glibc (2.19) only counts IPv4 addresses. This currently affects both Ubuntu[1] and Debian[2] users. ulibc code also notes nscount as a count for only IPv4 nameservers[3].

[1] https://bugs.launchpad.net/ubuntu/+source/mtr/+bug/752583
[2] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=528992
[3] http://git.uclibc.org/uClibc/tree/include/resolv.h?#n146

@hloeung
Copy link
Contributor Author

hloeung commented Aug 15, 2014

#57 to revert previous changes.

@aquerubin
Copy link
Contributor

On Fri, 15 Aug 2014, Haw Loeung wrote:

Output from program is shows nscount as 0 as glibc (2.19) only counts
IPv4 addresses. This currently affects both Ubuntu[1] and Debian[2]
users. ulibc code also notes nscount as a count for only IPv4
nameservers[3].

Really? I just checked the latest Debian release 7.6 and nscount includes
ALL resolver addresses in /etc/resolv.conf up to MAXNS, not just IPv4
addresses.

The latest Linux Mint 17 (Ubuntu derivative) also shows nscount includes
ALL addresses.

Antonio Querubin
e-mail: tony@lavanauts.org
xmpp: antonioquerubin@gmail.com

@hloeung
Copy link
Contributor Author

hloeung commented Aug 17, 2014

Yeah, I mentioned this in #57 to revert my set of changes.

https://lists.debian.org/debian-glibc/2014/06/msg00138.html

I've also reported it upstream to Debian in https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=528992

@aquerubin
Copy link
Contributor

On Sat, 16 Aug 2014, Haw Loeung wrote:

Yeah, I mentioned this in #57 to
revert my set of changes.

https://lists.debian.org/debian-glibc/2014/06/msg00138.html

I've also reported it upstream to Debian in
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=528992

So this affects only the Debian testing release "jessie". Which might
break other things than just mtr.

Antonio Querubin
e-mail: tony@lavanauts.org
xmpp: antonioquerubin@gmail.com

@rewolff
Copy link
Collaborator

rewolff commented Aug 17, 2014

Guys. Are you willing to test a new approach to the DNS lookups?

Check out the "newdns" branch.

The idea is to use the interface that the OS provides to lookup names from IP addresses. I'm not sure it works yet for IPV6 so let me know if you can test and if it works or not.

Here the new version already resolves one IP address that "stock" mtr does not resolve (my local ADSL modem has an entry in /etc/hosts, but nothing in DNS).

For a short while after startup there will be a whole bunch of mtr-processes each doing a name lookup. Might look a bit weird, but that's how it is supposed to work.

Oh... The good news: "dns.c" was reduced by about 80%.

@aquerubin
Copy link
Contributor

On Sun, 17 Aug 2014, rewolff wrote:

Guys. Are you willing to test a new approach to the DNS lookups?

Check out the "newdns" branch.

The idea is to use the interface that the OS provides to lookup names
from IP addresses. I'm not sure it works yet for IPV6 so let me know if
you can test and if it works or not.

Here the new version already resolves one IP address that "stock" mtr
does not resolve (my local ADSL modem has an entry in /etc/hosts, but
nothing in DNS).

For a short while after startup there will be a whole bunch of
mtr-processes each doing a name lookup. Might look a bit weird, but
that's how it is supposed to work.

There's some kind of corruption going on in the output display when
tracing to IPv6 targets and rdns is enabled. However, name resolution for
IPv4 targets does work when using all IPv6 resolvers for rdns.

Antonio Querubin
e-mail: tony@lavanauts.org
xmpp: antonioquerubin@gmail.com

@aquerubin
Copy link
Contributor

On Sun, 17 Aug 2014, rewolff wrote:

Check out the "newdns" branch.

Some corrections below to fix the output corruption.

diff --git a/dns.c b/dns.c
index bc29188..4eabda0 100644
--- a/dns.c
+++ b/dns.c
@@ -193,7 +193,7 @@ void dns_open(void)

  while (fgets (buf, 1024, infp)) {
    ip_t host;
  •  struct sockaddr sa;
    
  •  struct sockaddr_storage sa;
    char hostname [0x100];
    char result [0x100];
    // Find IPV6 version
    

    @@ -206,9 +206,9 @@ void dns_open(void)

      printf ("resolving %s (%d)\n", strlongip (&host), af);
    
  •    set_sockaddr_ip (&sa, &host);
    
  •    set_sockaddr_ip ((struct sockaddr *) &sa, &host);
    
  •    rv = getnameinfo  (&sa, sizeof  (sa),
    
  •    rv = getnameinfo  ((struct sockaddr *) &sa, sizeof  (sa),
                            hostname, 0x100, NULL, 0, 0);
    
      sprintf (result, "%s %s\n", strlongip (&host), hostname);
    

Antonio Querubin
e-mail: tony@lavanauts.org
xmpp: antonioquerubin@gmail.com

@aquerubin
Copy link
Contributor

On Sun, 17 Aug 2014, rewolff wrote:

Check out the "newdns" branch.

Another cumulative diff - hostnames can be very long.
diff --git a/dns.c b/dns.c
index bc29188..9c00c3d 100644
--- a/dns.c
+++ b/dns.c
@@ -165,7 +165,7 @@ void dns_open(void)
exit (-1);
}
if (pid == 0) {

  • char buf[1024];
  • char buf[2048];
    int i;
    FILE *infp; //, *outfp;

@@ -191,11 +191,11 @@ void dns_open(void)
infp = fdopen (todns[0],"r");
//outfp = fdopen (fromdns[1],"w");

  • while (fgets (buf, 1024, infp)) {

  • while (fgets (buf, sizeof buf, infp)) {
    ip_t host;

  •  struct sockaddr sa;
    
  •  char hostname [0x100];
    
  •  char result [0x100];
    
  •  struct sockaddr_storage sa;
    
  •  char hostname [NI_MAXHOST];
    
  •  char result [INET6_ADDRSTRLEN + NI_MAXHOST + 2];
    // Find IPV6 version
    if (!fork ()) {
      int rv;
    

    @@ -206,10 +206,10 @@ void dns_open(void)

      printf ("resolving %s (%d)\n", strlongip (&host), af);
    
  •    set_sockaddr_ip (&sa, &host);
    
  •    set_sockaddr_ip ((struct sockaddr *) &sa, &host);
    
  •    rv = getnameinfo  (&sa, sizeof  (sa), 
    
  •              hostname, 0x100, NULL, 0, 0);
    
  •    rv = getnameinfo  ((struct sockaddr *) &sa, sizeof  (sa), 
    
  •              hostname, sizeof hostname, NULL, 0, 0);
    
      sprintf (result, "%s %s\n", strlongip (&host), hostname);
    

@@ -243,12 +243,12 @@ int dns_waitfd (void)

void dns_ack(void)
{

  • char buf[0x100], host[0x100], name[0x100];

  • char buf[2048], host[NI_MAXHOST], name[NI_MAXHOST];
    ip_t hostip;
    struct dns_results *r;

  • //read (fromdns[0], buf, 0x100);
    
  • while ( fgets (buf, 0x100, fromdnsfp )) {

  • //read (fromdns[0], buf, NI_MAXHOST);
    
  • while ( fgets (buf, sizeof buf, fromdnsfp )) {
    sscanf (buf, "%s %s", host, name);

    longipstr (host, &hostip, af);
    @@ -280,7 +280,7 @@ void dns_ack6(void)
    char *dns_lookup2(ip_t * ip)
    {
    struct dns_results *r;

  • char buf[0x100];

  • char buf[INET6_ADDRSTRLEN + 1];
    int rv;

r = findip (ip);

Antonio Querubin
e-mail: tony@lavanauts.org
xmpp: antonioquerubin@gmail.com

@aquerubin
Copy link
Contributor

On Sun, 17 Aug 2014, rewolff wrote:

Check out the "newdns" branch.

Another cumulative diff.

diff --git a/dns.c b/dns.c
index bc29188..a648a73 100644
--- a/dns.c
+++ b/dns.c
@@ -165,7 +165,7 @@ void dns_open(void)
exit (-1);
}
if (pid == 0) {

  • char buf[1024];
  • char buf[2048];
    int i;
    FILE *infp; //, *outfp;

@@ -191,11 +191,11 @@ void dns_open(void)
infp = fdopen (todns[0],"r");
//outfp = fdopen (fromdns[1],"w");

  • while (fgets (buf, 1024, infp)) {

  • while (fgets (buf, sizeof buf, infp)) {
    ip_t host;

  •  struct sockaddr sa;
    
  •  char hostname [0x100];
    
  •  char result [0x100];
    
  •  struct sockaddr_storage sa;
    
  •  char hostname [NI_MAXHOST];
    
  •  char result [INET6_ADDRSTRLEN + NI_MAXHOST + 2];
    // Find IPV6 version
    if (!fork ()) {
      int rv;
    

    @@ -206,14 +206,18 @@ void dns_open(void)

      printf ("resolving %s (%d)\n", strlongip (&host), af);
    
  •    set_sockaddr_ip (&sa, &host);
    
  •    set_sockaddr_ip ((struct sockaddr *) &sa, &host);
    
  •    rv = getnameinfo  (&sa, sizeof  (sa), 
    

- hostname, 0x100, NULL, 0, 0);

- sprintf (result, "%s %s\n", strlongip (&host), hostname);

  •    printf ("resolved: %s -> %s (%d)\n", strlongip (&host), hostname, rv);
    
  •    rv = getnameinfo  ((struct sockaddr *) &sa, sizeof  (sa), 
    
  •          hostname, sizeof hostname, NULL, 0, NI_NAMEREQD);
    
  •    if (rv==0) {
    
  •        sprintf (result, "%s %s\n", strlongip (&host), hostname);
    
  •        printf ("resolved: %s -> %s (%d)\n", strlongip (&host), hostname, rv);
    
  •    }
    
  •    else {
    
  •        sprintf (result, "%s %s\n", strlongip (&host), strlongip (&host));
    
  •        printf ("not resolved: %s (%d)\n", strlongip (&host), rv);
    
  •    }
    
      rv = write (fromdns[1], result, strlen (result));
      if (rv < 0) perror ("write DNS lookup result");
    

    @@ -243,12 +247,12 @@ int dns_waitfd (void)

    void dns_ack(void)
    {

  • char buf[0x100], host[0x100], name[0x100];

  • char buf[2048], host[NI_MAXHOST], name[NI_MAXHOST];
    ip_t hostip;
    struct dns_results *r;

  • //read (fromdns[0], buf, 0x100);
    
  • while ( fgets (buf, 0x100, fromdnsfp )) {

  • //read (fromdns[0], buf, NI_MAXHOST);
    
  • while ( fgets (buf, sizeof buf, fromdnsfp )) {
    sscanf (buf, "%s %s", host, name);

    longipstr (host, &hostip, af);
    @@ -280,7 +284,7 @@ void dns_ack6(void)
    char *dns_lookup2(ip_t * ip)
    {
    struct dns_results *r;

  • char buf[0x100];

  • char buf[INET6_ADDRSTRLEN + 1];
    int rv;

r = findip (ip);


Antonio Querubin
e-mail: tony@lavanauts.org
xmpp: antonioquerubin@gmail.com

@hloeung
Copy link
Contributor Author

hloeung commented Aug 18, 2014

I'm curious in why fork processes for DNS resolution? If it's to speed up lookups and get around blocking caused by the system resolver why not look into switching to some asynchronous DNS lookup library?

@rewolff
Copy link
Collaborator

rewolff commented Aug 18, 2014

Antonio: I forgot to say: this is a proof-of-concept, There is a whole lot of "fixed size buffers" that need enlarging or making-dynamic.... That's on my todo list, provided that the strategy works.

MTR has had a whole stream of bugs because we were using our own DNS lookup. If say IPV7 comes out, we can be sure that the system lookup functions will still work. But not that the DNS library that we selected in 2014 is still being maintained...

@rewolff
Copy link
Collaborator

rewolff commented Aug 18, 2014

although I have my objections, I searched for asynchronous DNS and examined the top two hits:

  • ADNS
    "coming soon: IPV6 support". -> No go.
  • TADNS
    • Not sure if it supports IPV6, too low-level interface.
  • I don't like a dependency on still another library.

@rewolff
Copy link
Collaborator

rewolff commented Aug 18, 2014

And THIS: #59 my friends is why MTR should not use internal structures of the resolver.....

@hloeung
Copy link
Contributor Author

hloeung commented Aug 19, 2014

@rewolff How about libev or libevent together with getnameinfo() to implement non-blocking DNS?

Or even eventdns which you can bundle with mtr? https://www.imperialviolet.org/binary/eventdns.c

@aquerubin
Copy link
Contributor

newdns branch fails to build on redhat derivatives. Apparently they need
signal.h defined explicitly in dns.c:

diff --git a/dns.c b/dns.c
index b899410..1e92190 100644
--- a/dns.c
+++ b/dns.c
@@ -50,6 +50,7 @@
#include <stdlib.h>
//#include <errno.h>
//#include <time.h>
+#include <signal.h>

#include "mtr.h"
#include "dns.h"

Antonio Querubin
e-mail: tony@lavanauts.org
xmpp: antonioquerubin@gmail.com

@aquerubin
Copy link
Contributor

Apparently FreeBSD is a bit more picky about sockaddr lengths passed to
getnameinfo().

diff --git a/dns.c b/dns.c
index b899410..643c277 100644
--- a/dns.c
+++ b/dns.c
@@ -195,6 +195,7 @@ void dns_open(void)
while (fgets (buf, sizeof (buf), infp)) {
ip_t host;
struct sockaddr_storage sa;

  •  socklen_t salen;
    char hostname [NI_MAXHOST];
    char result [INET6_ADDRSTRLEN + NI_MAXHOST + 2];
    // Find IPV6 version
    

    @@ -208,8 +209,10 @@ void dns_open(void)
    printf ("resolving %s (%d)\n", strlongip (&host), af);

      set_sockaddr_ip (&sa, &host);
    
  • salen = (af == AF_INET ? sizeof (struct sockaddr_in)

  •              : sizeof (struct sockaddr_in6));
    
  •    rv = getnameinfo  ((struct sockaddr *) &sa, sizeof  (sa), 
    
  •    rv = getnameinfo  ((struct sockaddr *) &sa, salen,
               hostname, sizeof (hostname), NULL, 0, 0);
    
      sprintf (result, "%s %s\n", strlongip (&host), hostname);
    

Antonio Querubin
e-mail: tony@lavanauts.org
xmpp: antonioquerubin@gmail.com

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants