[ping6] segmentation fault when using argument -g or several hops

[maryse.levavasseur_stormshield.eu]

Summary

Since FreeBSD10, only one struct addrinfo is used in ping6.
But in case of option -g, another one must be used to store gateway addrinfo.

In detail, when we call "ping6 host":

1. the program calls getaddrinfo() for "host" and stores it into "res" which will be used later
2. the program uses the struct "res" to send an ICMP request to "host"

But when we call "ping6 -g gateway host":

1. the program calls getaddrinfo() for "host" and stores it into "res" which will be used later
2. if -g is set a) the program calls getaddrinfo() for "gateway"; b) it stores addrinfo of "gateway" into "res", which overwrites addrinfo of "host"; c) uses addrinfo of "gateway" immediately in setsockopt(); d) then it makes freeaddrinfo(res);
3. the program uses the struct "res" to send an ICMP request to "host", expecting that "res" contains addrinfo of "host"

... but "res" has been overwritten in step 2b (and freed in step 2d)

The given patch fixes this case, and the case of using hops too.
I have used the same variable names as in FreeBSD 9.3 .

Test Plan

Our automated tests use option -g, and they work fine since this patch is integrated, and they show no regression.

[araujo]

Hey,

Could you explain a little bit more verbose this sentence "But in case of option -g, another one must be used to store gateway addrinfo.".
What kind of issue it addresses besides the clear segfault?

Best,

[maryse.levavasseur_stormshield.eu]

diff with full context

[maryse.levavasseur_stormshield.eu]

Hi,
I have given more information in summary.
If you have more questions, please let me know.
Regards

[araujo]

Hi,

I took a look and made some tests with and without the patch.
The patch makes sense and your line of thoughts/logic too.

Seems here is where we have these changes:
https://svnweb.freebsd.org/base/head/sbin/ping6/ping6.c?r1=271909&r2=271910&

But still I can't simulate nor have any segmentation fault with it.
As an example my tests with/without patch:

root@srcCODE:/usr/src/sbin/ping6 # ping6 -g ::1 2001:db8:4672:6565:2026:5043:2d42:5344
PING6(56=40+8+8 bytes) 2001:db8:4672:6565:2026:5043:2d42:5344 --> 2001:db8:4672:6565:2026:5043:2d42:5344
ping6: sendmsg: No route to host
ping6: wrote 2001:db8:4672:6565:2026:5043:2d42:5344 16 chars, ret=-1
ping6: sendmsg: No route to host
ping6: wrote 2001:db8:4672:6565:2026:5043:2d42:5344 16 chars, ret=-1
ping6: sendmsg: No route to host
ping6: wrote 2001:db8:4672:6565:2026:5043:2d42:5344 16 chars, ret=-1

Following your lines of thoughts, I was expecting a icmp6 on ::1 that is the gateway and will become the host because of the free().

I'm using FreeBSD HEAD revision r301478M, with and without your patch the behave is the same.

I'm waiting your comments, also how did you make your setup to detect this problem.

[maryse.levavasseur_stormshield.eu]

Hi araujo,

Our system blanks the memory we free, then when ping6 tries to reuse the struct addrinfo previously freed, we have a segfault.

Your ping6 displays that it is pinging 2001:db8:4672:6565:2026:5043:2d42:5344,
but are you sure that it has sent an ICMP packet to this address ?
Have you checked with a tcpdump ?

[maryse.levavasseur_stormshield.eu]

For your information, I will not be able to answer your questions until July 19th.
Regards,

[araujo]

OK, I made more tests and debugs and this patch is right.

Thanks for the detailed report, I'm trying to include this patch on 11-RELEASE.

Best,