[tor-bugs] #13790 [Core Tor/Tor]: Refactor and add comments to new_route_len()
Tor Bug Tracker & Wiki
blackhole at torproject.org
Tue Feb 7 17:01:58 UTC 2017
#13790: Refactor and add comments to new_route_len()
-------------------------------------------------+-------------------------
Reporter: dgoulet | Owner:
| catalyst
Type: enhancement | Status:
| accepted
Priority: Low | Milestone: Tor:
| unspecified
Component: Core Tor/Tor | Version:
Severity: Normal | Resolution:
Keywords: 026-deferrable, | Actual Points:
tor-03-unspecified-201612 |
Parent ID: | Points:
Reviewer: | Sponsor:
-------------------------------------------------+-------------------------
Comment (by catalyst):
Below are my rough notes including analysis of each call chain to
`new_route_len()`. The new code works and passes `make check` but I'm
still working on making both the new and old `new_route_len()` unit
testable.
= Summary =
Current behavior is for `new_route_len()` to return `DEFAULT_ROUTE_LEN`
if there is no chosen exit node (`exit_ei == NULL`). If there is a
chosen exit node (`exit_ei != NULL`), return `DEFAULT_ROUTE_LEN + 1`
unless `purpose == CIRCUIT_PURPOSE_TESTING` or
`purpose == CIRCUIT_PURPOSE_S_ESTABLISH_INTRO`,
in which case still return `DEFAULT_ROUTE_LEN`.
New behavior is the same except for some logging and a nonfatal
assertion if we get an unexpected purpose with a chosen exit.
Most of the complexity related to `new_route_len()` actually lies in
its callers
== New behavior ==
When there's no chosen exit node (`exit_ei == NULL`), always return
`DEFAULT_ROUTE_LEN`.
When there is a chosen exit node (`exit_ei != NULL`), explicitly
return `(DEFAULT_ROUTE_LEN + 1)` for:
* `CIRCUIT_PURPOSE_C_GENERAL`: connections to a hidden service
directory
* `CIRCUIT_PURPOSE_C_INTRODUCING`: client connecting to an
introduction point -- the hidden service chose the introduction
point so could possibly collude with it to unmask the client
* `CIRCUIT_PURPOSE_S_CONNECT_REND`: hidden service connecting to a
rendezvous point -- the client could collude with the rendezvous
point to unmask the hidden service
but return `DEFAULT_ROUTE_LEN` for:
* `CIRCUIT_PURPOSE_S_ESTABLISH_INTRO` (as in old code) -- the hidden
service chose the introduction point, so doesn't need an extra hop
* `CIRCUIT_PURPOSE_TESTING` (as in old code)
For purposes not explicitly handled, log a warning but return
`(DEFAULT_ROUTE_LEN + 1)`. This defaults to a longer path for
purposes not explicitly handled, which is safer but possibly wasteful.
== Other future work ==
`circuit_get_open_circ_or_launch()` could use refactoring: it's a
complicated 300+ line function that tail-calls itself from multiple
places.
= Details =
`new_route_len()` is a static function that currently returns
`DEFAULT_ROUTE_LEN` unless
{{{
(exit_ei &&
purpose != CIRCUIT_PURPOSE_TESTING &&
purpose != CIRCUIT_PURPOSE_S_ESTABLISH_INTRO)
}}}
in which case it returns `DEFAULT_ROUTE_LEN + 1`.
The only caller of `new_route_len()` is static function whose only
caller is `onion_pick_cpath_exit()`, which doesn't call it unless
`!state->onehop_tunnel`.
The only caller of `onion_pick_cpath_exit()` is
`circuit_establish_circuit()` in src/or/circuitbuild.c.
The only apparent caller of `circuit_establish_circuit()` is
`circuit_launch_by_extend_info()` in src/or/circuituse.c
Callers of `circuit_launch_by_extend_info()` are:
* `circuit_launch()` in src/or/circuituse.c, which passes exit_ei=NULL
* `circuit_get_open_circ_or_launch()` static function in
src/or/circuituse.c
* `rend_service_receive_introduction()` in src/or/rendservice.c, which
passes purpose=`CIRCUIT_PURPOSE_S_CONNECT_REND`, exit_ei=`rp`
* `rend_service_relaunch_rendezvous()` in src/or/rendservice.c, which
passes purpose=`CIRCUIT_PURPOSE_S_CONNECT_REND`,
exit_ei=`oldstate->chosen_exit`
* `rend_service_launch_establish_intro()` in src/or/rendservice.c,
which passes purpose=`CIRCUIT_PURPOSE_S_ESTABLISH_INTRO`,
exit_ei=`launch_ei`; the current `new_route_len()` explicitly
excludes it
* `consider_testing_reachability()` in src/or/router.c, which passes
purpose=`CIRCUIT_PURPOSE_TESTING`,
exit_ei=`extend_info_from_router(me)`; the current `new_route_len()`
explicitly excludes it
`circuit_launch()` always passes exit_ei=`NULL`.
This leaves as possible 4-hop purposes
`CIRCUIT_PURPOSE_S_CONNECT_REND` (from rendservice.c callers) and
whatever purposes `circuit_get_open_circ_or_launch()` passes in.
`circuit_get_open_circ_or_launch()` is a complicated 300+ line static
function and tail-calls itself from two places.
`circuit_get_open_circ_or_launch()` sets extend_info if
desired_circuit_purpose is `CIRCUIT_PURPOSE_C_INTRODUCE_ACK_WAIT` or
`CIRCUIT_PURPOSE_C_GENERAL`.
`circuit_get_open_circ_or_launch()` rewrites
`CIRCUIT_PURPOSE_C_INTRODUCE_ACK_WAIT` to
new_circ_purpose=`CIRCUIT_PURPOSE_C_INTRODUCING`
The only non-self caller of `circuit_get_open_circ_or_launch()` is
`connection_ap_handshake_attach_circuit()`.
`connection_ap_handshake_attach_circuit()` uses a purpose of
`CIRCUIT_PURPOSE_C_GENERAL`, `CIRCUIT_PURPOSE_C_REND_JOINED`, or
`CIRCUIT_PURPOSE_C_INTRODUCE_ACK_WAIT`.
The only non-self caller of `connection_ap_handshake_attach_circuit()`
seems to be `connection_ap_attach_pending()`, which is dispatched by
some event-oriented stuff.
A purpose of `CIRCUIT_PURPOSE_C_GENERAL` with a chosen exit can happen
due to hsdir lookup: if `use_begindir == 1` in
`connection_ap_make_link()`, `conn->chosen_exit_name` gets set.
The call chain for `connection_ap_make_link()` that sets
`use_begindir=1` is
* `directory_get_from_hs_dir()`
* `directory_initiate_command_routerstatus_rend()`
* `directory_initiate_command_rend()`
* `connection_ap_make_link()`
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/13790#comment:13>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
More information about the tor-bugs
mailing list