Make sure every address function that takes for_listening supports IPv6

We need to make sure all of our basic address functions support IPv6.

For example, tor_addr_is_valid() only supports IPv4 for_listening.

We need to make this change before we create generic IPv6 listeners for proposal 312.

changed milestone to %Tor: 0.4.4.x-final

Trac:
Parent Ticket: #33049 (moved)

added actualpoints::0.3 component::core tor/tor extra-review ipv6 milestone::Tor: 0.4.4.x-final network-team-roadmap-2020Q1 outreachy-ipv6 owner::MrSquanchee parent::33049 points::0.5 priority::medium prop312 resolution::fixed reviewer::dgoulet severity::normal sponsor::55-must status::closed type::task labels

Thanks for making this ticket in reference to https://trac.torproject.org/projects/tor/ticket/33618#comment:8 I think I may be helpful here. Thanks, MrSquanchee.

Trac:
Owner: N/A to MrSquanchee
Status: new to assigned

Make sure every address function that takes for_listening supports IPv6

I see only one such function.

You can see my PR here https://github.com/torproject/tor/pull/1831

Thanks,

MrSquanchee.

Trac:
Status: assigned to needs_review

Replying to MrSquanchee:

Make sure every address function that takes for_listening supports IPv6

I see only one such function.

There are at least 5 such functions. Of the ones I checked, 3 only support IPv4, and 1 already supports IPv6 for_listening. Please double-check by searching address.c for for_listening.

You can see my PR here https://github.com/torproject/tor/pull/1831

Thanks!

Please update the function comment so it describes what the function does now.

And please add tests for the new code you just added.

It would also be great to simplify the IPv4 case the same way you simplified the IPv6 case. (If for_listening is true, any IPv4 address is acceptable.)

And merge the IPv4 and IPv6 cases, if you can find a simple way to do it.

Trac:
Status: needs_review to needs_revision

Hii teor,

My updated PR[https://github.com/torproject/tor/pull/1831].

I have added the required tests. And I didn't see any reasons to change tor_port_is_valid(). However I refactored tor_addr_is_valid_ipv4n() and We should not make a similar function for ipv6 as it will only be checking whether it's null or not and that could be done via tor_is_valid(). Should we make a function similar to tor_addr_is_valid_ipv4h() ??

Trac:
Status: needs_revision to needs_review

Should we make a function similar to tor_addr_is_valid_ipv4h I provided functions (network and host order) for both ipv4 and ipv6.

int
tor_addr_is_valid_ipvn(const tor_addr_t *addr, int for_listening) {
  return tor_addr_is_valid(addr, for_listening);
}

int
tor_addr_is_valid_ipvh(const tor_addr_t *addr, int for_listening) {
  tor_assert(addr);

  if (for_listening) {
    return 1;
  }

  if (addr->family == AF_INET) {
    return htonl(addr->addr->in_addr) != 0;
  } else if (addr->family == AF_INET6) {
    uint32_t *ipv6 = tor_addr_to_in6_addr32(addr);
    return !(htonl(ipv6[0]) == 0 &&
             htonl(ipv6[1]) == 0 &&
             htonl(ipv6[2]) == 0 &&
             htonl(ipv6[3]) == 0);
  } else {
    return 1;
  }
}

The other only ipv4 functions are tor_addr_port_is_valid_ipv4[h/n]. I think we should leave them as it is. What do you say ?? Should I push this with some tests on tor_addr_is_valid_ipv[h/n] ??

"n" stands for network-order, and "h" stands for host order. tor_addr_t is always in host order. So we don't need these extra functions. When we are refactoring, or adding features, we don't add extra functions. Unless we are going to actually use them.

I put some more comments on the pull request.

Thanks for trying to make these functions simpler. But they also need to be correct, and readable. Sometimes a function is more readable, if it is written using multiple lines.

Let's just focus on changing and testing tor_addr_is_valid() for now. I think the other functions are fine as they were.

Thanks for suggestions, updating the PR soon !!!

Trac:
Status: needs_review to needs_revision

Add all the tickets from sponsor 55 that are done and being worked on to the keyword #network-team-roadmap-2020Q1 so I can look at them in the wiki page...

Trac:
Keywords: N/A deleted, network-team-roadmap-2020Q1 added

Made all the changes requested and improved test for tor_addr_is_valid().

Github PR #1831.

Trac:
Status: needs_revision to needs_review

Just letting you know that I've seen these changes, and I'll try to review them some time in the next day or so.

Trac:
Reviewer: N/A to teor

For what it's worth appveyor seems broken with:

addr/address_validity: 
  FAIL ../src/test/test_addr.c:1730: assert(tor_addr_is_valid(test_addr, 0) OP_EQ 1): 0 vs 1
  [address_validity FAILED]

teor, we will let you figure if you want to move this to needs_revision!

Replying to asn:

For what it's worth appveyor seems broken with: {{{ addr/address_validity: FAIL ../src/test/test_addr.c:1730: assert(tor_addr_is_valid(test_addr, 0) OP_EQ 1): 0 vs 1 [address_validity FAILED] }}}

teor, we will let you figure if you want to move this to needs_revision!

Hii asn,

The above line of code works fine with linux.

Also the test case is reasonable in the context.

I don't really have a clue why it is failing in appveyor and

passing in travis ci. Could you suggest something ??

The test has an invalid IPv6 address,

It looks like the data returned is different on Windows vs macOS/Linux, when tor_inet_pton() gets some invalid addresses. I opened #33768 (moved) to fix this bug - the functions should behave the same across all platforms.

Trac:
Status: needs_review to needs_revision

Replying to teor:

The test has an invalid IPv6 address,

Thanks teor,

The PR is all green now :) You can review it here.

Replying to teor:

It looks like the data returned is different on Windows vs macOS/Linux, when tor_inet_pton() gets some invalid addresses. I opened #33768 (moved) to fix this bug - the functions should behave the same across all platforms.

Yeah !! sometimes stupidity proves to be useful. haha.

Trac:
Status: needs_revision to needs_review