Opened 3 years ago

Closed 3 years ago

#17950 closed enhancement (implemented)

Make address family search more accurate

Reported by: teor Owned by: rl1987
Priority: Medium Milestone: Tor: 0.2.8.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: lorax easy
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Tor searches local interfaces for IPv4/IPv6 addresses, but it often retrieves all addresses, then filters for IPv4/IPv6.

We could make this more efficient for some of the interface address functions:

  • getifaddrs doesn't take an address family, but we can check the address families of the returned addresses
  • ioctl(.,SIOCGIFCONF,.) only supports AF_INET6 on AIX, or on HP-UX and Solaris with SIOCGLIFCONF, and otherwise only returns IPv4 addresses
  • GetAdaptersAddresses (Win32) takes an address family as its first argument
  • tor_getsockname/get_interface_address6_via_udp_socket_hack takes an address family as its first argument

A design for this could be:

  • pass the address family to get_interface_addresses_raw
  • pass the address family to the API-specific functions that take an address family, or when converting the address to a smartlist, include/exclude addresses matching the specified address families

Child Tickets

Change History (13)

comment:1 Changed 3 years ago by rl1987

Owner: set to rl1987
Status: newaccepted

comment:2 Changed 3 years ago by teor

Milestone: Tor: 0.2.???Tor: 0.2.8.x-final
Summary: Make address family search more efficientMake address family search more accurate

I split off the ioctl AF_INET6 modifications to #17952, they're lower priority as they only affect some obscure platforms.

As discussed on IRC, if the function is passed AF_UNSPEC, it expects IPv4 and IPv6 addresses back. So some functions might need to be called twice with AF_INET and AF_INET6 if they don't accept AF_UNSPEC. This also includes #17951 - the socket hack should be called twice with AF_INET and AF_INET6 if AF_UNSPEC is passed.

Moving to 0.2.8 because this is a correctness patch.

comment:3 Changed 3 years ago by rl1987

Milestone: Tor: 0.2.8.x-finalTor: 0.2.???
Status: acceptedneeds_review
Summary: Make address family search more accurateMake address family search more efficient

comment:4 Changed 3 years ago by rl1987

Milestone: Tor: 0.2.???Tor: 0.2.8.x-final
Summary: Make address family search more efficientMake address family search more accurate

comment:5 Changed 3 years ago by teor

Status: needs_reviewneeds_revision

Thanks for this patch, it looks clean.

For the ioctl case:

The function should always pass AF_INET to the ioctl. But it should only bail out when family is AF_INET6, as it's still useful to get IPv4 addresses in response to an AF_UNSPEC.

comment:6 Changed 3 years ago by rl1987

Pushed one more commit that tweaks ioctl case.

comment:7 Changed 3 years ago by teor

Status: needs_revisionneeds_review

Looks great, let's get it merged!
(And thanks for doing this, it's going to be really helpful.
Want to work on #17951 next? It's a similar fix for the UDP socket hack, where AF_UNSPEC should lead to the socket hack being called twice, one for AF_INET and once for AF_INET6.)

comment:8 Changed 3 years ago by nickm

+    if (family != AF_UNSPEC && i->ifa_addr->sa_family != family)
       continue;

This means that in addition to AF_INET and AF_INET6 addresses, AF_UNSPEC will get you anything else that tor_addr_from_sockaddr can decode, including (for now) AF_UNIX addresses. How do we feel there? I'm worried that we might have some code that uses these addresses that assumes (for example) that everything is IPv6 or otherwise IPv4.

comment:9 Changed 3 years ago by teor

Status: needs_reviewneeds_revision

Let's keep the old conditional just to be safe - it will make sure that AF_UNSPEC only stands for IPv4 and IPv6 in ifaddrs_to_smartlist.

    if (i->ifa_addr->sa_family != AF_INET &&
        i->ifa_addr->sa_family != AF_INET6)
      continue;

Edit: three braces

Last edited 3 years ago by teor (previous) (diff)

comment:10 Changed 3 years ago by rl1987

Pushed one more commit that re-adds the above changes.

comment:11 Changed 3 years ago by rl1987

Status: needs_revisionneeds_review

comment:12 Changed 3 years ago by teor

Looks great, I'm happy with this, let's get it merged.

comment:13 Changed 3 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Merged!

Note: See TracTickets for help on using tickets.