-
Notifications
You must be signed in to change notification settings - Fork 1
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
enumerate ipv6 addresses #79
base: main
Are you sure you want to change the base?
Conversation
Fix a subtle bug where an Ord instance for ConnectionHint casused a hint to be dropped. While comparing two Direct hints, if they are deemed equal, then while adding them to a set, one of them get dropped out. This happens when an interface has an ipv4 and ipv6 listening socket and we enumerate interfaces and send hints to the other side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Sorry about the very long turn-around. Some comments inline. I think there are a couple potential misbehaviors and a couple places where we'll benefit a lot from some kind of automated tests (mostly to do with system integration, unfortunately...).
@@ -82,7 +82,7 @@ data ConnectionHint | |||
deriving (Eq, Show, Generic) | |||
|
|||
instance Ord ConnectionHint where | |||
Direct _ `compare` Direct _ = EQ | |||
Direct _ `compare` Direct _ = LT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks odd. Can you elaborate?
@@ -98,8 +98,11 @@ tcpListener :: Bool -> IO (Maybe Socket) | |||
tcpListener True = return Nothing | |||
tcpListener False = do | |||
let hints' = defaultHints { addrFlags = [AI_NUMERICHOST], addrSocketType = Stream } | |||
addr:_ <- getAddrInfo (Just hints') (Just "0.0.0.0") (Just (show defaultPort)) | |||
sock' <- socket (addrFamily addr) (addrSocketType addr) (addrProtocol addr) | |||
-- getAddrInfo gives us a list of addresses. With AI_ADDRCONFIG, we |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we didn't use AI_ADDRCONFIG though - only AI_NUMERICHOST (which is now perhaps obsolete since we don't pass an address in at all)?
Also the upstream docs sound a little different from this:
If hints.ai_flags includes the AI_ADDRCONFIG flag, then IPv4 addresses are returned in the list pointed to by res only if the local system has at least one IPv4 address configured, and IPv6 addresses are returned only if the local system has at least one IPv6 address configured.
-- get one for IPv4 and another for IPv6 | ||
addr:_ <- getAddrInfo (Just hints') Nothing (Just (show defaultPort)) | ||
-- Always use AF_INET6 so that the socket listens on both IPv4 and IPv6 | ||
sock' <- socket AF_INET6 (addrSocketType addr) (addrProtocol addr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. I think it depends on local system configuration whether this actually binds both AF_INET and AF_INET6. For example, Linux has /proc/sys/net/ipv6/bindv6only (https://man7.org/linux/man-pages/man7/ipv6.7.html). Apparently this defaults to false nowadays ... so maybe this is fine? At least on Linux. I'm not sure about macOS and Windows though.
Another case to consider - what if the system doesn't have any local IPv6 addresses? Does this still work to bind to IPv4?
This is probably something it would be good to have CI covering since otherwise the potentially diverging behavior of multiple platforms makes it really difficult to get consistent results.
, port = fromIntegral portnum | ||
, priority = 0 | ||
, ctype = DirectTcpV1 }) nonLoopbackInterfaces | ||
filter (\nwInterface -> name nwInterface /= "lo" && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alas, there can be multiple loopback interfaces with different names (lo0, lo1, etc, are common).
Also! They don't even have to have the address "127.0.0.1" - at least on Linux. Linux considers 127.0.0.0/8 loopback. So the old version is also probably not entirely correct.
It's probably okay to punt this to a future issue. Could you file one and link to it from here?
, ctype = DirectTcpV1 }) nonLoopbackInterfaces | ||
filter (\nwInterface -> name nwInterface /= "lo" && | ||
(show (ipv4 nwInterface) /= ("0.0.0.0" :: Text) || | ||
show (ipv6 nwInterface) /= ("0:0:0:0:0:0:0:0" :: Text))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://hackage.haskell.org/package/gi-gio-2.0.30/docs/GI-Gio-Objects-InetAddress.html#v:inetAddressGetIsAny would probably be a good way to check this ... except that it's from gi-gio. Also the implementation looks like a cheater (just compares against 0 instead of INADDR_ANY, IN6ADDR_ANY, etc). Though it will probably always get away with it.
You could probably compare against the Word32s inside IPv4 and IPv6 here ... but ... that's probably not really a win over this version. Oh well. Nothing to do here I guess.
[ Direct Hint { hostname = show (ipv4 nwInterface) | ||
, port = fromIntegral portnum | ||
, priority = 0 | ||
, ctype = DirectTcpV1 } | ||
, Direct Hint { hostname = show (ipv6 nwInterface) | ||
, port = fromIntegral portnum | ||
, priority = 0 | ||
, ctype = DirectTcpV1 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Above we filtered the interfaces to find any that had a real address for either IPv4 or IPv6 - but here we take that list and we put both IPv4 and IPv6 addresses from all interfaces into the hints. It seems like this will put INADDR_ANY / IN6ADDR_ANY into the hints for some interfaces?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DirectTcpV1
can have either kind of address -- if that's the question :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That part sounds good. I think this code will put "0.0.0.0" into a DirectTcpV1
hint sometimes though - which seems wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, yeah that sounds wrong to me too
enumerate and send ipv6 addresses as well in the direct hint list
This change is