Opened 9 years ago

Last modified 7 years ago

#1217 closed defect (Fixed)

>=0.2.1.3-alpha don't weight guard choice

Reported by: arma Owned by:
Priority: High Milestone: 0.2.1.22
Component: Core Tor/Tor Version: 0.2.1.21
Severity: Keywords:
Cc: arma, mikeperry, nickm, karsten, Sebastian Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Right now we're picking which guards we'll use uniformly at random from
all nodes, and we're picking which of our guards we'll use uniformly at
random from the nodes in our guard list that have the Guard flag.

We used to have this bad behavior, but we fixed it in 0.2.0.3-alpha
and 0.1.2.17. See bug 440 for more details.

But then in 0.2.1.3-alpha (git ed781e69), we refactored how we called
router_choose_random_node(), and we got it wrong. We end up never passing
CRN_NEED_GUARD in the case where we're choosing a new entry guard (where
state is NULL).

So a) we don't choose it by bandwidth, and b) we don't even see if it's got
the Guard flag -- or even the Fast flag -- when selecting it.

The fix is to call router_choose_random_node() with the NEED_GUARD flag when
state is NULL. The next piece of the fix is to modify
remove_obsolete_entry_guards() so it discards guards chosen between 0.2.1.3-alpha
and 0.2.1.21, and between 0.2.2.1-alpha and 0.2.2.6-alpha.

[Automatically added by flyspray2trac: Operating System: All]

Child Tickets

Change History (8)

comment:1 Changed 9 years ago by arma

My notes from debugging it, for posterity:

choose_random_entry() picks from among our guards,
uniformly as of master.
In 3abafccd it was uniform (Feb 2008).
In 7d1f675c we renamed it, no real change (Jan 2006)
dbd7b97e was even earlier, and also choose uniformly. (Dec 2005)

choose_good_entry_server() selects a new guard to add,
and selects it uniformly as of master.
In ed781e69 (Jul 2008), Nick changed it to build a 'flags' list, wrong.
That went into 0.2.1.3-alpha.
The original (correct) fix was git show 23ae5976, from Jun 2007.
See also bug 440.

comment:2 Changed 9 years ago by arma

The torperf runs set UseEntryGuards to 0, so they were selecting their first
hop out of the nodes that have the Guard flag right now, weighted by their
advertised bandwidth. So in that sense torperf represents the best possible
selection we could do that, since it's weighted by the bandwidth right now,
rather than weighted by the bandwidth numbers at some point in the past
up-to-two-months when we picked the node.

comment:3 Changed 9 years ago by arma

Here's my patch, on tor-021:

diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c
index 4b5ba62..90fd7b5 100644
--- a/src/or/circuitbuild.c
+++ b/src/or/circuitbuild.c
@@ -1697,7 +1697,7 @@ choose_good_entry_server(uint8_t purpose, cpath_build_stat

routerinfo_t *r, *choice;
smartlist_t *excluded;
or_options_t *options = get_options();

  • router_crn_flags_t flags = 0;

+ router_crn_flags_t flags = CRN_NEED_GUARD;

if (state && options->UseEntryGuards &&

(purpose != CIRCUIT_PURPOSE_TESTING
options->BridgeRelay)) {

@@ -1734,7 +1734,6 @@ choose_good_entry_server(uint8_t purpose, cpath_build_stat

}


if (state) {

  • flags |= CRN_NEED_GUARD;

if (state->need_uptime)

flags |= CRN_NEED_UPTIME;

if (state->need_capacity)

@@ -2206,7 +2205,11 @@ remove_obsolete_entry_guards(void)

} else if ((tor_version_as_new_as(ver, "0.1.0.10-alpha") &&

!tor_version_as_new_as(ver, "0.1.2.16-dev"))

(tor_version_as_new_as(ver, "0.2.0.0-alpha") &&

  • !tor_version_as_new_as(ver, "0.2.0.6-alpha"))) {
+ !tor_version_as_new_as(ver, "0.2.0.6-alpha"))

+ (tor_version_as_new_as(ver, "0.2.1.3-alpha") &&

+ !tor_version_as_new_as(ver, "0.2.1.22"))

+ (tor_version_as_new_as(ver, "0.2.2.0-alpha") &&
+ !tor_version_as_new_as(ver, "0.2.2.7-alpha"))) {

msg = "was selected without regard for guard bandwidth";
version_is_bad = 1;

} else if (entry->chosen_on_date + 3600*24*35 < this_month) {

comment:4 Changed 9 years ago by arma

Yeah. So. It turns out that whole "was selected without regard for
guard bandwidth" part of remove_obsolete_entry_guards() never worked.
It received ver as "0.2.1.21", and tor_version_as_new_as() says:

if (strcmpstart(platform,"Tor ")) /* nonstandard Tor; be safe and say yes */

return 1;

So all of the versions we were comparing were considered a foreign Tor.

Here's my patch that actually works:

diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c
index 4b5ba62..bf81e29 100644
--- a/src/or/circuitbuild.c
+++ b/src/or/circuitbuild.c
@@ -1697,7 +1697,7 @@ choose_good_entry_server(uint8_t purpose, cpath_build_stat

routerinfo_t *r, *choice;
smartlist_t *excluded;
or_options_t *options = get_options();

  • router_crn_flags_t flags = 0;

+ router_crn_flags_t flags = CRN_NEED_GUARD;

if (state && options->UseEntryGuards &&

(purpose != CIRCUIT_PURPOSE_TESTING
options->BridgeRelay)) {

@@ -1734,7 +1734,6 @@ choose_good_entry_server(uint8_t purpose, cpath_build_stat

}


if (state) {

  • flags |= CRN_NEED_GUARD;

if (state->need_uptime)

flags |= CRN_NEED_UPTIME;

if (state->need_capacity)

@@ -2203,13 +2202,25 @@ remove_obsolete_entry_guards(void)

} else if (tor_version_parse(ver, &v)) {

msg = "does not seem to be from any recognized version of Tor";
version_is_bad = 1;

  • } else if ((tor_version_as_new_as(ver, "0.1.0.10-alpha") &&
!tor_version_as_new_as(ver, "0.1.2.16-dev"))
  • (tor_version_as_new_as(ver, "0.2.0.0-alpha") &&
  • !tor_version_as_new_as(ver, "0.2.0.6-alpha"))) {
  • msg = "was selected without regard for guard bandwidth";
  • version_is_bad = 1;
  • } else if (entry->chosen_on_date + 3600*24*35 < this_month) {

+ } else {
+ size_t len = strlen(ver)+5;
+ char *tor_ver = tor_malloc(len);
+ tor_snprintf(tor_ver, len, "Tor %s", ver);
+ if ((tor_version_as_new_as(tor_ver, "0.1.0.10-alpha") &&

+ !tor_version_as_new_as(tor_ver, "0.1.2.16-dev"))

+ (tor_version_as_new_as(tor_ver, "0.2.0.0-alpha") &&

+ !tor_version_as_new_as(tor_ver, "0.2.0.6-alpha"))

+ /* above are bug 440; below are bug 1217 */
+ (tor_version_as_new_as(tor_ver, "0.2.1.3-alpha") &&

+ !tor_version_as_new_as(tor_ver, "0.2.1.22"))

+ (tor_version_as_new_as(tor_ver, "0.2.2.0-alpha") &&
+ !tor_version_as_new_as(tor_ver, "0.2.2.7-alpha"))) {
+ msg = "was selected without regard for guard bandwidth";
+ version_is_bad = 1;
+ }
+ tor_free(tor_ver);
+ }
+ if (!version_is_bad && entry->chosen_on_date + 3600*24*35 < this_month) {

/* It's been more than a month, and probably more like two since

  • chosen_on_date is clipped to the beginning of its month. */

msg = "was selected several months ago";

comment:5 Changed 9 years ago by arma

See http://archives.seul.org/or/dev/Jan-2010/msg00004.html
and the patch that follows it, for something that should go
in at the same time as this fix.

comment:6 Changed 9 years ago by arma

Fixed in 0.2.2.7-alpha, will be fixed in 0.2.1.23 if all goes well. Closing.

comment:7 Changed 9 years ago by arma

flyspray2trac: bug closed.

comment:8 Changed 7 years ago by nickm

Component: Tor ClientTor
Note: See TracTickets for help on using tickets.