[tor-bugs] #9729 [Tor]: Make bridges publish additional ORPort addresses in their descriptor

[Tor Bug Tracker & Wiki]

#9729: Make bridges publish additional ORPort addresses in their descriptor
-------------------------+-------------------------------------------------
     Reporter:  sqrt2    |      Owner:
         Type:           |     Status:  needs_revision
  enhancement            |  Milestone:  Tor: 0.2.6.x-final
     Priority:  normal   |    Version:  Tor: 0.2.5.1-alpha
    Component:  Tor      |   Keywords:  ORPort, bridge, multiple,
   Resolution:           |  addresses, 025-triaged
Actual Points:           |  Parent ID:
       Points:           |
-------------------------+-------------------------------------------------

Comment (by daube):

 See inline updates below. I'll keep editing this comment as I finish
 things off.

 daube

 Replying to [comment:29 nickm]:
 > Here's a preliminary code review.
 >
 > In address.c:
 >   * The code in get_stable_interface_address6 seems to assume that
 'family' is AF_INET or AF_INET6.  Maybe it should check that early on.
 Fixed
 >   * It would probably be good idea to have a function to malloc and copy
 a single address; that pattern happens a lot.
 Implemented, it's called tor_addr_clone:
 `tor_addr_t *tor_addr_clone(const tor_addr_t *src);`
 >   * get_stable_interface_address6 looks like it could benefit from the
 "goto end" pattern of having a single point of cleanup.
 >
 > In config.c:
 >   * We use tor_assert, not assert().
 Fixed
 >   * XXX come back and re-review resolve_my_address.  It's too
 complicated. :(
 >
 > In dirserv.c:
 >   * Wow, that conditional in dirserv_set_router_is_running is pretty
 complicated.  Can we turn it into a function?
 Refactored out. We now check if a router is contactable in a static
 function. There's also a new static helper around
 `node_af_reachable_since` that takes care of IPv4 & IPv6 called
 `node_reachable_since`.
 >   * Local variables named "l" are begging for confusion with numberes
 named "1".
 Couldn't find this (sorry). Unless you're referring to variables named
 `ri` or `rl`?
 >
 > In nodelist.c:
 >   * In node_set_last_reachability, what is the point of setting
 ar->addrport.port and ar->addrport.addr immediately after checking whether
 those fields are equal to the values you're about to set them to?  And why
 are we copying ap->addr into *addr?
 Clean, tidied and a memory leak fixed (I think).
 >   * The other new functions here need documentation.
 I've done my best
 >   * addr_replied should have a name that makes clear that it's a
 predicate.  Perhaps  "addr_has_replied"? Similarly with
 all_listenres_have_Replied.  Also, its arguments should be const.
 Fixed, they now have 'has/have' in their names. And params are `const X`.
 >   * all_listeners_have_replied looks a little expensive.  We should make
 sure it isn't in a critical path.
 I'll let someone more familiar with the codebase as a whole speak to this.
 >   * Some of these functions should probably be static.  Or STATIC, if
 they are getting tested directly.
 Namely? Does making `all_listeners_have_replied` and `addr_has_replied`
 static make sence. Can't see any issues with this myself. I won't commit
 this yet though.
 >
 > test_addr.c:
 >   * Maybe some of these tests should use the test mocking functionality,
 so that they don't depend so heavily on the actual IP addresses of the
 running host?
 I've tried to mock out `get_interface_addresses_raw`, which seems to be
 the root function in getting "real" addresses. Does this approach make
 sense, and have I done so correctly?
 >
 > Throughout:
 >   * Probably most of the loops should be declaring the type as "const
 tor_addr_t *", not "tor_addr_t *".
 Will work on this.
 >   * Try building this branch after running configure with "--enable-gcc-
 warnings".  That will turn on a bunch of warnings that aren't on by
 default, but which we use to write cleaner code.
 It's clean now!
 >   * Most tor_addr_compare usage could be more cleanly written as
 tor_addr_eq.
 Done
 >   * Try to write if-else statements with braces on both the "if" and
 "else" clauses.  Leaving braces out makes the code a little harder to
 review, since I need to make sure that the indentation is right.
 Will work on this too.
 >   * This could still use more testing.  Taking a look at the results of
 running the test coverage tools (see doc/HACKING for more info), I see
 that only about 1/3 of the new or modified lines have any tests reaching
 them.
 This I will work on. I have gcov working now.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/9729#comment:35>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online