Opened 2 years ago

Closed 2 years ago

Last modified 15 months ago

#16400 closed defect (fixed)

Bug: Assertion cp failed in microdescs_parse_from_string at ../src/or/routerparse.c:4168

Reported by: torkeln Owned by:
Priority: High Milestone: Tor: 0.2.7.x-final
Component: Core Tor/Tor Version: Tor: 0.2.6.9
Severity: Keywords: routerparse.c, assertion, 026-backport, 2016-bug-retrospective
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Since upgrading tor via my linux mint debian edition 2 upgrade mechanism i get following error in /var/log/tor/debug.log whenever i try to start the tor daemon

Jun 19 13:16:47.000 [err] tor_assertion_failed_(): Bug: ../src/or/routerparse.c:4168: microdescs_parse_from_string: Assertion cp failed; aborting.
Jun 19 13:16:47.000 [err] Bug: Assertion cp failed in microdescs_parse_from_string at ../src/or/routerparse.c:4168. Stack trace:
Jun 19 13:16:47.000 [err] Bug:     /usr/bin/tor(log_backtrace+0x52) [0xb76a54a2]
Jun 19 13:16:47.000 [err] Bug:     /usr/bin/tor(tor_assertion_failed_+0x94) [0xb76b33c4]
Jun 19 13:16:47.000 [err] Bug:     /usr/bin/tor(microdescs_parse_from_string+0x6dc) [0xb76095bc]
Jun 19 13:16:47.000 [err] Bug:     /usr/bin/tor(microdescs_add_to_cache+0x6d) [0xb75c14dd]
Jun 19 13:16:47.000 [err] Bug:     /usr/bin/tor(microdesc_cache_reload+0xb1) [0xb75c2901]
Jun 19 13:16:47.000 [err] Bug:     /usr/bin/tor(get_microdesc_cache+0xab) [0xb75c2a1b]
Jun 19 13:16:47.000 [err] Bug:     /usr/bin/tor(nodelist_set_consensus+0x32d) [0xb75caa9d]
Jun 19 13:16:47.000 [err] Bug:     /usr/bin/tor(networkstatus_set_current_consensus+0x993) [0xb75c6ba3]
Jun 19 13:16:47.000 [err] Bug:     /usr/bin/tor(router_reload_consensus_networkstatus+0x7f) [0xb75c711f]
Jun 19 13:16:47.000 [err] Bug:     /usr/bin/tor(do_main_loop+0x90) [0xb75bd4a0]
Jun 19 13:16:47.000 [err] Bug:     /usr/bin/tor(tor_main+0x14f5) [0xb75c0315]
Jun 19 13:16:47.000 [err] Bug:     /usr/bin/tor(main+0x35) [0xb75b99c5]
Jun 19 13:16:47.000 [err] Bug:     /lib/i386-linux-gnu/libc.so.6(__libc_start_main+0xf3) [0xb70ba723]
Jun 19 13:16:47.000 [err] Bug:     /usr/bin/tor(+0x26a14) [0xb75b9a14]

Child Tickets

Change History (25)

comment:1 Changed 2 years ago by torkeln

  • Milestone Tor: 0.2.6.x-final deleted

comment:2 Changed 2 years ago by asn

From a first glance it seems to be triggering at:

      const char *cp = tor_memstr(s, start_of_next_microdesc-s,
                                  "onion-key");
      tor_assert(cp);

Is this related to the ed25519 stuff? But how could this not trigger before?

comment:3 Changed 2 years ago by nickm

I really doubt this is ed25519-related. My first guess would be microdesc-cache corrupion of some kind.

comment:4 Changed 2 years ago by torkeln

When invoking tor from the command line it´s working without the bug.

~$ sudo -u debian-tor tor
Jun 19 15:13:04.576 [notice] Tor v0.2.6.9 (git-145b2587d1269af4) running on Linux with Libevent 2.0.21-stable, OpenSSL 1.0.1k and Zlib 1.2.8.
Jun 19 15:13:04.576 [notice] Tor can't help you if you use it wrong! Learn how to be safe at https://www.torproject.org/download/download#warning
Jun 19 15:13:04.576 [notice] Read configuration file "/etc/tor/torrc".
Jun 19 15:13:04.579 [notice] Opening Socks listener on 127.0.0.1:9050

So it seems the bug is distribution and/or init script related.

comment:5 follow-up: Changed 2 years ago by nickm

Possibly; though possibly it's looking in a different location for its data directory, and not finding the corrupt microdescriptor cache. :) {Yes, I'm still hypothesizing corrupt cache}

comment:6 in reply to: ↑ 5 Changed 2 years ago by torkeln

Replying to nickm:

Possibly; though possibly it's looking in a different location for its data directory, and not finding the corrupt microdescriptor cache. :) {Yes, I'm still hypothesizing corrupt cache}

Is there a way to repair the corrupt cache?

comment:7 Changed 2 years ago by nickm

Make a copy of the cached-microdescs and cached-microdescs.new files (both the in the data directory) in the system tor directory, then delete them.

If that fixes everything, compress those files and email them to nickm at torproject dot org so I can debug what the problem is?

comment:8 Changed 2 years ago by cypherpunks_backup

It seems like any string not starting by "onion-key" and passed to microdescs_parse_from_string can to trigger those assert.

comment:9 Changed 2 years ago by nickm

The files look as though they begin correctly; I'll check that out though. Investigating the files now.

comment:10 Changed 2 years ago by nickm

  • Keywords 025-backport 026-backport added
  • Priority changed from normal to major

Ahh. The last one in cached-microdescs.new is truncated. Apparently that doesn't happen often, but I guess it's bad news when it does.

comment:11 Changed 2 years ago by cypherpunks_backup

Then, it should be ready for missed "onion-key" for every md.

--- routerparse.c	2015-04-06 13:30:54.000000000 +0000
+++ routerparse.c.edit	2015-06-22 17:04:52.489550356 +0000
@@ -4156,6 +4156,7 @@
 
   while (s < eos) {
     int okay = 0;
+    int digest_computed = 0;
 
     start_of_next_microdesc = find_start_of_next_microdesc(s, eos);
     if (!start_of_next_microdesc)
@@ -4165,8 +4166,10 @@
     {
       const char *cp = tor_memstr(s, start_of_next_microdesc-s,
                                   "onion-key");
-      tor_assert(cp);
-
+      if (!cp) {
+        log_fn(LOG_PROTOCOL_WARN, LD_DIR, "Malformed microdescriptor");
+        goto next;
+      }
       md->bodylen = start_of_next_microdesc - cp;
       md->saved_location = where;
       if (copy_body)
@@ -4176,6 +4179,7 @@
       md->off = cp - start;
     }
     crypto_digest256(md->digest, md->body, md->bodylen, DIGEST_SHA256);
+    digest_computed = 1;
 
     if (tokenize_string(area, s, start_of_next_microdesc, tokens,
                         microdesc_token_table, flags)) {
@@ -4243,7 +4247,7 @@
 
     md = NULL;
   next:
-    if (! okay && invalid_digests_out) {
+    if (! okay && invalid_digests_out && digest_computed) {
       smartlist_add(invalid_digests_out,
                     tor_memdup(md->digest, DIGEST256_LEN));
     }

comment:12 Changed 2 years ago by nickm

Trying to track down when this happened. Apparently it worked fine in 0.2.5, but not 0.2.6. Bisecting. Patch looks reasonable, but want to identify the cause first.

comment:13 Changed 2 years ago by cypherpunks_backup

comment:14 Changed 2 years ago by nickm

That seems correct; I got the same result.

comment:15 Changed 2 years ago by nickm

oh, the problem was originally moving the call to tokenize_string below the assertion. When it was above, the TOK_START rule on "onion-key" prevented the assertion from being reached.

comment:16 Changed 2 years ago by nickm

  • Keywords 025-backport removed

comment:17 Changed 2 years ago by nickm

  • Milestone set to Tor: 0.2.7.x-final

comment:18 follow-up: Changed 2 years ago by nickm

  • Status changed from new to needs_review

I think I actually _do_ want to include the digest in the invalid list, just so that if it *does* somehow match one in the consensus, we don't try to re-fetch it. See fix with test in my branch bug16400_026 -- how does that look?

comment:19 Changed 2 years ago by cypherpunks_backup

Looks good, but this digest stuff could be fixed this way from start while keeping proper parsing. Idea for patch (0.2.5.x):

--- /home/ubuntu/tor-0.2.6.7/mod/routerparse.c	2014-10-10 13:06:25.000000000 +0000
+++ /home/ubuntu/tor-0.2.6.7/mod/routerparse.fromstart.c	2015-06-22 18:31:53.557326382 +0000
@@ -4019,12 +4019,14 @@
 smartlist_t *
 microdescs_parse_from_string(const char *s, const char *eos,
                              int allow_annotations,
-                             saved_location_t where)
+                             saved_location_t where,
+                             smartlist_t *invalid_digests_out)
 {
   smartlist_t *tokens;
   smartlist_t *result;
   microdesc_t *md = NULL;
   memarea_t *area;
+  char digest[DIGEST256_LEN];
   const char *start = s;
   const char *start_of_next_microdesc;
   int flags = allow_annotations ? TS_ANNOTATIONS_OK : 0;
@@ -4041,10 +4043,21 @@
   tokens = smartlist_new();
 
   while (s < eos) {
+    int okay = 0;
+
     start_of_next_microdesc = find_start_of_next_microdesc(s, eos);
     if (!start_of_next_microdesc)
       start_of_next_microdesc = eos;
 
+    const char *cp = tor_memstr(s, start_of_next_microdesc-s,
+                                "onion-key");
+    size_t md_len;
+    if (cp)
+      md_len = start_of_next_microdesc - cp;
+    else
+      md_len = start_of_next_microdesc - s;
+
+    crypto_digest256(digest, s, md_len, DIGEST_SHA256);
     if (tokenize_string(area, s, start_of_next_microdesc, tokens,
                         microdesc_token_table, flags)) {
       log_warn(LD_DIR, "Unparseable microdescriptor");
@@ -4053,8 +4066,6 @@
 
     md = tor_malloc_zero(sizeof(microdesc_t));
     {
-      const char *cp = tor_memstr(s, start_of_next_microdesc-s,
-                                  "onion-key");
       tor_assert(cp);
 
       md->bodylen = start_of_next_microdesc - cp;
@@ -4121,12 +4132,17 @@
       md->ipv6_exit_policy = parse_short_policy(tok->args[0]);
     }
 
-    crypto_digest256(md->digest, md->body, md->bodylen, DIGEST_SHA256);
+    memcpy(md->digest, digest, DIGEST256_LEN);
 
     smartlist_add(result, md);
+    okay = 1;
 
     md = NULL;
   next:
+    if (! okay && invalid_digests_out) {
+      smartlist_add(invalid_digests_out,
+                    tor_memdup(digest, DIGEST256_LEN));
+    }
     microdesc_free(md);
     md = NULL;

comment:20 Changed 2 years ago by cypherpunks_backup

+ crypto_digest256(digest, s, md_len, DIGEST_SHA256);

This part wrong, need some code yet.

comment:21 Changed 2 years ago by cypherpunks_backup

--- /home/ubuntu/tor-0.2.6.7/mod/routerparse.c	2014-10-10 13:06:25.000000000 +0000
+++ /home/ubuntu/tor-0.2.6.7/mod/routerparse.fromstart.c	2015-06-22 18:41:54.509300603 +0000
@@ -4019,12 +4019,14 @@
 smartlist_t *
 microdescs_parse_from_string(const char *s, const char *eos,
                              int allow_annotations,
-                             saved_location_t where)
+                             saved_location_t where,
+                             smartlist_t *invalid_digests_out)
 {
   smartlist_t *tokens;
   smartlist_t *result;
   microdesc_t *md = NULL;
   memarea_t *area;
+  char digest[DIGEST256_LEN];
   const char *start = s;
   const char *start_of_next_microdesc;
   int flags = allow_annotations ? TS_ANNOTATIONS_OK : 0;
@@ -4041,10 +4043,25 @@
   tokens = smartlist_new();
 
   while (s < eos) {
+    int okay = 0;
+
     start_of_next_microdesc = find_start_of_next_microdesc(s, eos);
     if (!start_of_next_microdesc)
       start_of_next_microdesc = eos;
 
+    const char *cp = tor_memstr(s, start_of_next_microdesc-s,
+                                "onion-key");
+    size_t md_len;
+    const char *mdp;
+    if (cp) {
+      mdp = cp;
+      md_len = start_of_next_microdesc - cp;
+    } else {
+      mdp = s;
+      md_len = start_of_next_microdesc - s;
+    }
+
+    crypto_digest256(digest, mdp, md_len, DIGEST_SHA256);
     if (tokenize_string(area, s, start_of_next_microdesc, tokens,
                         microdesc_token_table, flags)) {
       log_warn(LD_DIR, "Unparseable microdescriptor");
@@ -4053,8 +4070,6 @@
 
     md = tor_malloc_zero(sizeof(microdesc_t));
     {
-      const char *cp = tor_memstr(s, start_of_next_microdesc-s,
-                                  "onion-key");
       tor_assert(cp);
 
       md->bodylen = start_of_next_microdesc - cp;
@@ -4121,12 +4136,17 @@
       md->ipv6_exit_policy = parse_short_policy(tok->args[0]);
     }
 
-    crypto_digest256(md->digest, md->body, md->bodylen, DIGEST_SHA256);
+    memcpy(md->digest, digest, DIGEST256_LEN);
 
     smartlist_add(result, md);
+    okay = 1;
 
     md = NULL;
   next:
+    if (! okay && invalid_digests_out) {
+      smartlist_add(invalid_digests_out,
+                    tor_memdup(digest, DIGEST256_LEN));
+    }
     microdesc_free(md);
     md = NULL;

comment:22 in reply to: ↑ 18 Changed 2 years ago by asn

Replying to nickm:

I think I actually _do_ want to include the digest in the invalid list, just so that if it *does* somehow match one in the consensus, we don't try to re-fetch it. See fix with test in my branch bug16400_026 -- how does that look?

Patch at bug16400_026 looks OK.

cypherpunks_backup approach also looks reasonable. I think the tor_assert(cp); should be removed as well.

comment:23 Changed 2 years ago by nickm

  • Resolution set to fixed
  • Status changed from needs_review to closed

Hmmm. I think I do like cpunks_backup's approach too, but this isn't a backport to 0.2.5 IIUC. I think I'll merge the one I did, but I wouldn't mind a cleanup patch for 0.2.7.

comment:24 Changed 15 months ago by nickm

  • Keywords 2016-bug-retrospective added

Mark more tickets for severe bug retrospective, based on Priority and date and hand-inspection.

comment:25 Changed 15 months ago by nickm

Mark more tickets for bug retrospective based on hand-review of changelogs from 0.2.5 onwards.

Note: See TracTickets for help on using tickets.