Prefer default route when checking local interface addresses
First, let us assume that all network interfaces for IP host that runs Tor instance are internal as judged by tor_addr_is_internal() function.
There is the following code in get_interface_address6() function.
/* Try to do this the smart way if possible. */
if ((addrs = get_interface_addresses_raw(severity))) {
int rv = -1;
SMARTLIST_FOREACH_BEGIN(addrs, tor_addr_t *, a) {
if (family != AF_UNSPEC && family != tor_addr_family(a))
continue;
if (tor_addr_is_loopback(a) ||
tor_addr_is_multicast(a))
continue;
tor_addr_copy(addr, a);
rv = 0;
/* If we found a non-internal address, declare success. Otherwise,
* keep looking. */
if (!tor_addr_is_internal(a, 0))
break;
} SMARTLIST_FOREACH_END(a);
SMARTLIST_FOREACH(addrs, tor_addr_t *, a, tor_free(a));
smartlist_free(addrs);
return rv;
}
Caller will get the last entry from a interface address smartlist. Is this okay?
Activity
changed milestone to %Tor: unspecified
Trac:
Cc: N/A to rl1987@sdf.lonestar.org
According to the comment, it's doing it's job but I'm starting to wonder if it's actually used right.
/** Set *<b>addr</b> to the IP address (if any) of whatever interface * connects to the Internet.In connection.c, function
client_check_address_changed(), it's used to get the "last interface ip" (whichget_interface_address6()provides) and see if we've seen it before by looking intooutgoing_addrs.Let's assume we have eth0 with one public IPv6 address,
1::cafeget_interface_address6()will return that address since it fits the criteria and is the first valid public IP we see. Sooutgoing_addrsis updated with it and we live happily after. Lets add a second public usable IPv6,2::cafe.Now calling again
get_interface_address6()should return the first valid one which is1::cafe(considering here alphabetical order) thus disregarding the new IP and makingclient_check_address_changed()failing to notice.Lots of pieces here, I might be missing a check thus need review :). If it's true, maybe the solution is to keep a list of all valid IPs of any usable interface and validate our state that way.
Trac:
Parent: #12376 (moved) to N/AReplying to dgoulet:
Lots of pieces here, I might be missing a check thus need review :). If it's true, maybe the solution is to keep a list of all valid IPs of any usable interface and validate our state that way.
So if the purpose of the routine is "get the address used to reach the internet", then keeping a list like that isn't a great solution. The "correct" way to do this is non-portable, but there's "correct" ways for all the platforms we care about (and can fall back to the hack as needed...).
Under Linux, you want a rtnetlink(7) socket. Under *BSD, this information is available from sysctl (
net.route.0.0.dump) Under Windows, you wantGetIpForwardTable()(https://msdn.microsoft.com/en-us/library/aa365953%28v=vs.85%29.aspx).The high level "right" way is to use platform specific calls to dump the routing table, find the default route and the associated interface, and return the interface IP address.
A brief description of how the linux solution would work:
- Open a rtnetlink socket (AF_NETLINK, PF_ROUTE).
- Query the entire routing table with
RTM_GETROUTE, and find the default route and it's associated interface index (RTA_IIF, RTA_OIF). - Query the list of interfaces to get the address
RTM_GETADDR. You may be able to get away with looking at the RTA_SRC attribute in the response from 2, it's been a while so I don't remember off the top of my head. - Profit!
It's a decent chunk of code, especially if you do all platforms, but it should be fun to write.
Replying to nickm:
If this is a matter of the code behaving not-too-badly but the tests being not-quite-right, then can we arrange to have the tests detect this situation, and tt_skip() rather than failing?
It's a matter of the tests being correct, the code being flat out wrong, with the tests being new and always failing because the code is wrong and has always been wrong.
The 2 values the tests are comparing are the "smart way" of getting the external interface address which isn't routing table aware (which breaks triggering test failure), and the kludgy way (open a UDP socket destined for an external host, which the kernel assigns to the correct interface based on the internal routing table) which is correct.
The correct thing to do here would be to make the "determine our address with external connectivity based on enumerating interfaces" be routing table aware, but that requires writing 3 different implementations (at a minimum).
Till this is fixed (correctly) I suggest merging something like:
https://github.com/Yawning/tor/compare/bug12377_workaround
This disables tests that fail due to this bug. The tests are new, the bug is old. My comment #8 (closed) has the correct way to fix get_interface_address6().
This is an example of how to rewrite all of this code, to do the mostly right thing on Linux (For anything more recent than kernel 2.2 definitions of "more recent"). This supercedes the
getifaddrcode (which is broken), and is superior to the UDP trick since it doesn't make a random UDP socket.Adapting it to fit into tor is left as an exercise for the student:https://gist.github.com/Yawning/c70d804d4b8ae78cc698The same socket type can also be used to have the kernel tell us when the IP address/routing table changed, which may be useful to some people.
Edit: I lied about leaving it to the student.
NB: Yes the code is ugly, I sort of threw it together. No I will not do Darwin or Windows.
First pass at fixing this, at least for some systems: https://github.com/Yawning/tor/compare/bug12377_linux
Quirks:
- Because RTM_GETADDR on Linux sux, it has to dump every single address, of every single interface.
- It will always return the primary address associated with the interface, not any aliases or non-prefered IPv6 addresses.
- It uses the presence of a default route to determine which interface's address to return.
- It only works on Linux for obvious reasons.
- It carves out a new AF_NETLINK socket per request, and does blocking I/O. Not ideal, but probably ok, since this isn't anything critical path, and something is obviously horrifically broken if the kernel doesn't respond to the inquiries immediately.
This can be improved one step further, by ripping out the poll driven address determination code and just having the kernel send events on an AFN_NETLINK socket, but that's obviously more involved.
TODO: Needs more comments, though how it works should be obvious.