Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#4438 closed defect (fixed)

Dirauth sometimes exits when SafeLogging is toggled

Reported by: Sebastian Owned by:
Priority: Medium Milestone: Tor: 0.2.3.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-auth
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

When toggling SafeLogging, this sometimes happens:

[info] router_add_to_routerlist(): Dropping descriptor that we already have for router $F2044413DAC2E02E3D6BCF4735A19BCA1DE97281=gabelmoo at 212.112.245.170
[info] dirserv_add_descriptor(): Did not add descriptor from 'gabelmoo' (source: self): Router descriptor was not new..
[err] Unable to add own descriptor to directory: Router descriptor was not new.
[warn] options_act(): Bug: Error initializing keys; exiting
[err] set_options(): Bug: Acting on config options left us in a broken state. Dying.

Child Tickets

Change History (13)

comment:1 Changed 8 years ago by Sebastian

I think the first part of a fix is along these lines:

@@ -3314,6 +3314,8 @@ router_add_to_routerlist(routerinfo_t *router, const char 
       log_info(LD_DIR, "Replacing non-bridge descriptor with bridge "
                "descriptor for router %s",
                router_describe(router));
+    } else if (router_is_me(router)) {
+      log_info(LD_DIR, "Replacing my own descriptor");
     } else {
       log_info(LD_DIR,
                "Dropping descriptor that we already have for router %s",

comment:2 Changed 8 years ago by Sebastian

A second part might be

@@ -3404,8 +3406,9 @@ router_add_to_routerlist(routerinfo_t *router, const char 
 
   /* If we have a router with the same identity key, choose the newer one. */
   if (old_router) {
-    if (!in_consensus && (router->cache_info.published_on <=
-                          old_router->cache_info.published_on)) {
+    if (!in_consensus && !router_is_me(router) &&
+        (router->cache_info.published_on <=
+         old_router->cache_info.published_on)) {
       /* Same key, but old.  This one is not listed in the consensus. */
       log_debug(LD_DIR, "Not-new descriptor for router %s",
                 router_describe(router));

comment:3 Changed 8 years ago by Sebastian

I'm bound to screw this up because I'm always unsure which router is saved where. I'd appreciate it if someone else could take a look, too.

comment:4 Changed 8 years ago by Sebastian

Here's a better patch idea, inspired by a comment from frosty_un.

diff --git a/src/or/router.c b/src/or/router.c
index c9f141b..c2700a0 100644
--- a/src/or/router.c
+++ b/src/or/router.c
@@ -646,15 +646,26 @@ init_keys(void)
       return -1;
     }
     if (mydesc) {
+      was_router_added_t added;
       ri = router_parse_entry_from_string(mydesc, NULL, 1, 0, NULL);
       if (!ri) {
         log_err(LD_GENERAL,"Generated a routerinfo we couldn't parse.");
         return -1;
       }
-      if (!WRA_WAS_ADDED(dirserv_add_descriptor(ri, &m, "self"))) {
-        log_err(LD_GENERAL,"Unable to add own descriptor to directory: %s",
-                m?m:"<unknown error>");
-        return -1;
+      added = dirserv_add_descriptor(ri, &m, "self");
+      if (!WRA_WAS_ADDED(added)) {
+        if (WRA_WAS_REJECTED) {
+          log_err(LD_GENERAL, "Unable to add own descriptor to directory: %s",
+                  m?m:"<unknown error>");
+          return -1;
+        } else {
+          /* If the descriptor wasn't rejected, that's ok. This can happen
+           * when some config options are toggled that affect workers, but
+           * we don't really need new keys yet so the descriptor doesn't
+           * change and the old one is still fresh. */
+          log_info(LD_GENERAL, "Couldn't add own descriptor to directory: %s",
+                   m?m:"unknown error>");
+        }
       }
     }
   }

comment:5 Changed 8 years ago by Sebastian

Status: newneeds_review

eh, obviously not quite right there (WRA_WAS_REJECTED should really get an argument). Making it a proper branch now that I'm more convinced it's a good idea. bug4438 in my repository

comment:6 Changed 8 years ago by Sebastian

< frosty_un> I'm worring about future changes of returned values of router_add_to_routerlist(), so it can be easy missed by info level. Probably need aalow only one ROUTER_WAS_NOT_NEW reason if no success.

comment:7 Changed 8 years ago by cypherpunks

options_transition_affects_workers() must be split over two func. The one should trigger reinit keys func, and another affect CPU and DNS workers rotation.

comment:8 Changed 8 years ago by arma

Also note that some of the things that 'affect workers' only affect them if we're forking, not threading. (I'm not sure if it's worth it to complexify further though.)

comment:9 Changed 8 years ago by nickm

See also #4454 , closed as a duplicate of this.

comment:10 Changed 8 years ago by nickm

I think that the fix in your bug4438 branch looks basically good to me, Sebastian.

I think frosty_un's comment above (if I am reading it right) would be well addressed by tweaking the info-level log message to say that this isn't actually a problem, and switching the logic so that instead of checking for WRA_WAS_REJECTED(x), we check for x != WAS_NOT_NEW or !WRA_WAS_OUTDATED(x). That'd bulletproof the logic against future changes to the enum , and bulletproof the message against user confusion.

cypherpunks's comment is also a fine idea, but seems to be a different ticket from this one. I've split it out into #5507.

Please have a look at my branch bug4438 , based on Sebastian's?

comment:11 Changed 7 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Fixed a comment typo in my bug4438; see bug4438-v2; still seems like a good idea. Merging.

comment:12 Changed 7 years ago by nickm

Keywords: tor-auth added

comment:13 Changed 7 years ago by nickm

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