[tor-commits] [tor/master] Fix a lovely heisenbug in rend_cache/store_v2_desc_as_client

[nickm at torproject.org]

commit 92139b0077c8496006350f94aefbb72e29aa928f
Author: Nick Mathewson <nickm at torproject.org>
Date:   Thu Dec 15 08:42:03 2016 -0500

    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.
---
 src/test/test_rendcache.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/test/test_rendcache.c b/src/test/test_rendcache.c
index 7f72e44..0d53c78 100644
--- a/src/test/test_rendcache.c
+++ b/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