[tor-bugs] #9729 [Tor]: Make bridges publish additional ORPort addresses in their descriptor
Tor Bug Tracker & Wiki
blackhole at torproject.org
Fri Feb 13 01:32:06 UTC 2015
#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
More information about the tor-bugs
mailing list