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

ping: do a reverse lookup for numeric addresses #421

Closed
rnhmjoj opened this issue Aug 28, 2022 · 17 comments
Closed

ping: do a reverse lookup for numeric addresses #421

rnhmjoj opened this issue Aug 28, 2022 · 17 comments

Comments

@rnhmjoj
Copy link

rnhmjoj commented Aug 28, 2022

It seems that if you specify a "literal" IP address ping won't do a reverse name lookup. I think this would be good to have in general and it's particularly nice for IPv6: if you have set up a reverse zone for the link local you can then do

$ ping ff02::1%eth0 -c2
PING ff02::1%eth0(ff02::1%eth0 (ff02::1%eth0)) 56 data bytes
64 bytes from me.home.arpa (fe80::46f5:36ff:fe71:2081%eth0): icmp_seq=1 ttl=64 time=0.080 ms
64 bytes from router.home.arpa (fe80::3e48:edff:fe5a:50ab%eth0): icmp_seq=1 ttl=64 time=0.450 ms
64 bytes from ap-1.home.arpa (fe80::864c:80ff:fe0a:3058%eth0): icmp_seq=1 ttl=64 time=0.798 ms
64 bytes from ap-2.home.arpa (fe80::6231:4eff:feb2:50d3%eth0): icmp_seq=1 ttl=64 time=6.92 ms
64 bytes from me.home.arpa (fe80::46f5:36ff:fe71:2081%eth0): icmp_seq=2 ttl=64 time=0.083 ms

--- ff02::1%eth0 ping statistics ---
2 packets transmitted, 2 received, +3 duplicates, 0% packet loss, time 1001ms
rtt min/avg/max/mdev = 0.080/1.665/6.916/2.638 ms

and get a nice scan of your network.

It's enough to remove this check to get this behavior, not sure if there are other side effects.

diff --git a/ping/ping6_common.c b/ping/ping6_common.c
index 13bf45a..39f7d59 100644
--- a/ping/ping6_common.c
+++ b/ping/ping6_common.c
@@ -132,9 +132,6 @@ int ping6_run(struct ping_rts *rts, int argc, char **argv, struct addrinfo *ai,
 	memcpy(&rts->whereto6, ai->ai_addr, sizeof(rts->whereto6));
 	rts->whereto6.sin6_port = htons(IPPROTO_ICMPV6);
 
-	if (memchr(target, ':', strlen(target)))
-		rts->opt_numeric = 1;
-
 	if (IN6_IS_ADDR_UNSPECIFIED(&rts->firsthop.sin6_addr)) {
 		memcpy(&rts->firsthop.sin6_addr, &rts->whereto6.sin6_addr, 16);
 		rts->firsthop.sin6_scope_id = rts->whereto6.sin6_scope_id;
@pevik
Copy link
Contributor

pevik commented Aug 29, 2022

not sure if there are other side effects.
@rnhmjoj unfortunately nobody can quickly find this. Given many regressions I tend to be careful :).

Does it mean that you need to call getnameinfo() on name?

iputils/ping/ping.c

Lines 1650 to 1651 in 1ab5faf

if (!rts->exiting && !rts->opt_numeric)
getnameinfo(sa, salen, name, sizeof name, NULL, 0, getnameinfo_flags);

Could please also post snippet of your dns server configuration which could be used for testing? Also could you check how ping from busybox and fping behaves on this?

@gobenji please any opinion on this?

@rnhmjoj
Copy link
Author

rnhmjoj commented Aug 29, 2022

Given many regressions I tend to be careful :).

Uhm, a safer alternative could be to add the opposite of -n, to force the lookup.

Could please also post snippet of your dns server configuration which could be used for testing?

Sure, I'm using bind with this zone definition

zone "0.0.0.0.0.0.0.0.0.0.0.0.0.8.e.f.ip6.arpa" { type master; file "/etc/bind/home.arpa"; };

and the content should look something like this:

$TTL 7d
@  IN  SOA  router.home.arpa. root.home.arpa. (2019091200 2h 30m 30d 1h)
@  IN  NS   router.home.arpa.
b.a.0.5.a.5.e.f.f.f.d.e:8.4.e.3 PTR     router.home.arpa.
8.5.0.3.a.0.e.f.f.f.0.8:c.4.6.8 PTR     ap-1.home.arpa.  
3.d.0.5.2.b.e.f.f.f.e.4:1.3.2.6 PTR     ap-2.home.arpa.  
1.8.0.2.1.7.e.f.f.f.6.3:5.f.6.4 PTR     me.home.arpa.    

(tip: to reverse the addresses you can use rev)

Also could you check how ping from busybox and fping behaves on this?

I don't think busybox ping supports doing reverse lookups, fping does but strangely
I can't get it to work at all on my computer.

@pevik
Copy link
Contributor

pevik commented Aug 29, 2022

Given many regressions I tend to be careful :).

Uhm, a safer alternative could be to add the opposite of -n, to force the lookup.

As you correctly found it was added by original author YOSHIFUJI Hideaki (who only knows in deep ICMP protocols and kernel networking internal) in f6bc8f6 (fix memchr() in 6754cb6).

Since 2007 ping refuses reverse lookup on numeric address. I have no idea why, but I suppose YOSHIFUJI Hideaki had strong reason for that. You might not like it, but with this change you introduce backward incompatibility, there must be a very strong reason for changing that.

@MJochim
Copy link

MJochim commented Nov 26, 2022

As you correctly found it was added by original author YOSHIFUJI Hideaki (who only knows in deep ICMP protocols and kernel networking internal) in f6bc8f6 (fix memchr() in 6754cb6).

I justed wanted to weigh in with a small comment that these commits did not change anything related to this issue (edit: they did, but not intentionally; the first commit of the two addresses a different issue, but introduces a bug by mixing up the parameters of memchr; the bug broke the reverse lookup behavior; the second commit fixes that bug, restoring the original behavior; the bug was only in the code for two months). The behavior was already present in the very first commit of this repo (3337034). I don’t know how much further back it dates.

As you can see here:

iputils/ping6.c

Lines 296 to 309 in 3337034

if (inet_pton(AF_INET6, target, &whereto.sin6_addr) <= 0) {
struct hostent *hp;
hp = gethostbyname2(target, AF_INET6);
if (hp == NULL) {
fprintf(stderr, "unknown host\n");
exit(2);
}
memcpy(&whereto.sin6_addr, hp->h_addr_list[0], 16);
} else {
options |= F_NUMERIC;
}

If inet_pton (3) returns 1, which it does if target is an IP address and not a host name, then the F_NUMERIC option is enabled. That option suppresses the reverse lookup of the IP address:

iputils/ping6.c

Lines 894 to 909 in 3337034

char * pr_addr(struct in6_addr *addr)
{
struct hostent *hp = NULL;
if (!(options&F_NUMERIC))
hp = gethostbyaddr((__u8*)addr, sizeof(struct in6_addr), AF_INET6);
return hp ? hp->h_name : pr_addr_n(addr);
}
char * pr_addr_n(struct in6_addr *addr)
{
static char str[64];
inet_ntop(AF_INET6, addr, str, sizeof(str));
return str;
}

All of that already in the initial commit.

@MJochim
Copy link

MJochim commented Nov 26, 2022

[...] ping refuses reverse lookup on numeric address. I have no idea why, but I suppose YOSHIFUJI Hideaki had strong reason for that. You might not like it, but with this change you introduce backward incompatibility, there must be a very strong reason for changing that.

I absolutely share @rnhmjoj’s wish for a reverse lookup, but I can see that a breaking change may be too much here. Maybe an “opposite of -n” flag would be a good compromise. (Although personally, I would like to see it as the default behavior.)

As for the reason behind the current behavior, I can only speculate, but I can add my two cents:

I assume that the original author wanted to avoid a – potentially expensive – name lookup when the user starts out with an IP address. After all, if a user types an IP address and not a host name, we could probably assume that they are not interested in host names. Avoiding the cost of a reverse lookup (actually many reverse lookups, see below) is then a very good idea.

Also, I am pretty sure I have sometimes come across a situation like this (but only rarely):

  1. My primary name server is very slow, or even unavailable (but the secondary name server is available and fast).
  2. I ping example.net and expect a fast response.
  3. But ping seems to hang, because it is waiting for the name server.
  4. (Initially, of course, ping has to resolve example.net, there’s no way around that).
  5. But then, for every echo request, ping performs another reverse lookup, each time waiting for the name server, so there is a huge lag after each echo request+response.
  6. I find my computer obnoxious, hit ctrl-c and do ping -n example.net instead.
  7. The initial lookup has to be performed again, but now the lag between the packets is gone.

You can easily test this by adding nameserver 10.0.0.100 (any made-up IP that is not an actual name server) to the top of your /etc/resolv.conf.

Any behavior like that would be all the more obnoxious when I ping, say, a local IP address and don’t have and don’t want host names on the local network. Hence it might be a good idea to default to “no lookups“ when I didn’t work with host names to begin with.

But then again, it has probably been very long since I experienced this. Maybe this whole line of reasoning, the idea that a name lookup is typically expensive, is just from a different era of networking. Whenever my name server is healthy, it feels very strange that ping behaves differently between ping example.net and ping 10.0.0.25.

@MJochim
Copy link

MJochim commented Nov 26, 2022

And I found the release note of this feature. Apparently it is 22 years old (2000-09-28):

* ping prints addresses in numeric, if destination is numeric.
Proposed by Tim Waugh <twaugh@meme.surrey.redhat.com>

Maybe @twaugh remembers the real reason why he proposed this. (We could try to email him, I don’t think this mention will work.)

@MJochim
Copy link

MJochim commented Nov 26, 2022

Last message from me: I also found two ten-year-old issues on Debian’s bug tracker of the iputils-ping package (reported by @vinc17fr):

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=650479 basically asks to change this behavior or document it.

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=650478 describes a fault condition that’s basically the same as the one I described, with the primary name server unavailable.

@twaugh
Copy link

twaugh commented Nov 28, 2022

I don't remember my reasoning for that I'm afraid. My current opinion is entirely different, ie. that the behaviour shouldn't depend on whether the destination is given as a hostname or an IP address.

@pevik pevik added this to the next release milestone Nov 28, 2022
@pevik
Copy link
Contributor

pevik commented Nov 28, 2022

I tend to be conservative to preserve old behavior, therefore I'd agree with suggestion to add a new option for opposite behavior of -n. (e.g. -N).

Also, I am pretty sure I have sometimes come across a situation like this (but only rarely):

  1. My primary name server is very slow, or even unavailable (but the secondary name server is available and fast).
    +1
  1. I ping example.net and expect a fast response.
    +1
  1. But ping seems to hang, because it is waiting for the name server.
    This will be considered by some people as bug, they will not know -n.
  1. (Initially, of course, ping has to resolve example.net, there’s no way around that).
  2. But then, for every echo request, ping performs another reverse lookup, each time waiting for the name server, so there is a huge lag after each echo request+response.
    +1, IMHO this should not be as a default.
  1. I find my computer obnoxious, hit ctrl-c and do ping -n example.net instead.
    IMHO ping should work by default "normally".
  1. The initial lookup has to be performed again, but now the lag between the packets is gone.

Also these reasons stated by MJochim are really valid. IMHO people expect when they pass the address, that ping works without DNS => again, I'm for a new option.

Looking at other tools: doing test how ping from busybox behaves, it does not do reverse lookup and don't have option for it. I need more time to have look at their str2sockaddr() implementation (https://git.busybox.net/busybox/tree/libbb/xconnect.c#n176).

fping has even 2 options for reverse lookup:
-d, --rdns show targets by name (force reverse-DNS lookup)
-n, --name show targets by name (reverse-DNS lookup for target IPs)

Which suggest by default there is no reverse lookup, people need to ask.

@MJochim
Copy link

MJochim commented Nov 28, 2022

As for other tools: arp and netstat default to doing reverse lookups, which you can disable with -n. But their newer counterparts, ip neigh and ss, default to not doing reverse lookups. You can enable them with -r[esolve].

@pevik
Copy link
Contributor

pevik commented Jul 14, 2023

Maybe we should follow the same approach and do reverse lookup only when asked. But -r and -R has already been used in ping :(. There is not much of the letters left, e.g. -g or -G, which is not really helping to remember the option. Also, ping does not use long opts, thus --resolve cannot be used atm.

@vinc17fr
Copy link

-H could be used as being the first letter of "hostname".

@pevik
Copy link
Contributor

pevik commented Jul 14, 2023

@nmeyerhans @gault any opinion on this issue?

@gault
Copy link
Contributor

gault commented Jul 17, 2023

@gault any opinion on this issue?

I think ping shouldn't assume the network is in good condition by default and therefore shouldn't rely on DNS if not necessary.
Also, changing an old established behaviour is always risky. So I'm all for a new option (-H, as suggested by vinc17fr looks like a good fit).

@pevik pevik self-assigned this Sep 27, 2023
@pevik
Copy link
Contributor

pevik commented Oct 1, 2023

Thank you all for your input. I'm going to add -H option to force resolving with getnameinfo(3) even for numeric address.

People often asks for the opposite: to disable using getnameinfo(3) for non-numeric address (e.g. in this ticket: #421 (comment)) or at least are confused by the default functionality when with broken DNS -n is needed (e.g. #491 (comment), #414). While I understand it'd solve many issues if this behavior was a default, I hesitate to change ping after many years of this functionality.

pevik added a commit to pevik/iputils that referenced this issue Oct 19, 2023
Implements: iputils#421
Suggested-by: Michele Guerini Rocco
Signed-off-by: Petr Vorel <pvorel@suse.cz>
pevik added a commit to pevik/iputils that referenced this issue Oct 19, 2023
Implements: iputils#421
Closes: iputils#494
Suggested-by: Michele Guerini Rocco
Signed-off-by: Petr Vorel <pvorel@suse.cz>
@pevik
Copy link
Contributor

pevik commented Oct 19, 2023

@rnhmjoj @gault @nmeyerhans @MJochim @vapier Could you please have look at #494? Feel free to post your Reviewed-by or Tested-by: tag.

@rnhmjoj Can you please email me your email address or post it here, so that I can add it to Suggested-by: tag? (your credit).

pevik added a commit to pevik/iputils that referenced this issue Oct 19, 2023
Forcing DNS name resolution is useful for numeric destination,
or -f option, which by default do not perform it.

It overrides -n option.

Implements: iputils#421
Closes: iputils#494
Suggested-by: Michele Guerini Rocco
Signed-off-by: Petr Vorel <pvorel@suse.cz>
@rnhmjoj
Copy link
Author

rnhmjoj commented Oct 19, 2023

Could you please have look at #494?

I tested it by pinging ff02::1 as in the original example and seems to works as expected.
It would be good to have this on by default, but I understand the necessity of backward compatibility.

@rnhmjoj Can you please email me your email address or post it here, so that I can add it to Suggested-by: tag? (your credit).

You can use rnhmjoj@inventati.org. Thank you!

pevik added a commit to pevik/iputils that referenced this issue Oct 20, 2023
Forcing DNS name resolution is useful for numeric destination,
or -f option, which by default do not perform it.

It overrides -n option.

Fixes: iputils#421
Fixes: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=650479
Closes: iputils#494
Reported-by: Vincent Lefevre <vincent@vinc17.net>
Reported-by: Michele Guerini Rocco
Signed-off-by: Petr Vorel <pvorel@suse.cz>
pevik added a commit to pevik/iputils that referenced this issue Oct 20, 2023
Forcing DNS name resolution is useful for numeric destination,
or -f option, which by default do not perform it.

It overrides -n option.

Fixes: iputils#421
Fixes: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=650479
Closes: iputils#494
Reported-by: Vincent Lefevre <vincent@vinc17.net>
Reported-by: Michele Guerini Rocco
Signed-off-by: Petr Vorel <pvorel@suse.cz>
pevik added a commit to pevik/iputils that referenced this issue Oct 20, 2023
Forcing DNS name resolution is useful for numeric destination,
or -f option, which by default do not perform it.

It overrides -n option.

Fixes: iputils#421
Fixes: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=650479
Closes: iputils#494
Reported-by: Vincent Lefevre <vincent@vinc17.net>
Reported-by: Michele Guerini Rocco
Reviewed-by: Vincent Lefevre <vincent@vinc17.net>
Signed-off-by: Petr Vorel <pvorel@suse.cz>
pevik added a commit to pevik/iputils that referenced this issue Oct 20, 2023
Forcing DNS name resolution is useful for numeric destination,
or -f option, which by default do not perform it.

It overrides -n option.

Fixes: iputils#421
Fixes: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=650479
Closes: iputils#494
Reported-by: Vincent Lefevre <vincent@vinc17.net>
Reported-by: Michele Guerini Rocco
Reviewed-by: Vincent Lefevre <vincent@vinc17.net>
Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
Reviewed-by: Guillaume Nault <guillaume.nault@wanadoo.fr>
Signed-off-by: Petr Vorel <pvorel@suse.cz>
pevik added a commit to pevik/iputils that referenced this issue Oct 20, 2023
On certain network setup getaddrinfo() can return empty ai_canonname
when forcing IP protocol version.  Instead of printing nothing in "PING"
line use the target.

This fixes the output of PING line:

$ ping seznam.cz -c1 -4 -v
...
ai->ai_family: AF_INET6, ai->ai_canonname: 'seznam.cz'
ai->ai_family: AF_INET6, ai->ai_canonname: ''
ai->ai_family: AF_INET, ai->ai_canonname: ''
PING  (77.75.79.222) 56(84) bytes of data.
64 bytes from www.seznam.cz (77.75.79.222): icmp_seq=1 ttl=55 time=4.82 ms

to
$ ping seznam.cz -c1 -4 -v
...
PING seznam.cz (77.75.79.222) 56(84) bytes of data.
64 bytes from www.seznam.cz (77.75.79.222): icmp_seq=1 ttl=55 time=4.88 ms

Implements: iputils#421
Link: iputils#478 (comment)
Reported-by: Michele Guerini Rocco <rnhmjoj@inventati.org>
Reported-by: Robert Scheck <robert@fedoraproject.org>
Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
Signed-off-by: Petr Vorel <pvorel@suse.cz>
pevik added a commit to pevik/iputils that referenced this issue Oct 20, 2023
On certain network setup getaddrinfo() can return empty ai_canonname
when forcing IP protocol version.  Instead of printing nothing in "PING"
line use the target.

This fixes the output of PING line:

$ ping seznam.cz -c1 -4 -v
...
ai->ai_family: AF_INET6, ai->ai_canonname: 'seznam.cz'
ai->ai_family: AF_INET6, ai->ai_canonname: ''
ai->ai_family: AF_INET, ai->ai_canonname: ''
PING  (77.75.79.222) 56(84) bytes of data.
64 bytes from www.seznam.cz (77.75.79.222): icmp_seq=1 ttl=55 time=4.82 ms

to
$ ping seznam.cz -c1 -4 -v
...
PING seznam.cz (77.75.79.222) 56(84) bytes of data.
64 bytes from www.seznam.cz (77.75.79.222): icmp_seq=1 ttl=55 time=4.88 ms

Implements: iputils#421
Link: iputils#478 (comment)
Reported-by: Michele Guerini Rocco <rnhmjoj@inventati.org>
Reported-by: Robert Scheck <robert@fedoraproject.org>
Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
Signed-off-by: Petr Vorel <pvorel@suse.cz>
pevik added a commit to pevik/iputils that referenced this issue Oct 20, 2023
Forcing DNS name resolution is useful for numeric destination,
or -f option, which by default do not perform it.

It overrides -n option.

Fixes: iputils#421
Fixes: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=650479
Closes: iputils#494
Reported-by: Vincent Lefevre <vincent@vinc17.net>
Reported-by: Michele Guerini Rocco
Reviewed-by: Vincent Lefevre <vincent@vinc17.net>
Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
Reviewed-by: Guillaume Nault <guillaume.nault@wanadoo.fr>
Signed-off-by: Petr Vorel <pvorel@suse.cz>
@pevik pevik closed this as completed in dd5a81a Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants