Opened 7 months ago

Closed 7 months ago

#33679 closed task (fixed)

Make sure every address function that takes for_listening supports IPv6

Reported by: teor Owned by: MrSquanchee
Priority: Medium Milestone: Tor: 0.4.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: extra-review, prop312, ipv6, outreachy-ipv6, network-team-roadmap-2020Q1
Cc: Actual Points: 0.3
Parent ID: #33049 Points: 0.5
Reviewer: dgoulet Sponsor: Sponsor55-must

Description

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.

Child Tickets

Change History (24)

comment:1 Changed 7 months ago by MrSquanchee

Owner: set to MrSquanchee
Status: newassigned

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.

comment:2 Changed 7 months ago by MrSquanchee

Status: assignedneeds_review

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.

comment:3 in reply to:  2 Changed 7 months ago by teor

Status: needs_reviewneeds_revision

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.

comment:4 Changed 7 months ago by MrSquanchee

Hii teor,

My updated PRhttps://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() ??

Last edited 7 months ago by MrSquanchee (previous) (diff)

comment:5 Changed 7 months ago by MrSquanchee

Status: needs_revisionneeds_review

comment:6 Changed 7 months ago by MrSquanchee

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] ??

Last edited 7 months ago by MrSquanchee (previous) (diff)

comment:7 Changed 7 months ago by teor

"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.

Last edited 7 months ago by teor (previous) (diff)

comment:8 Changed 7 months ago by MrSquanchee

Status: needs_reviewneeds_revision

Thanks for suggestions, updating the PR soon !!!

comment:9 Changed 7 months ago by gaba

Keywords: network-team-roadmap-2020Q1 added

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...

comment:10 Changed 7 months ago by MrSquanchee

Status: needs_revisionneeds_review

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

Github PR #1831.

comment:11 Changed 7 months ago by teor

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.

comment:12 Changed 7 months ago by dgoulet

Reviewer: teor

comment:13 Changed 7 months ago by 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!

comment:14 in reply to:  13 Changed 7 months ago by MrSquanchee

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 ??

comment:15 Changed 7 months ago by teor

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 to fix this bug - the functions should behave the same across all platforms.

comment:16 Changed 7 months ago by teor

Status: needs_reviewneeds_revision

comment:17 in reply to:  15 Changed 7 months ago by MrSquanchee

Status: needs_revisionneeds_review

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 to fix this bug - the functions should behave the same across all platforms.

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

comment:18 Changed 7 months ago by teor

Actual Points: 0.3
Keywords: extra-review added
Reviewer: teor

Now that we have unit tests, it is easier to simplify the function a bit more:
https://github.com/torproject/tor/pull/1848

I think the check for the 0.0.0.0 IPv4 address was redundant.

I added this commit, what do you think?
https://github.com/torproject/tor/pull/1848/commits/ba68603d648d28d22e89acf1be8ea3c590836daf

Now that the pull request is finished, someone else will review it, then it will be merged. That should take about a week or so.

comment:19 in reply to:  18 Changed 7 months ago by MrSquanchee

Replying to teor:

I think the check for the 0.0.0.0 IPv4 address was redundant.
I added this commit.

Thanks for making the comments clearer.
And yes it was redundant.

Now that the pull request is finished, someone else will review it, then it will be merged. That should take about a week or so.

Looking forward to it !!

comment:20 Changed 7 months ago by dgoulet

Reviewer: dgoulet

comment:21 Changed 7 months ago by dgoulet

Status: needs_reviewneeds_revision

Code looks good. The only problem I have is with the git history.

For instance, I do not know what the fixup commits are against which commits? :S

May I suggest here to just rebase the branch against master, squash most of the commits into 2 commits basically, one for the IPv6 functionality and second one for the added tests?

If you need help with this, let me know, I can propose a branch and you can tell me if it works for you.

Thanks!

comment:22 Changed 7 months ago by MrSquanchee

Status: needs_revisionneeds_review

Hii David,

I made changes requested.
You can view my Pull request here at https://github.com/torproject/tor/pull/1831

comment:23 Changed 7 months ago by dgoulet

Status: needs_reviewmerge_ready

Awesome, once CI passes, I'll merge. Thanks!

comment:24 Changed 7 months ago by dgoulet

Resolution: fixed
Status: merge_readyclosed

Merged!

Note: See TracTickets for help on using tickets.