haesbaert / mdnsd

Mdns daemon for OpenBSD.
www.haesbaert.org/openmdns
135 stars 27 forks source link

IPv6 MDNS #35

Closed twinshadow closed 7 months ago

twinshadow commented 4 years ago

This PR is split into 3 commits, but they may not be entirely distinct. It basically goes like this:

  1. Add support for interfaces to store multiple IP addresses.
  2. Add support for using multiple addresses in service and control functions.
  3. Add support for sending and receiving IPv6 packets.

I've kept my FreeBSD patch in here as well, since I tested this with FreeBSD-current as well as OpenBSD-current.

Samples

Lookup of IPv6 addresses from mdnsctl

$ mdnsctl lookup Android.local
Address: 192.168.86.164
$ mdnsctl lookup -6 Android.local
Address: fe80::aa6b:adff:fe16:13e7

mdnsd advertisiment appearing on mdns-sd from another system. tutti.local is running mdnsd with these patches.

dns-sd -G v4v6 tutti.local
DATE: ---Thu 07 Nov 2019---
20:57:57.205  ...STARTING...
Timestamp     A/R Flags if Hostname                               Address                                      TTL
20:57:57.207  Add     3  1 tutti.local.                           FDCC:F80E:8097:0000:0000:0000:0000:0001%<0>  120
20:57:57.208  Add     2  1 tutti.local.                           192.168.32.5                                 120

Issues


Fixes #4

Cheers!

twinshadow commented 4 years ago

Yup, my patch does the wrong thing, and should be doing sizeof(&rtm) instead to accomplish the same effect.

On November 30, 2019 4:14:46 PM PST, Nick Briggs notifications@github.com wrote:

nbriggs commented on this pull request.

+#ifdef FreeBSD +#define rtm_hdrlen rtm_msglen +#endif

I suspect this is wrong -- on OpenBSD rtm_hdrlen is documented as being

  u_short rtm_hdrlen; /* sizeof(rt_msghdr) to skip over the header */

while rtm_msglen on both FreeBSD and OpenBSD is documented as being

  u_short rtm_msglen; /* to skip over non-understood messages */

so at kiface.c:150

               sa = (struct sockaddr *)(buf + rtm->rtm_hdrlen);
               get_rtaddrs(rtm->rtm_addrs, sa, rti_info);

you'll be skipping the whole message rather than just the header of the message.

-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/haesbaert/mdnsd/pull/35#pullrequestreview-324914198

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

nbriggs commented 4 years ago

sizeof(*rtm) rather than sizeof(&rtm) if you're not going to use sizeof(struct rt_msghdr)

But even with that correction, it still takes a segmentation fault (on FreeBSD) if you add or delete an address on the interface that mdnsd is monitoring.

nbriggs commented 4 years ago

The documentation for FreeBSD route(4) says

The RTM_IFINFO message uses a if_msghdr header, the RTM_NEWADDR and
RTM_DELADDR messages use a ifa_msghdr header, the RTM_NEWMADDR and
RTM_DELMADDR messages use a ifma_msghdr header, the RTM_IFANNOUNCE
message uses a if_announcemsghdr header, and all other messages use the
rt_msghdr header.

So while you can walk the messages by the rtm_msglen you have to get the type of message to know what the header length is -- a problem you don't have in OpenBSD because you've got the variable header length in the common part of the header(s).

twinshadow commented 4 years ago

In OpenBSD, struct rt_msghdr->rtm_addrs and struct ifa_msghdr->ifam_addrs point to the same offset in the buffer. This is not true in FreeBSD, so the bitmask wasn't being passed to get_rtaddrs with the right value, even though the initial offset was correct.

nbriggs commented 4 years ago

No SIGSEGVs now. It does get a bit confused if you delete the only IPv4 address off the interface it's listening on -- a situation that wouldn't have made sense before IPv6 support.

twinshadow commented 4 years ago

No SIGSEGVs now. It does get a bit confused if you delete the only IPv4 address off the interface it's listening on -- a situation that wouldn't have made sense before IPv6 support.

I've added some patches to tone-down the warnings from lack of IPv4 addresses. Still working on a patches to respond to remote DNS queries with all IP addresses and update the cache when changes occur to local interfaces.

twinshadow commented 4 years ago

I just realized that bug #31 will more easily trigger with this PR, since more addresses are added to the fixed-size array. I'll have to add a fix for that here.

twinshadow commented 4 years ago

DNS responses from the daemon now send all addresses from all interfaces and can respond to mdnsctl with the appropriate address from any interface (eg. em0 being IPv6 only and bridge0 being IPv4 only). I've tested out the MDNS packet changes with mdns-sd on NetBSD.

The only other things I can think to commit are style, grammar, and comment changes before rebasing everything for merging.