Opened 6 years ago

Closed 5 years ago

#9801 closed defect (fixed)

options_act thinks options have changed that haven't

Reported by: cypherpunks Owned by:
Priority: Medium Milestone: Tor: 0.2.6.x-final
Component: Core Tor/Tor Version: Tor: 0.2.4.17-rc
Severity: Keywords: tor-client
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

options_act(): Changed to using entry guards or bridges, or changed preferred or excluded node lists. Abandoning previous circuits.
options_act(): Worker-related options changed. Rotating workers.
They haven't. Tor 0.2.3 did not have this bug.

Child Tickets

Change History (15)

comment:1 Changed 6 years ago by arma

What are you doing, and what is telling you those log messages, and what's the bug?

comment:2 in reply to:  1 Changed 6 years ago by cypherpunks

Replying to arma:

What are you doing, and what is telling you those log messages, and what's the bug?

When I SIGHUP Tor it logs those messages and others to stdout. The bug is that it discards circuits when its configuration is reloaded, although nothing in it has changed. I'm using ExcludeNodes.

comment:3 Changed 6 years ago by arma

Are you using any external Tor controller, like arm or Vidalia or something else?

In short, give us a few more hints. What else would be good for us to know?

comment:4 Changed 6 years ago by nickm

Keywords: tor-client added
Status: newneeds_information

I just started a tor 0.2.4 client with a torrc consisting of a single ExcludeNodes line, and gave it a SIGHUP. It didn't tell me "Changed to using entry guards or bridges, or changed preferred or excluded node lists. Abandoning previous circuits.".

The relevant code here is:

    if ((options->UseEntryGuards && !old_options->UseEntryGuards) ||
        options->UseBridges != old_options->UseBridges ||
        (options->UseBridges &&
         !config_lines_eq(options->Bridges, old_options->Bridges)) ||
        !routerset_equal(old_options->ExcludeNodes,options->ExcludeNodes) ||
        !routerset_equal(old_options->ExcludeExitNodes,
                         options->ExcludeExitNodes) ||
        !routerset_equal(old_options->EntryNodes, options->EntryNodes) ||
        !routerset_equal(old_options->ExitNodes, options->ExitNodes) ||
        options->StrictNodes != old_options->StrictNodes) {
      log_info(LD_CIRC,
               "Changed to using entry guards or bridges, or changed "
               "preferred or excluded node lists. "
               "Abandoning previous circuits.");

The likeliest way I could see for a bug like this to happen would be if there's some code in Tor that's rewriting those options between SIGHUPs, or making them seem unequal every time we parse them; the next likeliest way would be if there's something making the torrc file look differently every time we reload it. (Any thoughts?)

If there are any ideas about how to reproduce this -- for example, what I need to put in my torrc to see this behavior -- that would help debug this one.

comment:5 Changed 5 years ago by andrea

Milestone: Tor: 0.2.4.x-finalTor: 0.2.???

comment:6 Changed 5 years ago by arma

Still waiting for hints on how to reproduce.

comment:7 Changed 5 years ago by cypherpunks

I'm able to reproduce this with

EntryNodes guard1,guard2,guard3 (choose 3 relay names)
ExcludeNodes {cc}
ExludeExitNodes {a different cc}

comment:8 Changed 5 years ago by nickm

I just tried that a few times with current git master and it didn't work for me. Maybe I was picking the wrong CCs or guards? Maybe I need to wait longer or do more activity before I sighup?

comment:9 Changed 5 years ago by rransom

Does a ‘routerset’ contain a set of routers and/or country codes, or does it only contain the set of routers? (If it's only a set of routers, then “{cc}” will be expanded at parse time, and that expansion can change over time.)

comment:10 Changed 5 years ago by nickm

routerset_equal(), if I'm reading it right, checks whether the textual elements of routerset (that is, the strings separated by the commas) are equal. So the stuff to look for would be anything that modifies the routerset from containing the split-up elements of the CSV string.

Here's what modifies routerset_t.list:

  • routerset_union().
  • routerset_add_unknown_ccs()
  • routerset_parse().

I don't see an obvious reason that one of those would get called or not called differently after a sighup. Maybe there's a non-obvious reason.

comment:11 Changed 5 years ago by qwerty1

See #7706 where this appeared. The comparison between old and new excluded routersets should be after unknown CCs have been added to the ones in torrc.

comment:12 Changed 5 years ago by qwerty1

Status: needs_informationneeds_review
--- src/or/config.c
+++ src/or/config.c
@@ -1592,20 +1592,6 @@
     return -1;
   }
 
-  config_maybe_load_geoip_files_(options, old_options);
-
-  if (geoip_is_loaded(AF_INET) && options->GeoIPExcludeUnknown) {
-    /* ExcludeUnknown is true or "auto" */
-    const int is_auto = options->GeoIPExcludeUnknown == -1;
-    int changed;
-
-    changed  = routerset_add_unknown_ccs(&options->ExcludeNodes, is_auto);
-    changed += routerset_add_unknown_ccs(&options->ExcludeExitNodes, is_auto);
-
-    if (changed)
-      routerset_add_unknown_ccs(&options->ExcludeExitNodesUnion_, is_auto);
-  }
-
   /* Check for transitions that need action. */
   if (old_options) {
     int revise_trackexithosts = 0;
@@ -1701,6 +1687,20 @@
       connection_or_update_token_buckets(get_connection_array(), options);
   }
 
+  config_maybe_load_geoip_files_(options, old_options);
+
+  if (geoip_is_loaded(AF_INET) && options->GeoIPExcludeUnknown) {
+    /* ExcludeUnknown is true or "auto" */
+    const int is_auto = options->GeoIPExcludeUnknown == -1;
+    int changed;
+
+    changed  = routerset_add_unknown_ccs(&options->ExcludeNodes, is_auto);
+    changed += routerset_add_unknown_ccs(&options->ExcludeExitNodes, is_auto);
+
+    if (changed)
+      routerset_add_unknown_ccs(&options->ExcludeExitNodesUnion_, is_auto);
+  }
+
   if (options->CellStatistics || options->DirReqStatistics ||
       options->EntryStatistics || options->ExitPortStatistics ||
       options->ConnDirectionStatistics ||

comment:13 Changed 5 years ago by nickm

Milestone: Tor: 0.2.???Tor: 0.2.6.x-final

comment:14 Changed 5 years ago by nickm

Yes, that looks good. (I think the patch is reversed, though.) Applied to master as 4da4c4c63f02e9551eaeb8ad9ce5c6f2d1f34ef9

comment:15 Changed 5 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed
Note: See TracTickets for help on using tickets.