[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

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



#9729: Make bridges publish additional ORPort addresses in their descriptor
-------------------------+-------------------------------------------------
     Reporter:  sqrt2    |      Owner:
         Type:           |     Status:  needs_revision
  enhancement            |  Milestone:  Tor: 0.2.7.x-final
     Priority:  normal   |    Version:  Tor: 0.2.5.1-alpha
    Component:  Tor      |   Keywords:  ORPort, bridge, multiple,
   Resolution:           |  addresses, 026-triaged-1, andrea-review
Actual Points:           |  Parent ID:
       Points:           |
-------------------------+-------------------------------------------------
Changes (by andrea):

 * status:  needs_review => needs_revision
 * milestone:  Tor: 0.2.6.x-final => Tor: 0.2.7.x-final


Comment:

 --- begin code review ---

 fca9c63ecefa7aef5fa6c0f20b694b8f614d31a5:

  - "Return when we can't find an address" in comments before
    get_interface_address6() is missing either NULL or the empty list,
    and should specify.  Looks like the actual behavior is to return NULL.

  - It looks like we're removing a tor_addr_is_internal() check from
    get_interface_address6(); is this correct?

  - If smartlist_len(return_addrs) == 0 at the end of the
    get_interface_addresses_raw() path in get_interface_address6(), the
 empty
    list will be leaked when the function returns NULL.

    - Okay, this is fixed in 836461227083938acf86f4f52170f0816c6266d6

  - Why is this function even called get_interface_address6() when it looks
    like it can return AF_INET or AF_INET6 addresses and now does so in the
    plural?

  - The error exit path in the non-get_interface_addresses_raw() case of
    get_interface_address6() can leak return_addrs. (looks like
    e8ea5abb681052cc8b90453cbb14b8a78efc11dd fixed this)

  - The comment for get_first_address_by_af() should document that the
    caller must free the returned tor_addr_t, if any.

    - Fixed by no longer allocating a new tor_addr_t in
      58ebb9b6d3908ee7b35fd5203252b0e2b421cbf8

  - Does this notion of firstness make any sense?  Shouldn't we be
 regarding
    addresses as logically an unordered set?  Hmm, is this used anywhere
    except for as a helper for get_stable_interface_address6()?  If not, I
    should be less worried about its exact semantics but it should be
 declared
    static.

  - get_stable_interface_address6() consists of three consecutive sections,
    each of which branch on AF_INET vs. AF_INET6 and then contain identical
    code except for using last_discovered_ipv4_address or
    last_discovered_ipv6_address.  Using a tor_addr_t ** initialized to
    point to one or the other of those static variables at the beginning of
    the function would be less redundant.

  - The new function get_first_address_by_af() is not declared static, but
    it seems to only be used in address.c and isn't added to address.h.

  - Okay, removing the tor_addr_is_internal() check from
    get_interface_address6() makes more sense in light of
    find_good_addr_from_list().

  - Why does find_good_addr_from_list() use uint32_t * rather than
    tor_addr_t *?  This looks like it won't be IPv6-friendly at all.

    - Fixed in 58ebb9b6d3908ee7b35fd5203252b0e2b421cbf8

  - These changes to resolve_my_address() look okay, modulo above-noted
 IPv6
    issues; has this function been changed since this patch was written to
    not assume IPv4, by any chance?

  - We're deleting a bunch of code from client_check_address_changed() that
    avoids redundancy using the technique I recommended above for
    get_stable_interface_address6(), and replacing it with a call to
    get_stable_interface_address6().  Can we please avoid clone-and-hack
    anti-refactoring?

  - I think the logic is correct in the many introductions of
    node_af_reachable_since() in dirserv.c.

  - Is skipping all reachability testing (in
 dirserv_single_reachability_test())
    for bridges with large numbers of addresses the right behavior?  What
 do
    bridge authorities do with this data?  Would testing a random sample of
    addresses be useful?

  - addr_replied() and all_listeners_replied() in nodelist.c should have
    explanatory comments and it looks like they should be static too.

  - I'm not sure I like this adding a list of *additional* listeners to
    routerinfo_t business; it breaks symmetry among the set of addresses
    we're listening on.  What's the motivation for singling out one address
    as primary?

  - Similar comments on the existence of
 router_get_main_listener_addr_by_af();
    is the motivation for designating one address as primary avoiding
 having
    to change a mess of old code that makes that assumption perhaps?

  - Maybe we should consider renaming router_pick_published_address() to
    match router_get_main_ipv6_listener_address() if we're going down this
    path.

  - So in the descriptor generated in router_dump_router_to_string(), the
    only distinction being preserved between main and additional or-address
    lines is that the 'main' orport comes first?

  - The ticket is full of assumptions that this is *for bridges*, but the
    same code (called via router_parse_list_from_string()) is used on
 dirauths
    to parse uploaded descriptors.  I don't see any checks that uploaded
    descriptors to dirauths *lack* extra or-address lines, so we're
 expanding
    the set of things that are valid descriptors to upload.  I don't think
    that causes any serious problems *right now* since I didn't see any
 client-
    side changes here for picking an address to use, but I'd really rather
    changes like that happen consciously.  What's the correct behavior for
 the
    dirauths here?  What will dirauths not yet running this patch do if
 they
    happen across a router descriptor with extra or-address lines?

 836461227083938acf86f4f52170f0816c6266d6:

  - Fixed the memory leak in get_interface_address6(); good.

  - A bunch of needful style fixups.

  - Fixed a comparison bug in
 router_get_active_listener_port_by_addr_type_af()
    that I didn't notice but ln5 did.

 e8ea5abb681052cc8b90453cbb14b8a78efc11dd:

  - Avoids a null pointer deref I missed.

  - Fixes memory leaks in get_stable_interface_address6() and
    get_interface_address().

  - Adds get_first_address_by_af() to address.h, but is it ever called
    from anywhere else?  Perhaps it should be static instead.

 29c149db6c9fcd496992eb5a8c23fa943a518654:

  - Adds some unit tests.  Good.

 58ebb9b6d3908ee7b35fd5203252b0e2b421cbf8:

  - Fixes some memory leaks and constness

  - Changes get_first_address_by_af() so the caller doesn't need to
    to free anything.

  - Changes around some of how we copy tor_addr_t in
    get_stable_interace_address6(); I like this better since it's
    clear where the
 last_discoverd_ipv4_address/last_discovered_ipv6_address
    are supposed to get allocated/freed, but it still seems to have
    more duplicated code than necessary; cf. my suggestions in comments
    on fca9c63ecefa7aef5fa6c0f20b694b8f614d31a5.

  - Removes the get_interface_address() function.

  - Good, all those weird uint32_ts in find_good_addr_from_list() are
    addressed here.

  - resolve_my_address() looks better; not carrying IPv4 dependencies any
    more and I hadn't thought of the awareness of user's config aspect
    this improves.

 06d76fe4025d38db9ddec2f134a371324b09d001:

  - This is just a merge of 58ebb9b6d3908ee7b35fd5203252b0e2b421cbf8.
    Looks fine to me.

 4698c309a1bb635ec1eb8eca5b6a25f24389126f:

  - Removes stale test, looks fine.

 5a1b50dcae0837ff7fb9afc245c74bf9f371b35d:

  - This fixes callers of get_first_address_by_af() to handle the new
    behavior introduced in 58ebb9b6d3908ee7b35fd5203252b0e2b421cbf8.
    Looks fine to me.

 fc6c446841d4081ed3a7406738b09830e372b72d:

  - Compiler appeasement which only touches test suite; looks fine to me.

 e29b61f63e2cfd0d623bb2e1f02a2b5cb385257f:

  - Comment improvements in test suite; looks fine to me.

 1ccfb795f0e5e79925de4b374221b47e12d68320:

  - More test suite tweaks; looks fine to me.

 c18593e772e43056590a72c0b0275f8300a3b0d0:

  - Style fixups in address.c and add an address family sanity check to
    get_stable_interface_address6(); looks okay to me modulo what
    75d7b0d53e6d5cbc1f1b98f7b7d341828b8d2eee covers.

 b1faa03570ccfe33a629ccd9485736ed5c8e3f68:

  - Implements a new utility function to clone a tor_addr_t; looks okay
    to me.

 485f91efaccd5f358f86dd00cba9ffc96a335961:

  - Looks fine to me.

 75d7b0d53e6d5cbc1f1b98f7b7d341828b8d2eee:

  - Compiler warning appeasement on
 b1faa03570ccfe33a629ccd9485736ed5c8e3f68;
    looks okay to me.

 c18429f19d4920d6583965626a7ebe1deb3296bb:

  - Compiler appeasement; looks fine.

 b7c07e78c0de1bdc7db316e133b2c771ad916486:

  - Looks fine to me.

 73b0af833222a3431c971c26b2d0bcd10ac82eab:

  - Compiler appeasement and use of tor_addr_clone(); looks fine to me.

 d8d610ae507eebcb505f801ac1056c53a959370f:

  - Fixes some initializations in find_good_addr_from_list()

 20718a4a0b9dd572e70d36661be5bee3d06559f0:

  - Compiler appeasement; looks fine, but why does
    router_get_main_ipv6_listener_address() have an unused param?

 3d8a714920c45bd6f03d0d6b8efb0dd47091d039:

  - This looks okay.

 e2ac66e16bf5e524101e3f75829760b2a38c1927:

  - Minor comment fix in test suite; looks fine.

 66c75a40ca6ba4e5b6cc65e835039fe8afd7d6d8:

  - This introduces copy and equality-testing functions for
 tor_addr_port_t;
    they look fine, except that tor_addr_port_eq(NULL, NULL) should perhaps
    return 1.

 f4b35098ed3783d029fef2241456f661b1ff0a19:

  - This looks fine to me

 329c30c098066e1ad3167732c4702f102260287b:

  - This looks fine but it'd sure be nice if the commit description
 mentioned
    what bugs it was fixing.

 68b473b9f2b2795a18b14f57a089f596e98e630f:

  - This looks okay

 a4fdffc975719f9b35ffe2e23fca89696f471f1c:

  - This looks okay.

 e42b225eacd6fb6650cad5c5940e30102bf1a201:

  - This looks fine to me.

 b292fc563937ffa7f3e8e9084c3306deb69f60fd:

  - This looks okay.

 6530497d8474fb50fe79e54c1556e8275ebabbc0:

  - This looks okay to me.

 75c821e60b619d9dbfd104eed36fee54f5f98b1a:

  - This looks okay to me.

 dd586150424e32bd7641552b93812ddfe470c5ca:

  - This looks okay to me.

 95ff579c205594ca07b5ae41448b3bc35fc4047f:

  - Formatting fixes; this looks okay.

 10092ddeb9df6e89ca7cae1aaf1744ee32792738:

  - This looks fine to me.

 Summary:

 This is a complicated branch and I don't like the idea of merging it this
 late in the release cycle, so I'm deferring it to 0.2.7-alpha.  I'm
 marking
 it needs_revision for the lack of consideration of effect on the dirauths
 and the broken symmetry of designating one orport as primary and the
 others
 as additional, but an acceptable revision might consist of a persuasive
 argument that it does the right thing and an added comment to that effect.

 --- end code review ---

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/9729#comment:45>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs