Browse Source

Fix a lovely heisenbug in rend_cache/store_v2_desc_as_client

Act I.

    "                    But that I am forbid
     To tell the secrets of my prison-house,
     I could a tale unfold..."

Here's the bug: sometimes, rend_cache/store_v2_desc_as_client would
say:

"Dec 15 08:31:26.147 [warn] rend_cache_store_v2_desc_as_client():
   Bug: Couldn't decode base32 [scrubbed] for descriptor id. (on Tor
   0.3.0.0-alpha-dev 4098bfa26073551f)"

When we merged ade5005853c17b3 back in 0.2.8.1-alpha, we added that
test: it mangles the hidden service ID for a hidden service, and
ensures that when the descriptor ID doesn't match the descriptor's
key, we don't store the descriptor.

How did it mangle the descriptor ID?  By doing
     desc_id_base32[0]++;

So, if the hidden service ID started with z or 7, we'd wind up with an
invalid base32 string, and get the warning.  And if it started with
any other character, we wouldn't.

That there is part 1 of the bug: in 2/32 cases, we'd get a BUG
warning.  But we wouldn't display it, since warnings weren't shown
from the unit tests.

Act II.

    "Our indiscretion sometime serves us well,
     When our deep plots do pall"

Part two: in 0.2.9.3-alpha, for part of #19999, we turned on BUG
warnings in the unit tests, so that we'd actually start seeing them.
At this point we also began to consider each BUG warning that made
it through the unit tests to be an actual bug.  So before this
point, we wouldn't actually notice anything happening in those 2/32
cases.

So, at this point it was a nice random _visible_ bug.

Act III.

   "Our thoughts are ours, their ends none of our own"

In acbb60cd6310d30c8cb763, which was part of my prop220 work, I
changed how RSA key generation worked in the unit tests.  While
previously we'd use pre-made RSA keys in some cases, this change
made us use a set of pregenerated RSA keys for _all_ 1024 or 2048
keys, and to return them in a rotation when Tor tried to generate a
key.

And now we had the heisenbug: anything that affected the number of
pregenerated keys that we had yielded before reaching
rend_cache/store_v2_desc_as_client would make us return a different
key, which would give us a different base32 ID, which would make the
bug occur, or not.  So as we added or removed test cases, the bug
might or might not happen.

So yeah.  Don't mangle a base32 ID like that.  Do it this way instead.
Nick Mathewson 7 years ago
parent
commit
92139b0077
1 changed files with 6 additions and 2 deletions
  1. 6 2
      src/test/test_rendcache.c

+ 6 - 2
src/test/test_rendcache.c

@@ -158,12 +158,16 @@ test_rend_cache_store_v2_desc_as_client(void *data)
   // Test incorrect descriptor ID
   rend_cache_init();
   mock_rend_query = mock_rend_data(service_id);
-  desc_id_base32[0]++;
+  char orig = desc_id_base32[0];
+  if (desc_id_base32[0] == 'a')
+    desc_id_base32[0] = 'b';
+  else
+    desc_id_base32[0] = 'a';
   ret = rend_cache_store_v2_desc_as_client(desc_holder->desc_str,
                                            desc_id_base32, mock_rend_query,
                                            NULL);
   tt_int_op(ret, OP_EQ, -1);
-  desc_id_base32[0]--;
+  desc_id_base32[0] = orig;
   rend_cache_free_all();
 
   // Test too old descriptor