Dirauth sometimes exits when SafeLogging is toggled

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.

changed milestone to %Tor: 0.2.3.x-final

added component::core tor/tor milestone::Tor: 0.2.3.x-final priority::medium resolution::fixed status::closed tor-auth type::defect labels

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",

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));

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.

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>");
+        }
       }
     }
   }

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

Trac:
Status: new to needs_review

< 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.

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.

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.)

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

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 (moved).

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

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

Trac:
Status: needs_review to closed
Resolution: N/A to fixed

Trac:
Keywords: N/A deleted, tor-auth added

Trac:
Component: Tor Directory Authority to Tor

closed

mentioned in issue #4544 (moved)

mentioned in issue #5507 (moved)

moved to tpo/core/tor#4438 (closed)

mentioned in issue tpo/core/tor#5507 (closed)