Opened 4 years ago

Closed 3 years ago

Last modified 20 months ago

#11200 closed defect (fixed)

cached consensus inteferes with DisableNetwork=1

Reported by: mcs Owned by:
Priority: High Milestone: Tor: 0.2.5.x-final
Component: Core Tor/Tor Version: Tor: 0.2.4.21
Severity: Keywords: tor-client, 024-backport, 025-triaged, andrea-review-0254, tbb-tor-backported-3.6b2, TorBrowserTeam201408, 2016-bug-retrospective
Cc: brade, mikeperry, nickm Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

When testing with a TBB 3.6b1 build on Mac OS 10.8.5, I noticed that sometimes tor tries to open a network connection even though it was started with DisableNetwork=1 (and bootstrapping proceeds but fails with a NOROUTE error).

I can also reproduce this problem with TBB 3.5.2.1 (although the error message that is displayed by Tor Launcher is less detailed).

Here is a log snippet from TBB 3.6b1 (tor 0.2.4.21):

... [notice] Bootstrapped 5%: Connecting to directory server.
... [warn] connection_connect(): Bug: Tried to open a socket with DisableNetwork set.
... [warn] Problem bootstrapping. Stuck at 5%: Connecting to directory server. (Network is unreachable; NOROUTE; count 1; recommendation warn)
... [notice] We now have enough directory information to build circuits.
... [notice] Bootstrapped 80%: Connecting to the Tor network.
... [notice] New control connection opened.
... [warn] Problem bootstrapping. Stuck at 80%: Connecting to the Tor network. (Network is unreachable; NOROUTE; count 2; recommendation warn)
... [notice] DisableNetwork is set. Tor will not make or accept non-control network connections. Shutting down all existing connections.

This occurs when there is a cached-microdesc-consensus file (so not the first time I start the Tor Browser).

Here are some steps to reproduce:

1) Grab a build from here:
https://people.torproject.org/~mikeperry/builds/3.6-beta-1/

2) Start Tor Browser and click "Connect" when the wizard opens. Let it connect.

3) Exit the browser and delete the file Data/Browser/profile.default/prefs.js.

4) Start Tor Browser a second time. You will see an error "Connecting to the Tor network failed (no route to host)."

Child Tickets

Change History (36)

comment:1 Changed 4 years ago by mcs

Cc: nickm added

comment:2 Changed 4 years ago by arma

Looks like the same as #10405. I agree this is important to solve.

comment:3 Changed 4 years ago by arma

(note that i think tor doesn't *actually* open any connections. it just starts preparing to and then stops itself.)

comment:4 Changed 4 years ago by nickm

Keywords: tor-client 024-backport added
Milestone: Tor: 0.2.5.x-final
Status: newneeds_review

I have an easy fix in branch "bug11200" in my public repository. When I enable it, the warnings go away and DisableNetwork works like a charm. Please test and/or review?

This should get an 0.2.4 backport, right after the backport of #11156 . (They alter the same function.)

Roger is right that no connections are actually opened in this case.

comment:5 Changed 4 years ago by nickm

Keywords: 025-triaged added

comment:6 Changed 4 years ago by mikeperry

I went ahead and threw this patch into TBB already. It should appear in a nightly at https://people.torproject.org/~linus/builds/ (though I'm not sure if it will be the 2013-03-29 nightly, or the next one. I don't know when they start building).

comment:7 Changed 4 years ago by nickm

Keywords: andrea-review-0254 added

Drop owners from needs_review tickets in tor 0.2.5

comment:8 Changed 4 years ago by andrea

This branch looks fine to me for merging in 0.2.5.4.

comment:9 Changed 4 years ago by mikeperry

FYI: I backported this to 0.2.4.x and it was applied to TBB 3.6-beta-2 (https://gitweb.torproject.org/builders/tor-browser-bundle.git/blob/HEAD:/gitian/patches/bug11200.patch)

comment:10 Changed 4 years ago by mikeperry

Keywords: tbb-tor-backported-3.6b2 added

comment:11 Changed 4 years ago by nickm

Milestone: Tor: 0.2.5.x-finalTor: 0.2.4.x-final

Merged to 0.2.5; marking for possible backport.

comment:12 Changed 4 years ago by nickm

Recommendation, no backport to 0.2.4. (persuade arma that this is causing huge problems in 0.2.4 if I'm wrong.)

comment:13 Changed 4 years ago by arma

Fine to not backport I think -- it's only a log complaint after all.

comment:14 Changed 4 years ago by nickm

Milestone: Tor: 0.2.4.x-finalTor: 0.2.5.x-final
Resolution: fixed
Status: needs_reviewclosed

Okay, no backport to 0.2.4 for these, for stability reasons.

comment:15 Changed 3 years ago by cypherpunks

Revert this patch, this can't handle case of router_have_minimum_dir_info() which caches returned value. If to disable network and later to enable it then no new circuits till some changes for directory information happen.

comment:16 in reply to:  15 Changed 3 years ago by nickm

Resolution: fixed
Status: closedreopened

Replying to cypherpunks:

Revert this patch, this can't handle case of router_have_minimum_dir_info() which caches returned value. If to disable network and later to enable it then no new circuits till some changes for directory information happen.

reopening

comment:17 Changed 3 years ago by arma

This patch, applied to the Tor 0.2.4.x in Tor Browser, could be responsible for all the "it only works once and then never again" complaints by TBB 3.6.3 users. So we should really look into it -- and while looking into it, be sure to consider all the TB Tor patches:
https://gitweb.torproject.org/builders/tor-browser-bundle.git/tree/HEAD:/gitian/patches

comment:18 Changed 3 years ago by cypherpunks

--- config.c
+++ config_2.c
@@ -1803,9 +1803,11 @@
    * server descriptor.
    */
   if (!old_options ||
-      options_transition_affects_descriptor(old_options, options))
+      options_transition_affects_descriptor(old_options, options)) {
     mark_my_descriptor_dirty("config change");
 
+    router_dir_info_changed();
+  }
   /* We may need to reschedule some directory stuff if our status changed. */
   if (old_options) {
     if (authdir_mode_v3(options) && !authdir_mode_v3(old_options))

comment:19 Changed 3 years ago by mikeperry

Keywords: TorBrowserTeam201408 added

comment:20 Changed 3 years ago by mikeperry

Priority: normalmajor

comment:21 Changed 3 years ago by cypherpunks

More clear patch.

--- config.c
+++ config_3.c
@@ -1806,6 +1806,14 @@
       options_transition_affects_descriptor(old_options, options))
     mark_my_descriptor_dirty("config change");
 
+  /* We may need to call update_router_have_min_dir_info. If network was
+   * enabled then circuits building need to be enabled too, give it chance
+   * by recalculate cached have_min_dir_info value on next call of 
+   * router_have_minimum_dir_info().
+   */ 
+  if (old_options && old_options->DisableNetwork && ! options->DisableNetwork)
+    router_dir_info_changed();
+
   /* We may need to reschedule some directory stuff if our status changed. */
   if (old_options) {
     if (authdir_mode_v3(options) && !authdir_mode_v3(old_options))

comment:22 Changed 3 years ago by cypherpunks

Not sure if it's correct behavior to return zero by router_have_minimum_dir_info() if network disabled. Why it should to report about insufficient information? It should depends on actual directory stuff not network status.

comment:23 Changed 3 years ago by nickm

Status: reopenedneeds_review

I think the right move here is to apply the second patch, and make a new ticket for 0.2.6 to try to clean up the behavior of router_have_minimum_dir_info(). (I agree that the current behavior doesn't match the name, but if we change it, I think we might have to change the behavior of everything that calls it, which feels too big for 0.2.5.x.)

comment:24 Changed 3 years ago by cypherpunks

Then patch should to trigger recalculate for any change of DisableNetwork

if (old_options && (old_options->DisableNetwork != options->DisableNetwork))

Not only for enabling network but for disabling too.

comment:25 Changed 3 years ago by arma

The patch looks good to me. I bet the Tor Browser people will want it, likely even for TB 3.6.4 that they're building now.

To be thorough, there are two other cases where should_delay_dir_fetches() returns 1 so update_router_have_minimum_dir_info() sets have_min_dir_info to 0:

  • options->UseBridges and !any_bridge_descriptors_known():
  • options->UseBridges and pt_proxies_configuration_pending():

And at first glance it seems like each of these also has edge cases where have_min_dir_info will get set to 0 and not recomputed when these cases change.

comment:26 Changed 3 years ago by arma

What would you think of a patch like this?

diff --git a/src/or/nodelist.c b/src/or/nodelist.c
index 8f87081..998cd21 100644
--- a/src/or/nodelist.c
+++ b/src/or/nodelist.c
@@ -1494,6 +1494,7 @@ update_router_have_minimum_dir_info(void)
 {
   time_t now = time(NULL);
   int res;
+  static int disabled=0;
   const or_options_t *options = get_options();
   const networkstatus_t *consensus =
     networkstatus_get_reasonably_live_consensus(now,usable_consensus_flavor());
@@ -1512,9 +1513,11 @@ update_router_have_minimum_dir_info(void)
   }
 
   if (should_delay_dir_fetches(get_options(), &delay_fetches_msg)) {
-    log_notice(LD_DIR, "Delaying directory fetches: %s", delay_fetches_msg);
+    if (!disabled)
+      log_notice(LD_DIR, "Delaying directory fetches: %s", delay_fetches_msg);
     strlcpy(dir_info_status, delay_fetches_msg,  sizeof(dir_info_status));
     res = 0;
+    disabled = 1;
     goto done;
+  } else {
+    disabled = 0;
   }
 
@@ -1566,6 +1569,7 @@ update_router_have_minimum_dir_info(void)
     control_event_client_status(LOG_NOTICE, "NOT_ENOUGH_DIR_INFO");
   }
   have_min_dir_info = res;
-  need_to_update_have_min_dir_info = 0;
+  if (!disabled)
+    need_to_update_have_min_dir_info = 0;
 }

Basically we don't cache the answer if we're returning 0 because of should_delay_dir_fetches().

comment:27 Changed 3 years ago by arma

(My patch will work best if we move the "if (!consensus) {" stanza down below the "if (should_delay_dir_fetches(get_options(), &delay_fetches_msg)) {" check.)

comment:28 Changed 3 years ago by arma

I made a bug11200-caching branch with this version of the patch.

comment:29 Changed 3 years ago by cypherpunks

bug11200-caching branch

router_have_minimum_dir_info() zeroing need_to_update_have_min_dir_info just after calling of update_router_have_minimum_dir_info(). And it's called every second by run_scheduled_events().

comment:30 Changed 3 years ago by arma

Good stuff! I have pushed a further commit.

comment:31 Changed 3 years ago by arma

(I haven't found any way to actually trigger this bug for myself, so I haven't been able to test whether my patch fixes anything in practice.)

comment:32 Changed 3 years ago by arma

For those who like refactoring, see branch bug11200-refactor, which includes the previous commits plus also moves the main check from update_router_have_minimum_dir_info() to router_have_minimum_dir_info() so it's clearer that in this case it's not actually updating the variable.

comment:33 Changed 3 years ago by arma

I managed to reproduce the bug! If you completely bootstrap your Tor Browser, and then turn it off and go offline and start it and let it fail, then then next time Tor Browser starts it will start with DisableNetwork set, thus setting up the conditions for this bug. But it only works when you completely bootstrap the first time -- otherwise it will have some directory stuff to fetch which will un-wedge you.

I tested stock maint-0.2.5 with a fully bootstrapped datadir, and encountered the bug.

Then I tested my patches here, and they appear to resolve the bug. Great.

I've pushed a bug11200-combined branch that squashes it all together.

comment:34 Changed 3 years ago by arma

I pushed my bug11200-combined, since this patch is already in Tor Browser 3.6.4 and 4.0-alpha-3.

But that doesn't mean people shouldn't review it. :)

comment:35 Changed 3 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Seems okay to me.

comment:36 Changed 20 months ago by nickm

Keywords: 2016-bug-retrospective added

Mark the last set of tickets that came up in my last-few-years changelog hand-review.

Note: See TracTickets for help on using tickets.