commit 92139b0077c8496006350f94aefbb72e29aa928f Author: Nick Mathewson nickm@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